Page MenuHomePhabricator

[SLP]Fix crash on reordering of ScatterVectorize nodes.
ClosedPublic

Authored by ABataev on May 25 2022, 7:18 AM.

Details

Summary

ScatterVectorize nodes should be handled same way as gathers in
reorderBottomToTop function, since we can simple reorder the loads in
this node. Because of that need to include such nodes to the list of
gathered nodes to fix compiler crash.

Diff Detail

Event Timeline

ABataev created this revision.May 25 2022, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 7:18 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
ABataev requested review of this revision.May 25 2022, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 7:18 AM
vporpo accepted this revision.May 25 2022, 12:18 PM

LG

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

Please add a comment about when this is expected to happen. If I understand correctly that this is happening for State == TreeEntry::ScatterVectorize. It is a bit confusing that this code is guarded by if (getVectorizedOperand()), so one would expect that TE's state would be Vectorize.

This revision is now accepted and ready to land.May 25 2022, 12:18 PM
This revision was landed with ongoing or failed builds.May 26 2022, 6:30 AM
This revision was automatically updated to reflect the committed changes.

I guess the "crash" the patch suggested to fix looked like this:
llvm-project/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8214: llvm::Value* llvm::slpvectorizer::BoUpSLP::vectorizeTree(llvm::slpvectorizer::BoUpSLP::ExtraValueToDebugLocsMap&): Assertion `Vec && "Can't find vectorizable value"' failed.

Please find a test case attached. It started to crash after this patch.

llvm/test/Transforms/SLPVectorizer/X86/scatter-vectorize-reorder.ll
2

It seems the test does not actually execute the change. At this state it did not crash before the patch.
You probably wanted "skylake" here (i.e. an AVX2 with fast gather) instead of cascadelake . Otherwise TTI's forceScalarizeMaskedGather would return true and we really do not have any ScatterVectorize nodes in the tree.

ABataev added inline comments.Wed, Jun 22, 6:55 AM
llvm/test/Transforms/SLPVectorizer/X86/scatter-vectorize-reorder.ll
2

Just probably it was committed before the patch that added forceScalarizeMaskedGather check.