[DAGCombiner] eliminate shuffle of insert element
ClosedPublic

Authored by spatel on Sat, Nov 18, 9:13 AM.

Details

Summary

I noticed this pattern in D38316 / D38388. We failed to combine a shuffle that is either repeating a scalar insertion at the same position in a vector or translated to a different element index.

Like the earlier patch, this could be an instcombine too, but since we opted to make this a DAG transform earlier, I've made this one a DAG patch too.

I don't think we need any legality checking because the new insert is identical to the existing insert except that it may have a different constant insertion operand.

The constant insertion test in test/CodeGen/X86/vector-shuffle-combining.ll was the motivation for D38756.

Diff Detail

Repository
rL LLVM
spatel created this revision.Sat, Nov 18, 9:13 AM

I don't think we need any legality checking because the new insert is identical to the existing insert except that it may have a different constant insertion operand.

We should probably document this rule somewhere... I mean, it makes sense, but it's not obvious given that we allow the legality to vary based on whether the index is constant, and some targets have operations like movss which could be modeled as an insert_vector_elt.

spatel updated this revision to Diff 124484.Mon, Nov 27, 3:38 PM

Patch updated:
Added a code comment to explain why we shouldn't need a legality check or hook to perform this fold. Not sure if that's sufficient. Let me know if there's another place or way to document the logic. Thanks!

efriedma added inline comments.Mon, Nov 27, 4:57 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15737 ↗(On Diff #124484)

Do we need to handle the case where Mask[i] < 0?

15778 ↗(On Diff #124484)

I guess there's sort of a family of related transforms here... does it make sense to optimize other cases where we know the value? A SCALAR_TO_VECTOR? A constant? A BUILD_VECTOR? Another shuffle we could examine recursively? (Not that you need to implement any of that right now, but it would make sense to note the direction you expect for future generalizations.)

spatel added inline comments.Tue, Nov 28, 9:11 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
15737 ↗(On Diff #124484)

I was going to postpone that to a follow-up because I know we can get into some gray areas with undefs in shuffle masks. Ie, is it better to preserve the undef-ness of some vector lanes or eliminate an op but then have all lanes defined?

Ok, if I add a TODO in this patch?

15778 ↗(On Diff #124484)

Good point - I haven't seen the other patterns in real code, but it's worth noting here.

spatel updated this revision to Diff 124591.Tue, Nov 28, 9:20 AM
spatel marked 2 inline comments as done.

Patch updated:

  1. Added TODO comment to check for undef shuffle mask elements.
  2. Added TODO comment to extend the search for a scalar element.
spatel updated this revision to Diff 125818.Wed, Dec 6, 2:45 PM

Patch updated:
Offline, @RKSimon noted that Mask could point to CommutedMask after it has gone out-of-scope, so I moved that out of the 'if' clause.

efriedma accepted this revision.Wed, Dec 6, 3:45 PM

LGTM

This revision is now accepted and ready to land.Wed, Dec 6, 3:45 PM
This revision was automatically updated to reflect the committed changes.