<div dir="ltr">Hi, I'm seeing two SimplifyCFG tests crashing after this commit. Backtrace is:<div><br></div><div><segfault handler invoked></div><div> #5 0x000056442fcb15d4 llvm::PHINode::getBasicBlockIndex(llvm::BasicBlock const*) const </div><div> #6 0x00005644324808b3 (anonymous namespace)::SimplifyCFGOpt::simplifyCommonResume(llvm::ResumeInst*) </div><div> #7 0x000056443246e65c (anonymous namespace)::SimplifyCFGOpt::simplifyResume(llvm::ResumeInst*, llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>&) </div><div> #8 0x000056443246cc03 (anonymous namespace)::SimplifyCFGOpt::simplifyOnce(llvm::BasicBlock*) </div><div> #9 0x000056443246c780 (anonymous namespace)::SimplifyCFGOpt::run(llvm::BasicBlock*) <br></div><div><br></div><div>Failing tests are:</div><div><br></div><div>test/Transforms/SimplifyCFG/bug-25299.ll<br>test/Transforms/SimplifyCFG/invoke_unwind.ll<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 19 May 2020 at 11:42, Jay Foad via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: Jay Foad<br>
Date: 2020-05-19T19:34:49+01:00<br>
New Revision: 59f49f7ee7f3397e000f7e11facb4a5605cd1cab<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/59f49f7ee7f3397e000f7e11facb4a5605cd1cab.diff</a><br>
<br>
LOG: [IR] Simplify BasicBlock::removePredecessor. NFCI.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D80141" rel="noreferrer" target="_blank">https://reviews.llvm.org/D80141</a><br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
llvm/include/llvm/IR/BasicBlock.h<br>
llvm/lib/IR/BasicBlock.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h<br>
index 1210ed1eb2ef..312d2a0569ad 100644<br>
--- a/llvm/include/llvm/IR/BasicBlock.h<br>
+++ b/llvm/include/llvm/IR/BasicBlock.h<br>
@@ -370,12 +370,8 @@ class BasicBlock final : public Value, // Basic blocks are data objects also<br>
/// except operator delete.<br>
void dropAllReferences();<br>
<br>
- /// Notify the BasicBlock that the predecessor \p Pred is no longer able to<br>
- /// reach it.<br>
- ///<br>
- /// This is actually not used to update the Predecessor list, but is actually<br>
- /// used to update the PHI nodes that reside in the block. Note that this<br>
- /// should be called while the predecessor still refers to this block.<br>
+ /// Update PHI nodes in this BasicBlock before removal of predecessor \p Pred.<br>
+ /// Note that this function does not actually remove the predecessor.<br>
void removePredecessor(BasicBlock *Pred, bool KeepOneInputPHIs = false);<br>
<br>
bool canSplitPredecessors() const;<br>
<br>
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp<br>
index a9fa7cae92d8..fb4639c675d5 100644<br>
--- a/llvm/lib/IR/BasicBlock.cpp<br>
+++ b/llvm/lib/IR/BasicBlock.cpp<br>
@@ -317,58 +317,31 @@ iterator_range<BasicBlock::phi_iterator> BasicBlock::phis() {<br>
return make_range<phi_iterator>(P, nullptr);<br>
}<br>
<br>
-/// This method is used to notify a BasicBlock that the<br>
-/// specified Predecessor of the block is no longer able to reach it. This is<br>
-/// actually not used to update the Predecessor list, but is actually used to<br>
-/// update the PHI nodes that reside in the block. Note that this should be<br>
-/// called while the predecessor still refers to this block.<br>
-///<br>
+/// Update PHI nodes in this BasicBlock before removal of predecessor \p Pred.<br>
+/// Note that this function does not actually remove the predecessor.<br>
void BasicBlock::removePredecessor(BasicBlock *Pred,<br>
bool KeepOneInputPHIs) {<br>
- assert((hasNUsesOrMore(16)||// Reduce cost of this assertion for complex CFGs.<br>
+ // Use hasNUsesOrMore to bound the cost of this assertion for complex CFGs.<br>
+ assert((hasNUsesOrMore(16) ||<br>
find(pred_begin(this), pred_end(this), Pred) != pred_end(this)) &&<br>
- "removePredecessor: BB is not a predecessor!");<br>
-<br>
- if (InstList.empty()) return;<br>
- PHINode *APN = dyn_cast<PHINode>(&front());<br>
- if (!APN) return; // Quick exit.<br>
-<br>
- // If there are exactly two predecessors, then we want to nuke the PHI nodes<br>
- // altogether.<br>
- unsigned max_idx = APN->getNumIncomingValues();<br>
- assert(max_idx != 0 && "PHI Node in block with 0 predecessors!?!?!");<br>
-<br>
- // <= Two predecessors BEFORE I remove one?<br>
- if (max_idx <= 2 && !KeepOneInputPHIs) {<br>
- // Yup, loop through and nuke the PHI nodes<br>
- while (PHINode *PN = dyn_cast<PHINode>(&front())) {<br>
- // Remove the predecessor first.<br>
- PN->removeIncomingValue(Pred, !KeepOneInputPHIs);<br>
-<br>
- // If the PHI _HAD_ two uses, replace PHI node with its now *single* value<br>
- if (max_idx == 2) {<br>
- if (PN->getIncomingValue(0) != PN)<br>
- PN->replaceAllUsesWith(PN->getIncomingValue(0));<br>
- else<br>
- // We are left with an infinite loop with no entries: kill the PHI.<br>
- PN->replaceAllUsesWith(UndefValue::get(PN->getType()));<br>
- getInstList().pop_front(); // Remove the PHI node<br>
- }<br>
+ "Pred is not a predecessor!");<br>
<br>
- // If the PHI node already only had one entry, it got deleted by<br>
- // removeIncomingValue.<br>
- }<br>
- } else {<br>
- // Okay, now we know that we need to remove predecessor #pred_idx from all<br>
- // PHI nodes. Iterate over each PHI node fixing them up<br>
- PHINode *PN;<br>
- for (iterator II = begin(); (PN = dyn_cast<PHINode>(II)); ) {<br>
- ++II;<br>
- PN->removeIncomingValue(Pred, false);<br>
- // If all incoming values to the Phi are the same, we can replace the Phi<br>
- // with that value.<br>
- Value* PNV = nullptr;<br>
- if (!KeepOneInputPHIs && (PNV = PN->hasConstantValue())) {<br>
+ // Return early if there are no PHI nodes to update.<br>
+ if (!isa<PHINode>(begin()))<br>
+ return;<br>
+ unsigned NumPreds = cast<PHINode>(front()).getNumIncomingValues();<br>
+<br>
+ // Update all PHI nodes.<br>
+ for (iterator II = begin(); isa<PHINode>(II);) {<br>
+ PHINode *PN = cast<PHINode>(II++);<br>
+ PN->removeIncomingValue(Pred);<br>
+ // If we have a single predecessor, removeIncomingValue erased the PHI node<br>
+ // itself.<br>
+ // FIXME in practice "KeepOneInputPHIs" means "KeepConstantPHIs" and some<br>
+ // callers seem to rely on that.<br>
+ if (NumPreds > 1 && !KeepOneInputPHIs) {<br>
+ if (Value *PNV = PN->hasConstantValue()) {<br>
+ // Replace the PHI node with its constant value.<br>
PN->replaceAllUsesWith(PNV);<br>
PN->eraseFromParent();<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>