Page MenuHomePhabricator

[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
6758

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
6758

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
6750

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

6758

Got it - thanks.

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

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
6812

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
6491

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
6491

Will merge them

6812

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
6736–6773

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).

6737

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
6736–6773

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.

6737

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
6736–6773

Could you add this explanation to the comment?

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

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.