This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][AArch64][AMDGPU][X86] Teach LegalizationArtifactCombiner to combine trunc(g_constant).
ClosedPublic

Authored by craig.topper on Oct 24 2019, 12:23 AM.

Details

Summary

This allows X86 to properly form shift by immediate instructions
since we require an 8-bit constant to match the imported
SelectionDAG patterns.

Event Timeline

craig.topper created this revision.Oct 24 2019, 12:23 AM
This revision is now accepted and ready to land.Oct 24 2019, 10:54 AM
dsanders accepted this revision.Oct 24 2019, 8:01 PM

LGTM too

I did implement the same thing in D56706 which @aemerson and @aditya_nandakumar disagreed with

Looking at 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?

The discussion in 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. :)

Looking at 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?

The discussion in 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

Looking at 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?

The discussion in 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).