This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Improve splat detection.
ClosedPublic

Authored by ABataev on Nov 12 2021, 7:14 AM.

Details

Summary

A bunch of scalars can be treated as a splat not only if all elements
are the same but also if some of them are undefvalues.

Diff Detail

Event Timeline

ABataev created this revision.Nov 12 2021, 7:14 AM
ABataev requested review of this revision.Nov 12 2021, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 7:14 AM
vporpo added a subscriber: vporpo.Nov 12 2021, 12:15 PM
vporpo added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
257–258

Please update the comment to mention that this also accepts undefs.

260–268

NIT: Perhaps renaming V to something like FirstNonUndef would make this more readable.

ABataev updated this revision to Diff 386922.Nov 12 2021, 12:28 PM

Address comments

RKSimon added inline comments.Nov 13 2021, 3:17 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
261

for-range loop?

270–271

Should we accept all undef cases? The comment says 'some'.

return FirstNonUndef != nullptr;
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Change it or just remove?

ABataev added inline comments.Nov 15 2021, 6:38 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Why?

ABataev updated this revision to Diff 387241.Nov 15 2021, 6:47 AM

Rebase + address comments.

RKSimon accepted this revision.Nov 15 2021, 7:33 AM

LGTM - cheers

This revision is now accepted and ready to land.Nov 15 2021, 7:33 AM
This revision was landed with ongoing or failed builds.Nov 15 2021, 7:51 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Entry->Scalars[0] can be undef with this patch, for this case the output is uninformative.

ABataev added inline comments.Nov 15 2021, 9:17 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Hard to tell, this is still the very first element of the entry.

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

Hmm, but why do we output the very first element specially? If Scalars = { undef, add .., add .., add .. }, we label this as <splat> undef. I would just remove this block at all, since "splat" now doesn't mean what it meant before.

ABataev added inline comments.Nov 15 2021, 9:51 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Better to output all the scalars here, I think

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

That's exactly what I mean by removing this if-block which early returns otherwise.

ABataev added inline comments.Nov 15 2021, 10:02 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

So, you suggest to remove << *Entry->Scalars[0]; and ёкуегкт Str;? We can keep splat` label here.

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

Ok, keep "splat" label, but remove return Str; below.

ABataev added inline comments.Nov 15 2021, 10:08 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2596

Ok, will do in a separate patch