Page MenuHomePhabricator

[SLP]Allow to reorder nodes with >2 scalar values.
ClosedPublic

Authored by ABataev on May 27 2021, 6:48 AM.

Details

Summary

tryToVectorizeList function allows to reorder only 2 scalars. Patch
allows to reorder >2 scalars. Also, to avoid possible regressions, it
allows extra vectorization of the remaining parts of the scalars
elements if possible.

Part of D57059.

Diff Detail

Event Timeline

ABataev created this revision.May 27 2021, 6:48 AM
ABataev requested review of this revision.May 27 2021, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 6:48 AM
RKSimon added inline comments.May 27 2021, 8:14 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6673

Merge Sz and BoundVal?

llvm/test/Transforms/SLPVectorizer/X86/alternate-calls.ll
10–73

strip CHECK lines

ABataev updated this revision to Diff 348282.May 27 2021, 8:36 AM

Address comments

vdmitrie added inline comments.May 27 2021, 8:42 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6670

typo

6716–6717

this can be just Chain.size(). Following llvm::transform fills it in anyway.

ABataev updated this revision to Diff 348290.May 27 2021, 8:48 AM

Address comments

vdmitrie added inline comments.May 27 2021, 8:59 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7018

missed llvm:: namespace

ABataev added inline comments.May 27 2021, 9:02 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7018

It is not required, I'll remove it from other places.

ABataev updated this revision to Diff 348300.May 27 2021, 9:10 AM

Address comments

RKSimon accepted this revision.Jun 2 2021, 2:23 AM

LGTM with a few minors

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

All the calls to fixupOrderingIndices create a 'NewOrder' immediately beforehand - maybe just perform the copy inside fixupOrderingIndices and return it? Its up to you - I have no strong preference.

8072

tbh I'd prefer to see the AllowReorder arg retained (and made compulsory in tryToVectorizeList) to make it easier to grok.

llvm/test/Transforms/SLPVectorizer/X86/alternate-calls-inseltpoison.ll
7

I may be wrong but I think all the AVX modes can use the same AVX prefix?

llvm/test/Transforms/SLPVectorizer/X86/alternate-calls.ll
7

I may be wrong but I think all the AVX modes can use the same AVX prefix?

This revision is now accepted and ready to land.Jun 2 2021, 2:23 AM
ABataev added inline comments.Jun 3 2021, 8:53 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7641

Yes, thought about it too, will check once again.

8072

Ok, will restore it.

llvm/test/Transforms/SLPVectorizer/X86/alternate-calls-inseltpoison.ll
7

Will check, thanks!

This revision was landed with ongoing or failed builds.Jun 3 2021, 10:03 AM
This revision was automatically updated to reflect the committed changes.