Differential D126713
[SLPVectorizer] Fix extractelement insertion point Authored by anna on May 31 2022, 11:42 AM.
Details 7d8060bc1 / D111574 exposed an assertion failure in setInsertPointAfterBundle, Updated the insertion point to avoid the assertion failure.
Diff Detail
Unit Tests Event TimelineComment Actions JFI: It also requires updating a lot of tests, around 25 in X86 with CHECK lines updated since the insertion point changed. I have not tested the other targets, but there would be others as well. Comment Actions Thanks for the patch.
Comment Actions The original patch is not the cause of the issue, it just reveals it. This patch also does not fix the issue, it just hides it. Comment Actions Could you pls elaborate? Unfortunately, I don't know what needs to be done here and I think irrespective of any vectorization "performance issue", we would need to fix the *functional problem*.
Will do.
yes, it exposed the issue and I do not know what the actual issue is. However, we don't have any forward path downstream (with applications failing with this assertion failure) and I believe the ideal solution is to revert the exposing patch, fix the issue correctly and then land it again, if the actual fix is more involved. As you can see there are a whole bunch of tests that need to be fixed as well for this (at least the X86 ones were CHECK updates) and if this is not the correct fix, I think it best to revert the change to allow unblocking failing issues. I will go ahead with the update for the test case here as requested. What do you think? Comment Actions Could you try this patch: diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp index db8f97271db6..1f33ad604d11 100644 --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp @@ -7237,18 +7237,41 @@ void BoUpSLP::setInsertPointAfterBundle(const TreeEntry *E) { auto *BB = Front->getParent(); assert(llvm::all_of(E->Scalars, [=](Value *V) -> bool { auto *I = cast<Instruction>(V); - return !E->isOpcodeOrAlt(I) || I->getParent() == BB; + return !E->isOpcodeOrAlt(I) || I->getParent() == BB || + isVectorLikeInstWithConstOps(I); })); - auto &&FindLastInst = [E, Front]() { + auto &&FindLastInst = [E, Front, this, &BB]() { Instruction *LastInst = Front; for (Value *V : E->Scalars) { auto *I = dyn_cast<Instruction>(V); if (!I) continue; - if (LastInst->comesBefore(I)) + if (LastInst->getParent() == I->getParent()) { + if (LastInst->comesBefore(I)) + LastInst = I; + continue; + } + assert(isVectorLikeInstWithConstOps(LastInst) && + isVectorLikeInstWithConstOps(I) && + "Expected vector-like insts only."); + if (!DT->isReachableFromEntry(LastInst->getParent())) { + LastInst = I; + continue; + } + if (!DT->isReachableFromEntry(I->getParent())) + continue; + auto *NodeA = DT->getNode(LastInst->getParent()); + auto *NodeB = DT->getNode(I->getParent()); + assert(NodeA && "Should only process reachable instructions"); + assert(NodeB && "Should only process reachable instructions"); + assert((NodeA == NodeB) == + (NodeA->getDFSNumIn() == NodeB->getDFSNumIn()) && + "Different nodes should have different DFS numbers"); + if (NodeA->getDFSNumIn() < NodeB->getDFSNumIn()) LastInst = I; } + BB = LastInst->getParent(); return LastInst; }; |