[NewGVN] Break infinite recursion in singleReachablePHIPath().

We can have cycles between PHIs and this causes singleReachablePhi()
to call itself indefintely (until we run out of stack). The proper
solution would be that of computing SCCs, but it's not worth for
now, so just keep a visited set and give up when we find a cycle.
Thanks to Dan for the discussion/help with this.

Fixes PR33014.

llvm-svn: 303393

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 34b93a8..0b6f750 100644 (file)
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -643,7 +643,8 @@ private:
   void verifyMemoryCongruency() const;
   void verifyIterationSettled(Function &F);
   void verifyStoreExpressions() const;
-  bool singleReachablePHIPath(const MemoryAccess *, const MemoryAccess *) const;
+  bool singleReachablePHIPath(SmallPtrSet<const MemoryAccess *, 8> &,
+                              const MemoryAccess *, const MemoryAccess *) const;
   BasicBlock *getBlockForValue(Value *V) const;
   void deleteExpression(const Expression *E) const;
   unsigned InstrToDFSNum(const Value *V) const {
@@ -2460,13 +2461,23 @@ void NewGVN::valueNumberInstruction(Instruction *I) {
 
 // Check if there is a path, using single or equal argument phi nodes, from
 // First to Second.
-bool NewGVN::singleReachablePHIPath(const MemoryAccess *First,
-                                    const MemoryAccess *Second) const {
+bool NewGVN::singleReachablePHIPath(
+    SmallPtrSet<const MemoryAccess *, 8> &Visited, const MemoryAccess *First,
+    const MemoryAccess *Second) const {
   if (First == Second)
     return true;
   if (MSSA->isLiveOnEntryDef(First))
     return false;
 
+  // This is not perfect, but as we're just verifying here, we can live with
+  // the loss of precision. The real solution would be that of doing strongly
+  // connected component finding in this routine, and it's probably not worth
+  // the complexity for the time being. So, we just keep a set of visited
+  // MemoryAccess and return true when we hit a cycle.
+  if (Visited.count(First))
+    return true;
+  Visited.insert(First);
+
   const auto *EndDef = First;
   for (auto *ChainDef : optimized_def_chain(First)) {
     if (ChainDef == Second)
@@ -2489,7 +2500,8 @@ bool NewGVN::singleReachablePHIPath(const MemoryAccess *First,
     Okay =
         std::equal(OperandList.begin(), OperandList.end(), OperandList.begin());
   if (Okay)
-    return singleReachablePHIPath(cast<MemoryAccess>(OperandList[0]), Second);
+    return singleReachablePHIPath(Visited, cast<MemoryAccess>(OperandList[0]),
+                                  Second);
   return false;
 }
 
@@ -2556,13 +2568,15 @@ void NewGVN::verifyMemoryCongruency() const {
            "Memory not unreachable but ended up in TOP");
     if (auto *FirstMUD = dyn_cast<MemoryUseOrDef>(KV.first)) {
       auto *SecondMUD = dyn_cast<MemoryUseOrDef>(KV.second->getMemoryLeader());
-      if (FirstMUD && SecondMUD)
-        assert((singleReachablePHIPath(FirstMUD, SecondMUD) ||
+      if (FirstMUD && SecondMUD) {
+        SmallPtrSet<const MemoryAccess *, 8> VisitedMAS;
+        assert((singleReachablePHIPath(VisitedMAS, FirstMUD, SecondMUD) ||
                 ValueToClass.lookup(FirstMUD->getMemoryInst()) ==
                     ValueToClass.lookup(SecondMUD->getMemoryInst())) &&
                "The instructions for these memory operations should have "
                "been in the same congruence class or reachable through"
                "a single argument phi");
+      }
     } else if (auto *FirstMP = dyn_cast<MemoryPhi>(KV.first)) {
       // We can only sanely verify that MemoryDefs in the operand list all have
       // the same class.

diff --git a/llvm/test/Transforms/NewGVN/pr33014.ll b/llvm/test/Transforms/NewGVN/pr33014.ll
new file mode 100644 (file)
index 0000000..4157178
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr33014.ll
@@ -0,0 +1,54 @@
+; Make sure we don't end up in an infinite recursion in singleReachablePHIPath().
+; REQUIRES: asserts
+; RUN: opt -newgvn -S %s | FileCheck %s
+
+@c = external global i64, align 8
+
+; CHECK-LABEL: define void @tinkywinky() {
+; CHECK: entry:
+; CHECK-NEXT:   br i1 undef, label %l2, label %if.then
+; CHECK: if.then:                                          ; preds = %entry
+; CHECK-NEXT:   br label %for.body
+; CHECK: ph:                                               ; preds = %back, %ontrue
+; CHECK-NEXT:   br label %for.body
+; CHECK: for.body:                                         ; preds = %ph, %if.then
+; CHECK-NEXT:   br i1 undef, label %ontrue, label %onfalse
+; CHECK: onfalse:                                          ; preds = %for.body
+; CHECK-NEXT:   %patatino = load i64, i64* @c
+; CHECK-NEXT:   ret void
+; CHECK: ontrue:                                           ; preds = %for.body
+; CHECK-NEXT:   %dipsy = load i64, i64* @c
+; CHECK-NEXT:   br label %ph
+; CHECK: back:                                             ; preds = %l2
+; CHECK-NEXT:   store i8 undef, i8* null
+; CHECK-NEXT:   br label %ph
+; CHECK: end:                                              ; preds = %l2
+; CHECK-NEXT:   ret void
+; CHECK: l2:                                               ; preds = %entry
+; CHECK-NEXT:   br i1 false, label %back, label %end
+; CHECK-NEXT: }
+
+define void @tinkywinky() {
+entry:
+  br i1 undef, label %l2, label %if.then
+if.then:
+  br label %for.body
+ph:
+  br label %for.body
+for.body:
+  br i1 undef, label %ontrue, label %onfalse
+onfalse:
+  %patatino = load i64, i64* @c
+  store i64 %patatino, i64* @c
+  ret void
+ontrue:
+  %dipsy = load i64, i64* @c
+  store i64 %dipsy, i64* @c
+  br label %ph
+back:
+  br label %ph
+end:
+  ret void
+l2:
+  br i1 false, label %back, label %end
+}