[Support] Assert that DomTree nodes share parent #101198
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-backend-aarch64 Author: Alexis Engelke (aengelke) ChangesA dominance query of a block that is in a different function is There are two cases, where this behavior did occur. LoopFuse didn't Depends on #101195, included here as first commit (only look at the second commit, will rebase once #101195 is merged). Full diff: https://github.com/llvm/llvm-project/pull/101198.diff 9 Files Affected:
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index 84ed882c6de84..ca4ce68b85cbc 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -232,7 +232,7 @@ class GenericDomTreeUpdater {
/// insertEdge/deleteEdge or is unnecessary in the batch update.
bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
- /// Erase Basic Block node that has been unlinked from Function
+ /// Erase Basic Block node before it is unlinked from Function
/// in the DomTree and PostDomTree.
void eraseDelBBNode(BasicBlockT *DelBB);
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index e05e5f0f842e3..00bf607fbc8b1 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -359,6 +359,8 @@ class DominatorTreeBase {
/// may (but is not required to) be null for a forward (backwards)
/// statically unreachable block.
DomTreeNodeBase<NodeT> *getNode(const NodeT *BB) const {
+ assert((!BB || Parent == NodeTrait::getParent(const_cast<NodeT *>(BB))) &&
+ "cannot get DomTreeNode of block with different parent");
auto I = DomTreeNodes.find(BB);
if (I != DomTreeNodes.end())
return I->second.get();
diff --git a/llvm/lib/Analysis/DomTreeUpdater.cpp b/llvm/lib/Analysis/DomTreeUpdater.cpp
index 6895317c1d03a..351bd66e389bc 100644
--- a/llvm/lib/Analysis/DomTreeUpdater.cpp
+++ b/llvm/lib/Analysis/DomTreeUpdater.cpp
@@ -42,9 +42,8 @@ bool DomTreeUpdater::forceFlushDeletedBB() {
// delete only has an UnreachableInst inside.
assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
"DelBB has been modified while awaiting deletion.");
- BB->removeFromParent();
eraseDelBBNode(BB);
- delete BB;
+ BB->eraseFromParent();
}
DeletedBBs.clear();
Callbacks.clear();
@@ -63,9 +62,8 @@ void DomTreeUpdater::deleteBB(BasicBlock *DelBB) {
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
- delete DelBB;
+ DelBB->eraseFromParent();
}
void DomTreeUpdater::callbackDeleteBB(
@@ -77,8 +75,8 @@ void DomTreeUpdater::callbackDeleteBB(
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
+ DelBB->removeFromParent();
Callback(DelBB);
delete DelBB;
}
diff --git a/llvm/lib/Analysis/TypeMetadataUtils.cpp b/llvm/lib/Analysis/TypeMetadataUtils.cpp
index 67ce1540112bb..9ec0785eb5034 100644
--- a/llvm/lib/Analysis/TypeMetadataUtils.cpp
+++ b/llvm/lib/Analysis/TypeMetadataUtils.cpp
@@ -33,6 +33,8 @@ findCallsAtConstantOffset(SmallVectorImpl<DevirtCallSite> &DevirtCalls,
// after indirect call promotion and inlining, where we may have uses
// of the vtable pointer guarded by a function pointer check, and a fallback
// indirect call.
+ if (CI->getFunction() != User->getFunction())
+ continue;
if (!DT.dominates(CI, User))
continue;
if (isa<BitCastInst>(User)) {
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 285284dc27071..f813cba0167e6 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -526,6 +526,10 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
return AllowEphemerals || !isEphemeralValueOf(Inv, CxtI);
}
+ // Inv and CxtI are in different functions.
+ if (Inv->getFunction() != CxtI->getFunction())
+ return false;
+
// Inv and CxtI are in different blocks.
if (DT) {
if (DT->dominates(Inv, CxtI))
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index d506c625d8ca5..0de8112fb72c8 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -181,8 +181,8 @@ class SSAIfConv {
bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
/// convertIf - If-convert the last block passed to canConvertIf(), assuming
- /// it is possible. Add any erased blocks to RemovedBlocks.
- void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+ /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
+ void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate = false);
};
} // end anonymous namespace
@@ -678,9 +678,9 @@ void SSAIfConv::rewritePHIOperands() {
/// convertIf - Execute the if conversion after canConvertIf has determined the
/// feasibility.
///
-/// Any basic blocks erased will be added to RemovedBlocks.
+/// Any basic blocks that need to be erased will be added to RemoveBlocks.
///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate) {
assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
@@ -721,15 +721,18 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
DebugLoc HeadDL = Head->getFirstTerminator()->getDebugLoc();
TII->removeBranch(*Head);
- // Erase the now empty conditional blocks. It is likely that Head can fall
+ // Mark the now empty conditional blocks for removal and move them to the end.
+ // It is likely that Head can fall
// through to Tail, and we can join the two blocks.
if (TBB != Tail) {
- RemovedBlocks.push_back(TBB);
- TBB->eraseFromParent();
+ RemoveBlocks.push_back(TBB);
+ if (TBB != &TBB->getParent()->back())
+ TBB->moveAfter(&TBB->getParent()->back());
}
if (FBB != Tail) {
- RemovedBlocks.push_back(FBB);
- FBB->eraseFromParent();
+ RemoveBlocks.push_back(FBB);
+ if (FBB != &FBB->getParent()->back())
+ FBB->moveAfter(&FBB->getParent()->back());
}
assert(Head->succ_empty() && "Additional head successors?");
@@ -740,8 +743,9 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
Head->splice(Head->end(), Tail,
Tail->begin(), Tail->end());
Head->transferSuccessorsAndUpdatePHIs(Tail);
- RemovedBlocks.push_back(Tail);
- Tail->eraseFromParent();
+ RemoveBlocks.push_back(Tail);
+ if (Tail != &Tail->getParent()->back())
+ Tail->moveAfter(&Tail->getParent()->back());
} else {
// We need a branch to Tail, let code placement work it out later.
LLVM_DEBUG(dbgs() << "Converting to unconditional branch.\n");
@@ -1062,11 +1066,13 @@ bool EarlyIfConverter::tryConvertIf(MachineBasicBlock *MBB) {
while (IfConv.canConvertIf(MBB) && shouldConvertIf()) {
// If-convert MBB and update analyses.
invalidateTraces();
- SmallVector<MachineBasicBlock*, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
@@ -1200,11 +1206,13 @@ bool EarlyIfPredicator::tryConvertIf(MachineBasicBlock *MBB) {
bool Changed = false;
while (IfConv.canConvertIf(MBB, /*Predicate*/ true) && shouldConvertIf()) {
// If-convert MBB and update analyses.
- SmallVector<MachineBasicBlock *, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks, /*Predicate*/ true);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks, /*Predicate*/ true);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
diff --git a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
index 49e5211af50cc..9669a393bc2b9 100644
--- a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
@@ -711,7 +711,6 @@ void SSACCmpConv::convert(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks) {
Head->updateTerminator(CmpBB->getNextNode());
RemovedBlocks.push_back(CmpBB);
- CmpBB->eraseFromParent();
LLVM_DEBUG(dbgs() << "Result:\n" << *Head);
++NumConverted;
}
@@ -918,6 +917,8 @@ bool AArch64ConditionalCompares::tryConvert(MachineBasicBlock *MBB) {
CmpConv.convert(RemovedBlocks);
Changed = true;
updateDomTree(RemovedBlocks);
+ for (MachineBasicBlock *MBB : RemovedBlocks)
+ MBB->eraseFromParent();
updateLoops(RemovedBlocks);
}
return Changed;
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 8512b2accbe7c..fe0e30d1965e0 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1729,7 +1729,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
@@ -2023,7 +2025,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
diff --git a/llvm/unittests/IR/DominatorTreeTest.cpp b/llvm/unittests/IR/DominatorTreeTest.cpp
index 44bde74ad350f..555348c65a63d 100644
--- a/llvm/unittests/IR/DominatorTreeTest.cpp
+++ b/llvm/unittests/IR/DominatorTreeTest.cpp
@@ -607,11 +607,10 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop2) {
SwitchC->removeCase(SwitchC->case_begin());
DT->deleteEdge(C, C2);
PDT->deleteEdge(C, C2);
- C2->removeFromParent();
EXPECT_EQ(DT->getNode(C2), nullptr);
PDT->eraseNode(C2);
- delete C2;
+ C2->eraseFromParent();
EXPECT_TRUE(DT->verify());
EXPECT_TRUE(PDT->verify());
|
|
@llvm/pr-subscribers-llvm-transforms Author: Alexis Engelke (aengelke) ChangesA dominance query of a block that is in a different function is There are two cases, where this behavior did occur. LoopFuse didn't Depends on #101195, included here as first commit (only look at the second commit, will rebase once #101195 is merged). Full diff: https://github.com/llvm/llvm-project/pull/101198.diff 9 Files Affected:
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index 84ed882c6de84..ca4ce68b85cbc 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -232,7 +232,7 @@ class GenericDomTreeUpdater {
/// insertEdge/deleteEdge or is unnecessary in the batch update.
bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
- /// Erase Basic Block node that has been unlinked from Function
+ /// Erase Basic Block node before it is unlinked from Function
/// in the DomTree and PostDomTree.
void eraseDelBBNode(BasicBlockT *DelBB);
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index e05e5f0f842e3..00bf607fbc8b1 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -359,6 +359,8 @@ class DominatorTreeBase {
/// may (but is not required to) be null for a forward (backwards)
/// statically unreachable block.
DomTreeNodeBase<NodeT> *getNode(const NodeT *BB) const {
+ assert((!BB || Parent == NodeTrait::getParent(const_cast<NodeT *>(BB))) &&
+ "cannot get DomTreeNode of block with different parent");
auto I = DomTreeNodes.find(BB);
if (I != DomTreeNodes.end())
return I->second.get();
diff --git a/llvm/lib/Analysis/DomTreeUpdater.cpp b/llvm/lib/Analysis/DomTreeUpdater.cpp
index 6895317c1d03a..351bd66e389bc 100644
--- a/llvm/lib/Analysis/DomTreeUpdater.cpp
+++ b/llvm/lib/Analysis/DomTreeUpdater.cpp
@@ -42,9 +42,8 @@ bool DomTreeUpdater::forceFlushDeletedBB() {
// delete only has an UnreachableInst inside.
assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
"DelBB has been modified while awaiting deletion.");
- BB->removeFromParent();
eraseDelBBNode(BB);
- delete BB;
+ BB->eraseFromParent();
}
DeletedBBs.clear();
Callbacks.clear();
@@ -63,9 +62,8 @@ void DomTreeUpdater::deleteBB(BasicBlock *DelBB) {
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
- delete DelBB;
+ DelBB->eraseFromParent();
}
void DomTreeUpdater::callbackDeleteBB(
@@ -77,8 +75,8 @@ void DomTreeUpdater::callbackDeleteBB(
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
+ DelBB->removeFromParent();
Callback(DelBB);
delete DelBB;
}
diff --git a/llvm/lib/Analysis/TypeMetadataUtils.cpp b/llvm/lib/Analysis/TypeMetadataUtils.cpp
index 67ce1540112bb..9ec0785eb5034 100644
--- a/llvm/lib/Analysis/TypeMetadataUtils.cpp
+++ b/llvm/lib/Analysis/TypeMetadataUtils.cpp
@@ -33,6 +33,8 @@ findCallsAtConstantOffset(SmallVectorImpl<DevirtCallSite> &DevirtCalls,
// after indirect call promotion and inlining, where we may have uses
// of the vtable pointer guarded by a function pointer check, and a fallback
// indirect call.
+ if (CI->getFunction() != User->getFunction())
+ continue;
if (!DT.dominates(CI, User))
continue;
if (isa<BitCastInst>(User)) {
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 285284dc27071..f813cba0167e6 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -526,6 +526,10 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
return AllowEphemerals || !isEphemeralValueOf(Inv, CxtI);
}
+ // Inv and CxtI are in different functions.
+ if (Inv->getFunction() != CxtI->getFunction())
+ return false;
+
// Inv and CxtI are in different blocks.
if (DT) {
if (DT->dominates(Inv, CxtI))
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index d506c625d8ca5..0de8112fb72c8 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -181,8 +181,8 @@ class SSAIfConv {
bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
/// convertIf - If-convert the last block passed to canConvertIf(), assuming
- /// it is possible. Add any erased blocks to RemovedBlocks.
- void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+ /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
+ void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate = false);
};
} // end anonymous namespace
@@ -678,9 +678,9 @@ void SSAIfConv::rewritePHIOperands() {
/// convertIf - Execute the if conversion after canConvertIf has determined the
/// feasibility.
///
-/// Any basic blocks erased will be added to RemovedBlocks.
+/// Any basic blocks that need to be erased will be added to RemoveBlocks.
///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate) {
assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
@@ -721,15 +721,18 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
DebugLoc HeadDL = Head->getFirstTerminator()->getDebugLoc();
TII->removeBranch(*Head);
- // Erase the now empty conditional blocks. It is likely that Head can fall
+ // Mark the now empty conditional blocks for removal and move them to the end.
+ // It is likely that Head can fall
// through to Tail, and we can join the two blocks.
if (TBB != Tail) {
- RemovedBlocks.push_back(TBB);
- TBB->eraseFromParent();
+ RemoveBlocks.push_back(TBB);
+ if (TBB != &TBB->getParent()->back())
+ TBB->moveAfter(&TBB->getParent()->back());
}
if (FBB != Tail) {
- RemovedBlocks.push_back(FBB);
- FBB->eraseFromParent();
+ RemoveBlocks.push_back(FBB);
+ if (FBB != &FBB->getParent()->back())
+ FBB->moveAfter(&FBB->getParent()->back());
}
assert(Head->succ_empty() && "Additional head successors?");
@@ -740,8 +743,9 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
Head->splice(Head->end(), Tail,
Tail->begin(), Tail->end());
Head->transferSuccessorsAndUpdatePHIs(Tail);
- RemovedBlocks.push_back(Tail);
- Tail->eraseFromParent();
+ RemoveBlocks.push_back(Tail);
+ if (Tail != &Tail->getParent()->back())
+ Tail->moveAfter(&Tail->getParent()->back());
} else {
// We need a branch to Tail, let code placement work it out later.
LLVM_DEBUG(dbgs() << "Converting to unconditional branch.\n");
@@ -1062,11 +1066,13 @@ bool EarlyIfConverter::tryConvertIf(MachineBasicBlock *MBB) {
while (IfConv.canConvertIf(MBB) && shouldConvertIf()) {
// If-convert MBB and update analyses.
invalidateTraces();
- SmallVector<MachineBasicBlock*, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
@@ -1200,11 +1206,13 @@ bool EarlyIfPredicator::tryConvertIf(MachineBasicBlock *MBB) {
bool Changed = false;
while (IfConv.canConvertIf(MBB, /*Predicate*/ true) && shouldConvertIf()) {
// If-convert MBB and update analyses.
- SmallVector<MachineBasicBlock *, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks, /*Predicate*/ true);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks, /*Predicate*/ true);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
diff --git a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
index 49e5211af50cc..9669a393bc2b9 100644
--- a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
@@ -711,7 +711,6 @@ void SSACCmpConv::convert(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks) {
Head->updateTerminator(CmpBB->getNextNode());
RemovedBlocks.push_back(CmpBB);
- CmpBB->eraseFromParent();
LLVM_DEBUG(dbgs() << "Result:\n" << *Head);
++NumConverted;
}
@@ -918,6 +917,8 @@ bool AArch64ConditionalCompares::tryConvert(MachineBasicBlock *MBB) {
CmpConv.convert(RemovedBlocks);
Changed = true;
updateDomTree(RemovedBlocks);
+ for (MachineBasicBlock *MBB : RemovedBlocks)
+ MBB->eraseFromParent();
updateLoops(RemovedBlocks);
}
return Changed;
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 8512b2accbe7c..fe0e30d1965e0 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1729,7 +1729,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
@@ -2023,7 +2025,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
diff --git a/llvm/unittests/IR/DominatorTreeTest.cpp b/llvm/unittests/IR/DominatorTreeTest.cpp
index 44bde74ad350f..555348c65a63d 100644
--- a/llvm/unittests/IR/DominatorTreeTest.cpp
+++ b/llvm/unittests/IR/DominatorTreeTest.cpp
@@ -607,11 +607,10 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop2) {
SwitchC->removeCase(SwitchC->case_begin());
DT->deleteEdge(C, C2);
PDT->deleteEdge(C, C2);
- C2->removeFromParent();
EXPECT_EQ(DT->getNode(C2), nullptr);
PDT->eraseNode(C2);
- delete C2;
+ C2->eraseFromParent();
EXPECT_TRUE(DT->verify());
EXPECT_TRUE(PDT->verify());
|
nikic
reviewed
Jul 30, 2024
aengelke
force-pushed
the
domtreecleanup4
branch
from
c8bec47 to
d0462c5
Compare
nikic
reviewed
Jul 31, 2024
nikic
requested changes
Jul 31, 2024
Yeah, Polly is somewhat broken... I tested Clang, Bolt, and MLIR but didn't think about Polly. There's this big comment above the workaround that explains the problems of fixing them. This was introduced in e3c0558 in 2014. @tobiasgrosser Any advice on the easiest way to fix Polly to not use the dominator tree for blocks outside of its function? Is removing the dominator tree(/loop info?) from Polly's CodeGen feasible with reasonably low effort? Unfortunately I don't know enough about the Polly code to do the "proper" fix (if there is such a thing). |
|
As a first approximation, we may look if the assertion failures in Polly's tests only cover OpenMP code? In that case, we could potentially just drop the OpenMP code generation? Or maybe @Meinersbur has an idea? |
|
The OpenMP outlining is the only functionality that writes outside the current function. However, this is not the only violation of the pass manager constraints (legacy/NPM). The other is that it does not preserve analyses such as (Post-)DominatorTree, LoopInfo and RegionInfo after finishing the CodeGen pass, but claims that it would. The reason is that invalidating any of those analysis would also invalidate its own analysis ScopInfo, making processing any other SCoP in the function impossible. Instead, I would suggest to make Polly a module-level pass and that pass call Polly's components itself. The "giant comment" claims that it is not possible, but that's not entirely true: Polly is free to create its own pass manager instance and requesting analyses from it. That is true with the legacy as well as the NPM and Polly does so already with the PollyCanonicalize pass. However, all is not directly related with Polly's assertion failures due to this patch. The problem is that I will prepare a patch. |
|
The more difficult problem turns out to be ScalarEvolution. It has the same problem in that Polly uses it across function boundaries, but also uses DominatorTree of which we need two (one for the original function, one for the outlined function). We also cannot just not create a DominatorTree/LoopInfo for the outline function (since it would be recomputed for the next pass anyway) because In contrast to DomTree and LoopInfo, the separation on whether the SCEV is on the original code or the new code emitted by |
|
#102460 should fix Polly's test failures. |
Meinersbur
added a commit
that referenced
this pull request
Aug 10, 2024
DominatorTree, LoopInfo, and ScalarEvolution are function-level analyses that expect to be called only on instructions and basic blocks of the function they were original created for. When Polly outlined a parallel loop body into a separate function, it reused the same analyses seemed to work until new checks to be added in #101198. This patch creates new analyses for the subfunctions. GenDT, GenLI, and GenSE now refer to the analyses of the current region of code. Outside of an outlined function, they refer to the same analysis as used for the SCoP, but are substituted within an outlined function. Additionally to the cross-function queries of DT/LI/SE, we must not create SCEVs that refer to a mix of expressions for old and generated values. Currently, SCEVs themselves do not "remember" which ScalarEvolution analysis they were created for, but mixing them is just as unexpected as using DT/LI across function boundaries. Hence `SCEVLoopAddRecRewriter` was combined into `ScopExpander`. `SCEVLoopAddRecRewriter` only replaced induction variables but left SCEVUnknowns to reference the old function. `SCEVParameterRewriter` would have done so but its job was effectively superseded by `ScopExpander`, and now also `SCEVLoopAddRecRewriter`. Some issues persist put marked with a FIXME in the code. Changing them would possibly cause this patch to be not NFC anymore.
A dominance query of a block that is in a different function is ill-defined, so assert that getNode() is only called for blocks that are in the same function. There are two cases, where this behavior did occur. LoopFuse didn't explicitly do this, but didn't invalidate the SCEV block dispositions, leaving dangling pointers to free'ed basic blocks behind, causing use-after-free. We do, however, want to be able to dereference basic blocks inside the dominator tree, so that we can refer to them by a number stored inside the basic block.
aengelke
force-pushed
the
domtreecleanup4
branch
from
5a3375f to
be63e6d
Compare
|
Rebased after #102460 landed. @Meinersbur Thanks a lot for fixing Polly! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/2297 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/1738 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/7/builds/2622 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/2327 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/2103 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/2238 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/29/builds/2073 Here is the relevant piece of the build log for the reference: |
|
I think this is caused by unclean(ccache-ed?) builds. I was able to reproduce locally at first, but in a clean working dir (with clean ccache), it worked fine. I think the "root cause" is #102180, where Not sure how to fix the bots. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/1983 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/1980 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/1928 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/25/builds/1543 Here is the relevant piece of the build log for the reference: |
|
Now where the bot spam is over, I repeat my comment from above (I don't expect anyone to look for comments in a wave of error messages...)
|
|
I don't think this is strictly related to ccache. We are seeing many internal tests fail with the exact same assertion failure message that I am now trying to bisect and highly suspect this change as it is the only one in the range that seems likely to have caused the issue. |
|
In the meantime, can we revert this change since there are so many bot failures and that continue to fail so that we can get them back to green? I will try to work on one of our internal test failures to try and get a repro for the assertion failure that will hopefully be representative if you cannot reproduce the failure from the build bots. |
vitalybuka
added a commit
that referenced
this pull request
Aug 11, 2024
This reverts commit 8101d18.
vitalybuka
added a commit
that referenced
this pull request
Aug 11, 2024
aengelke
added a commit
that referenced
this pull request
Aug 13, 2024
A dominance query of a block that is in a different function is ill-defined, so assert that getNode() is only called for blocks that are in the same function. There are three cases, where this behavior did occur. LoopFuse didn't explicitly do this, but didn't invalidate the SCEV block dispositions, leaving dangling pointers to free'ed basic blocks behind, causing use-after-free. We do, however, want to be able to dereference basic blocks inside the dominator tree, so that we can refer to them by a number stored inside the basic block. Reverts #102780 Reland #101198 Fixes #102784 Co-authored-by: Alexis Engelke <[email protected]>
kartcq
added a commit
to kartcq/llvm-project
that referenced
this pull request
Dec 18, 2024
The patch llvm#102460 already implements separate DT/LI/SE for parallel sub function. Crashes have been reported while region generator tries using oringinal function's DT while creating new parallel sub function due to checks in llvm#101198. This patch aims at fixing those cases by switching the DT/LI while generating parallel function using Region Generator. Fixes llvm#117877
kartcq
added a commit
that referenced
this pull request
Jan 8, 2025
The patch #102460 already implements separate DT/LI/SE for parallel sub function. Crashes have been reported while region generator tries using oringinal function's DT while creating new parallel sub function due to checks in #101198. This patch aims at fixing those cases by switching the DT/LI while generating parallel function using Region Generator. Fixes #117877
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A dominance query of a block that is in a different function is
ill-defined, so assert that getNode() is only called for blocks that are
in the same function.
There are two cases, where this behavior did occur. LoopFuse didn't
explicitly do this, but didn't invalidate the SCEV block dispositions,
leaving dangling pointers to free'ed basic blocks behind, causing
use-after-free. We do, however, want to be able to dereference basic
blocks inside the dominator tree, so that we can refer to them by a
number stored inside the basic block.
~~Depends on #101195, included here as first commit (only look at the second commit, will rebase once #101195 is merged).~~Rebased.