Page MenuHomePhabricator

[SLP] Simplify indices processing for insertelements
ClosedPublic

Authored by anton-afanasyev on Feb 12 2022, 12:31 AM.

Details

Summary

Get rid of non-constant and undef indices of insertelements
at buildTree() stage. Fix bugs.

Diff Detail

Event Timeline

anton-afanasyev requested review of this revision.Feb 12 2022, 12:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2022, 12:31 AM

Something to take it account with this and D119536 - we're going to need to cherrypick these fixes for PR52275 as they affect 14.x as well - will they be able to apply cleanly to the 14.x branch?

What kind of bugs do you see with these test cases and current version of the function? I would try to keep it, better to generate UndefMaskElem if possible, rather than cut of the vectorization early.

What kind of bugs do you see with these test cases and current version of the function? I would try to keep it, better to generate UndefMaskElem if possible, rather than cut of the vectorization early.

All these test cases are crashing without patch. There are several issues with this code. First of all, see lines 4028 and 4053 -- we use already dereferenced Optional<int> Idx = *getInsertIndex(V); (it should be without *). Also MinIdx is computed but not used at all.

But the main problem is that UndefMaskElem generally makes no sense for index: the function getInsertIndex() returns _index_ rather than element, if it is undef/poison/out of range then whole vector is poison. Also this return value UndefMaskElem isn't used now anywhere, if somehow (how?) it could be useful in future, it can be added.

Something to take it account with this and D119536 - we're going to need to cherrypick these fixes for PR52275 as they affect 14.x as well - will they be able to apply cleanly to the 14.x branch?

No, they doesn't apply cleanly right now, but I'm to backport them through new workflow (https://discourse.llvm.org/t/rfc-new-automated-release-workflow-part-2/59981)

What kind of bugs do you see with these test cases and current version of the function? I would try to keep it, better to generate UndefMaskElem if possible, rather than cut of the vectorization early.

All these test cases are crashing without patch. There are several issues with this code. First of all, see lines 4028 and 4053 -- we use already dereferenced Optional<int> Idx = *getInsertIndex(V); (it should be without *). Also MinIdx is computed but not used at all.

But the main problem is that UndefMaskElem generally makes no sense for index: the function getInsertIndex() returns _index_ rather than element, if it is undef/poison/out of range then whole vector is poison. Also this return value UndefMaskElem isn't used now anywhere, if somehow (how?) it could be useful in future, it can be added.

Ok, got it. Could you also add a check to the buildvector detection function for such kind of insertelement (+ possibly insertvalue) instructions? They should not be treated as a part of buildvector, need to treat them as an initial/terminating instruction and exclude from possibly vectorizable buildvector instructions.

Ok, got it. Could you also add a check to the buildvector detection function for such kind of insertelement (+ possibly insertvalue) instructions? They should not be treated as a part of buildvector, need to treat them as an initial/terminating instruction and exclude from possibly vectorizable buildvector instructions.

Do you mean findBuildAggregate()? This check is already there, see line 9603.

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

@ABataev Do you mean this check?

ABataev added inline comments.Feb 13 2022, 1:49 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9603

Yes, but I thought about returning true here.

anton-afanasyev marked an inline comment as done.Feb 13 2022, 2:02 PM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9603

Hmm, but true means that this insertelement is a part of buildvector. Do you want this?

ABataev added inline comments.Feb 13 2022, 2:14 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9603

True means that we have a buildvector, but do not include this insertelement into the list of the vectorisable insertelements, i.e. treat it as an initial vector of the buildvector. Could you try this?

ABataev added inline comments.Feb 13 2022, 2:22 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9607–9609

I mean, maybe return true here too, if we have at least 2 insertelements in the InsertElts vector?

anton-afanasyev marked 3 inline comments as done.Feb 13 2022, 3:32 PM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
9603

Ok, I see.

9607–9609

I've simplified this a bit more, removed boolean return type of function at all, since "at least 2 insertelements" condition has already been checking at the non-recursive wrapper findBuildAggregate().

anton-afanasyev marked 2 inline comments as done.

Interpret insertelements with undef/uncorrect/non-constant index as initial vector

Would be good to test it using test suite at least, just in case we missed something.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

With an early check for insertelements in tryVectorizePair this can be turned to an assert rather than check and schedule cancellation.

anton-afanasyev marked an inline comment as done.Feb 14 2022, 1:23 AM

Would be good to test it using test suite at least, just in case we missed something.

Sure, I've tested, it's ok (no changes).

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

Yes, done in D119679

anton-afanasyev marked an inline comment as done.Feb 14 2022, 1:30 AM
ABataev added inline comments.Feb 14 2022, 3:15 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

Update this patch after D119679

anton-afanasyev marked an inline comment as done.Feb 14 2022, 3:22 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

Hmm, conversely, D119679 is based on this patch.

ABataev accepted this revision.Feb 14 2022, 3:46 AM

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

Ok, do not forget to remove this check then.

This revision is now accepted and ready to land.Feb 14 2022, 3:46 AM
anton-afanasyev marked 2 inline comments as done.Feb 14 2022, 3:49 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4026–4032

Sure, already removed.

This revision was automatically updated to reflect the committed changes.
anton-afanasyev marked an inline comment as done.