This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Improve/fix reordering of the gathered graph nodes.
ClosedPublic

Authored by ABataev on Oct 25 2021, 7:35 AM.

Details

Summary

Gathered loads/extractelements/extractvalue instructions should be
checked if they can represent a vector reordering node too and their
order should ve taken into account for better graph reordering analysis/
Also, if the gather node has reused scalars, they must be reordered
instead of the scalars themselves.

Diff Detail

Event Timeline

ABataev created this revision.Oct 25 2021, 7:35 AM
ABataev requested review of this revision.Oct 25 2021, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 7:35 AM
RKSimon added inline comments.Oct 25 2021, 7:46 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2700

Don't all the lanes need to match for this to be Identity?

2718

Maybe pull E out once and give it a better name, rather than having it redefined by both for loops?

2755–2756

It might be slighter easier to read by returning here and breaking the if-else chain - not sure though?

2756–2776

are we guaranteed to be TE->State == TreeEntry::NeedToGather at this point? If so assert(TE->State == TreeEntry::NeedToGather) might be better.

2916–2917

return and break if-else ?

ABataev added inline comments.Oct 25 2021, 7:48 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2700

Not necessarily, partial matching is good too.

2718

Ok

2755–2756

Ok

2756–2776

We may have scattervectorize here too, at least.

2916–2917

Ok

RKSimon added inline comments.Oct 25 2021, 8:11 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2700

Maybe add a description comment then? I took this to mean we have a identity permute mask.

ABataev updated this revision to Diff 382021.Oct 25 2021, 9:14 AM

Address comments.

RKSimon accepted this revision.Oct 26 2021, 9:06 AM

LGTM, although tbh I'm still not totally clear on the 'partial identity' path

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7460

split off this NFC clang-format change

This revision is now accepted and ready to land.Oct 26 2021, 9:06 AM

LGTM, although tbh I'm still not totally clear on the 'partial identity' path

The identity is 'partial' only in terms that some of the scalars are/may be replaced by undefs, so actually, it is a regular identity, where some indeces are undefs.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7460

Did this accidentally, will restore the original code.

ABataev updated this revision to Diff 382398.Oct 26 2021, 10:38 AM

Added an extra comment and an extra check for profitable gather nodes.

This revision was landed with ongoing or failed builds.Oct 27 2021, 6:11 AM
This revision was automatically updated to reflect the committed changes.