Page MenuHomePhabricator

[DAGCombine] reduceBuildVecToShuffle(): sort input vectors by decreasing size
ClosedPublic

Authored by lebedev.ri on Jun 12 2021, 2:22 PM.

Details

Summary

The sorting, obviously, must be stable, else we will have random assembly fluctuations.

Apparently there was no test coverage that would benefit from that,
so i've added one test.

The sorting consists of two parts - just sort the input vectors,
and recompute the shuffle mask -> input vector mapping.
I don't believe we need to do anything else.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 12 2021, 2:22 PM
lebedev.ri requested review of this revision.Jun 12 2021, 2:22 PM

A few nits but the premise seems sound - did you encounter real world cases that led you to this?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19124

assert message

19275

Move this inside reduceBuildVecToShuffle?

19421

assert message (or merge asserts)

A few nits but the premise seems sound

did you encounter real world cases that led you to this?

Not particularly, was just looking at the shuffle handling code and saw this.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19275

I actually want to pull this into STLExtras or something,
this seems like a common patter.

RKSimon added inline comments.Jun 13 2021, 7:41 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19275

SGTM - but until then moving it inside reduceBuildVecToShuffle is still probably cleaner - maybe add a TODO comment?

lebedev.ri marked 4 inline comments as done.

@RKSimon thank you for taking a look!
Addressing review notes.

RKSimon accepted this revision.Jun 14 2021, 5:44 AM

LGTM - cheers

This revision is now accepted and ready to land.Jun 14 2021, 5:44 AM

Thank you for the review!
I should hopefully have a few more follow-ups for createBuildVecShuffle() soon..

This revision was landed with ongoing or failed builds.Jun 14 2021, 6:19 AM
This revision was automatically updated to reflect the committed changes.
jpienaar added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19277

Seems to be failing to build for some compiler configs: https://lab.llvm.org/buildbot/#/builders/13/builds/8869/steps/6/logs/stdio

lebedev.ri marked an inline comment as done.Jun 14 2021, 6:40 AM
lebedev.ri added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19277

Yes, already fixed.