This allows X86 to properly form shift by immediate instructions
since we require an 8-bit constant to match the imported
SelectionDAG patterns.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 39980 Build 40049: arc lint + arc unit
Event Timeline
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. :)
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).