[SCEV] Fix a hang introduced by collectForPHI

[preames]

If we have a phi where one of it's source blocks is an unreachable block, we don't want to traverse back into the unreachable region. Doing so allows e.g. finding a trivial self loop when walking back the predecessor chain.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Philip Reames (preames)

Changes

If we have a phi where one of it's source blocks is an unreachable block, we don't want to traverse back into the unreachable region. Doing so allows e.g. finding a trivial self loop when walking back the predecessor chain.

---

Full diff: https://github.com/llvm/llvm-project/pull/158153.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+9)
• (modified) llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info-with-multiple-predecessors.ll (+31)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 34e497d9ea3cb..8c48ffbf1a049 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -15445,6 +15445,12 @@ void ScalarEvolution::LoopGuards::collectFromPHI(
     const BasicBlock *InBlock = Phi.getIncomingBlock(IncomingIdx);
     if (!VisitedBlocks.insert(InBlock).second)
       return {nullptr, scCouldNotCompute};
+
+    // Avoid analyzing unreachable blocks so that we don't get trapped
+    // traversing cycles with ill-formed dominance or infinite cycles
+    if (!SE.DT.isReachableFromEntry(InBlock))
+      return {nullptr, scCouldNotCompute};
+
     auto [G, Inserted] = IncomingGuards.try_emplace(InBlock, LoopGuards(SE));
     if (Inserted)
       collectFromBlock(SE, G->second, Phi.getParent(), InBlock, VisitedBlocks,
@@ -15499,6 +15505,9 @@ void ScalarEvolution::LoopGuards::collectFromBlock(
     ScalarEvolution &SE, ScalarEvolution::LoopGuards &Guards,
     const BasicBlock *Block, const BasicBlock *Pred,
     SmallPtrSetImpl<const BasicBlock *> &VisitedBlocks, unsigned Depth) {
+
+  assert(SE.DT.isReachableFromEntry(Block) && SE.DT.isReachableFromEntry(Pred));
+
   SmallVector<const SCEV *> ExprsToRewrite;
   auto CollectCondition = [&](ICmpInst::Predicate Predicate, const SCEV *LHS,
                               const SCEV *RHS,
diff --git a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info-with-multiple-predecessors.ll b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info-with-multiple-predecessors.ll
index 28035b05303db..13762e3fa0474 100644
--- a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info-with-multiple-predecessors.ll
+++ b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info-with-multiple-predecessors.ll
@@ -364,3 +364,34 @@ body:
 exit:
   ret void
 }
+
+define void @hang_due_to_unreachable_phi_inblock() personality ptr null {
+  br label %1
+
+1:                                                ; preds = %1, %0
+  %2 = invoke ptr null(i64 0)
+          to label %1 unwind label %3
+
+3:                                                ; preds = %1
+  %4 = landingpad { ptr, i32 }
+          cleanup
+  br label %7
+
+5:                                                ; No predecessors!
+  %6 = landingpad { ptr, i32 }
+          cleanup
+  br label %7
+
+7:                                                ; preds = %5, %3
+  %8 = phi ptr [ null, %5 ], [ null, %3 ]
+  br i1 false, label %13, label %9
+
+9:                                                ; preds = %9, %7
+  %10 = phi ptr [ %11, %9 ], [ null, %7 ]
+  %11 = getelementptr i8, ptr %10, i64 24
+  %12 = icmp eq ptr %10, null
+  br i1 %12, label %13, label %9
+
+13:                                               ; preds = %9, %7
+  resume { ptr, i32 } zeroinitializer
+}

[nikic]

LGTM

[nikic]

Would be nice to replace this invoke + landingpad with simple branches (which I think should work?)

[preames]

I tried that, and it no longer hung on old opt. I might have gone too simple (I used a poison condition), but I suspect we might special case that idiom somewhere. I'll give it one more go before landing to be sure.

[preames]

Yeah, trying multiple variants, up to an including volatile stores, doesn't seem to trip the hang any more. Not sure why we need the invoke, but we appear to.

[preames]

Oh, I figured out the difference.

for (; Pair.first;
       Pair = SE.getPredecessorWithUniqueSuccessorForBB(Pair.first)) {
    VisitedBlocks.insert(Pair.second);
    const BranchInst *LoopEntryPredicate =
        dyn_cast<BranchInst>(Pair.first->getTerminator());
    if (!LoopEntryPredicate || LoopEntryPredicate->isUnconditional())
      continue;

    Terms.emplace_back(LoopEntryPredicate->getCondition(),
                       LoopEntryPredicate->getSuccessor(0) == Pair.second);
    NumCollectedConditions++;

    // If we are recursively collecting guards stop after 2
    // conditions to limit compile-time impact for now.
    if (Depth > 0 && NumCollectedConditions == 2)
      break;
  }

This the loop which is actually infintte. The Depth on this example is 1. As a result, if the cycle we're looping on contains a conditional branch, we hit the NumCollectedConditions guard. Per the comment, it wasn't meant to be functional, but that explains why we hit this so rarely.