From ed2b8bd9f467135943cebba2fd27e1960bc2c569 Mon Sep 17 00:00:00 2001 From: Davide Italiano Date: Thu, 18 May 2017 23:22:44 +0000 Subject: [PATCH] [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. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@303393 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Scalar/NewGVN.cpp | 26 +++++++++++---- test/Transforms/NewGVN/pr33014.ll | 54 +++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 test/Transforms/NewGVN/pr33014.ll diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp index 34b93a88718..0b6f75093aa 100644 --- a/lib/Transforms/Scalar/NewGVN.cpp +++ b/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 *, 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 &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(OperandList[0]), Second); + return singleReachablePHIPath(Visited, cast(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(KV.first)) { auto *SecondMUD = dyn_cast(KV.second->getMemoryLeader()); - if (FirstMUD && SecondMUD) - assert((singleReachablePHIPath(FirstMUD, SecondMUD) || + if (FirstMUD && SecondMUD) { + SmallPtrSet 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(KV.first)) { // We can only sanely verify that MemoryDefs in the operand list all have // the same class. diff --git a/test/Transforms/NewGVN/pr33014.ll b/test/Transforms/NewGVN/pr33014.ll new file mode 100644 index 00000000000..4157178e4f0 --- /dev/null +++ b/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 +}