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

Fri Oct 25 11:30:43 PDT 2019

arsenm 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. :)

Mostly that I haven't bothered to add a combiner pass for AMDGPU yet

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69367