This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Improve registering and merging of compatible shuffles.
ClosedPublic

Authored by ABataev on Nov 17 2021, 6:45 AM.

Details

Summary

If several shuffle instructions are emitted, some of them might
same/compatible (less defined) with the previously emitted ones. Such
shuffles can be removed safely, improving the total cost of the
vectorized code.

Diff Detail

Event Timeline

ABataev created this revision.Nov 17 2021, 6:45 AM
ABataev requested review of this revision.Nov 17 2021, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 6:45 AM
RKSimon added inline comments.Nov 30 2021, 8:22 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6815

I don't understand - why do you reset LastUndefsCnt here?

ABataev added inline comments.Nov 30 2021, 8:27 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6815

LastUndefsCnt is the number of terminating undefmaskelems, that's why I reset it each time we ran into non-undef mask elem.

RKSimon added inline comments.Nov 30 2021, 8:30 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6807

Add a comment about keep track of the terminating (trailing?) undefmaskelems

6815

Got it - thanks.

ABataev added inline comments.Nov 30 2021, 8:31 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6807

Will do, thanks!

ABataev updated this revision to Diff 390752.Nov 30 2021, 10:08 AM

Address comment

RKSimon added inline comments.Nov 30 2021, 10:38 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6871

Can't we remove this dyn_cast thanks to the isa<> test above?

RKSimon added inline comments.Nov 30 2021, 10:49 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6549

Do these 2 if() have to be kept separate?

ABataev added inline comments.Nov 30 2021, 12:30 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6549

Will merge them

6871

No, we need SI here of ShuffleVectorInst type to be able to replace its shuffle mask.

vporpo added inline comments.Dec 1 2021, 10:58 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6793–6832

nit: Could you briefly explain in the comment what you mean by "less defined" by showing for example two masks where one is less defined than the other (like the ones from the lit tests below).

6794

nit: Also related to the comment above, perhaps it is worth splitting this lambda in two: IsIdentical() and LessDefined(), wdyt ?

ABataev added inline comments.Dec 1 2021, 11:28 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6793–6832

For example, if we have 2 shuffles:

shuffle %0, <0, 0, 0, undef>

and

shuffle %0, <0, 0, 0, 0>

the first shuffle is less defined than the second and can be replaced by the second one.

6794

Not sure that it will result in better code.

vporpo added inline comments.Dec 1 2021, 12:03 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6793–6832

Could you add this explanation to the comment?

ABataev added inline comments.Dec 1 2021, 12:16 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6793–6832

Sure, will do.

ABataev updated this revision to Diff 391134.Dec 1 2021, 1:56 PM

Rebase + address comments

RKSimon accepted this revision.Dec 2 2021, 6:49 AM

LGTM

This revision is now accepted and ready to land.Dec 2 2021, 6:49 AM
This revision was landed with ongoing or failed builds.Dec 2 2021, 8:49 AM
This revision was automatically updated to reflect the committed changes.