[PATCH] D69367: [GlobalISel][AArch64][AMDGPU][X86] Teach LegalizationArtifactCombiner to combine trunc(g_constant).

Fri Oct 25 11:30:44 PDT 2019

aditya_nandakumar added a comment.

In D69367#1721627 <https://reviews.llvm.org/D69367#1721627>, @paquette wrote:

> Looking at D56706 <https://reviews.llvm.org/D56706>, @aditya_nandakumar said:
>
> > The legalizer artifact combiner was originally intended only to combine away legalization artifacts - which include extends, truncates, merges, build_vectors and not for generic combines. This change looks like it should belong in CombinerHelper.
>
> I suppose we could/should just move it to CombinerHelper?
>
> @arsenm, was there any reason you didn't move it to CombinerHelper in D56706 <https://reviews.llvm.org/D56706>?
>
> The discussion in D56706 <https://reviews.llvm.org/D56706> implies that part of the argument against putting it here is that this change isn't required for correctness. However, IIRC from a dev meeting discussion with @craig.topper this is required for most targets so it feels like it could be appropriate in either place. I don't have a strong opinion here though, so I'm personally happy either way. :)

I think @paquette summed up my thoughts here. 
I think at that point we felt if it isn't strictly required for correctness then it should probably go to the CombinerHelper. Now that we know this is required for correctness, I'm not sure 100% where this should belong. I suppose this is tiny enough now and can be here in the artifact combiner - but if we start adding more such trivial transformations, then it probably makes sense to move it to a combiner helper (so we don't re-implement all of this there as well).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69367/new/

https://reviews.llvm.org/D69367