This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Enhance SLPVectorizer to vectorize different combinations of aggregates
ClosedPublic

Authored by anton-afanasyev on Nov 22 2019, 1:25 AM.

Details

Summary

Make SLPVectorize to recognize homogeneous aggregates like
{<2 x float>, <2 x float>}, {{float, float}, {float, float}},
[2 x {float, float}] and so on.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 1:25 AM

Continuation of https://reviews.llvm.org/D70068. I've merged findBuildVector() and findBuildAggregate() to one findBuildAggregate() function making it recursive to recognize multidimensional aggregates. Aggregates required to be homogeneous.

anton-afanasyev edited the summary of this revision. (Show Details)Nov 22 2019, 9:21 AM
RKSimon added inline comments.Nov 22 2019, 9:34 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3103

cast<SequentialType>

anton-afanasyev marked an inline comment as done.Nov 22 2019, 9:42 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3103

Yes, thanks.

anton-afanasyev marked an inline comment as done.Nov 22 2019, 9:50 AM

A couple of minors - does anyone else have any comments?

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

Update the description?

6960

If the insertion index isn't constant we should call getVectorInstrCost with a ~0/-1 value

anton-afanasyev marked 3 inline comments as done.Nov 25 2019, 3:20 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
637

Yes, done.

6960

Hmm, what is ~0/-1 value exactly?
Also, I'm not sure the cost tuning is related to this commit -- the current logic is taken unchanged as it was before.

anton-afanasyev marked an inline comment as done.

Fix comment

ABataev added inline comments.Nov 25 2019, 3:27 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2417

Why is this commented out?

anton-afanasyev marked 2 inline comments as done.Nov 25 2019, 3:38 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
2417

Oops, mistakenly updated it last time. Fixing.

anton-afanasyev marked an inline comment as done.

Fixed

RKSimon added inline comments.Nov 25 2019, 3:52 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6960

OK, that's acceptable for now but we should be enabling this for non-const indices as well in a future patch.

getVectorInstrCost recognizes the 'all bits set' insert/extract index argument as a non-constant index (which on most targets has a high cost).

@spatel is currently looking at this on x86 as part of PR43605

spatel added inline comments.Nov 25 2019, 5:23 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6960

I was wondering about that -1 setting too. It's confusing because of the the way the cost model is organized with abstract class, concrete class, overrides, etc. But there is this documentation comment in TargetTransformInfo.h:

/// \return The expected cost of vector Insert and Extract.
/// Use -1 to indicate that there is no information on the index value.
int getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index = -1) const;
vporpo added inline comments.Nov 25 2019, 4:26 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6941–6945

Just a minor comment: Well, this function does not check for homogeneity, this is done by canMapToVector(), so the "homogeneous" in the comment is a bit misleading. It looks like this function can actually handle heterogeneous types just fine, as long as they get accepted by canMapToVector().

anton-afanasyev marked 2 inline comments as done.Nov 26 2019, 12:12 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6941–6945

Ok, I've removed this misleading word.

anton-afanasyev marked an inline comment as done.

Comment fix

ABataev added inline comments.Dec 2 2019, 2:14 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
634

Homogeneous

This revision is now accepted and ready to land.Dec 3 2019, 6:45 AM
This revision was automatically updated to reflect the committed changes.