From 47c282c3dffc60b553aa8be6b075651b025311a4 Mon Sep 17 00:00:00 2001 From: Daniel Berlin Date: Mon, 19 Jun 2017 00:24:00 +0000 Subject: [PATCH] NewGVN: Fix PR 33461, caused by slightly overzealous verification. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@305657 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Scalar/NewGVN.cpp | 48 ++++++++++++++++++++++++--------------- test/Transforms/NewGVN/pr33461.ll | 36 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 test/Transforms/NewGVN/pr33461.ll diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp index cbbd55512c9..c3a593e9267 100644 --- a/lib/Transforms/Scalar/NewGVN.cpp +++ b/lib/Transforms/Scalar/NewGVN.cpp @@ -1244,27 +1244,24 @@ const Expression *NewGVN::performSymbolicStoreEvaluation(Instruction *I) const { // only do this for simple stores, we should expand to cover memcpys, etc. const auto *LastStore = createStoreExpression(SI, StoreRHS); const auto *LastCC = ExpressionToClass.lookup(LastStore); - // Basically, check if the congruence class the store is in is defined by a - // store that isn't us, and has the same value. MemorySSA takes care of - // ensuring the store has the same memory state as us already. - // The RepStoredValue gets nulled if all the stores disappear in a class, so - // we don't need to check if the class contains a store besides us. - if (LastCC && - LastCC->getStoredValue() == lookupOperandLeader(SI->getValueOperand())) + // We really want to check whether the expression we matched was a store. No + // easy way to do that. However, we can check that the class we found has a + // store, which, assuming the value numbering state is not corrupt, is + // sufficient, because we must also be equivalent to that store's expression + // for it to be in the same class as the load. + if (LastCC && LastCC->getStoredValue() == LastStore->getStoredValue()) return LastStore; - deleteExpression(LastStore); // Also check if our value operand is defined by a load of the same memory // location, and the memory state is the same as it was then (otherwise, it // could have been overwritten later. See test32 in // transforms/DeadStoreElimination/simple.ll). - if (auto *LI = - dyn_cast(lookupOperandLeader(SI->getValueOperand()))) { + if (auto *LI = dyn_cast(LastStore->getStoredValue())) if ((lookupOperandLeader(LI->getPointerOperand()) == - lookupOperandLeader(SI->getPointerOperand())) && + LastStore->getOperand(0)) && (lookupMemoryLeader(getMemoryAccess(LI)->getDefiningAccess()) == StoreRHS)) - return createStoreExpression(SI, StoreRHS); - } + return LastStore; + deleteExpression(LastStore); } // If the store is not equivalent to anything, value number it as a store that @@ -3014,14 +3011,29 @@ void NewGVN::verifyIterationSettled(Function &F) { // a no-longer valid StoreExpression. void NewGVN::verifyStoreExpressions() const { #ifndef NDEBUG - DenseSet> StoreExpressionSet; + // This is the only use of this, and it's not worth defining a complicated + // densemapinfo hash/equality function for it. + std::set< + std::pair>> + StoreExpressionSet; for (const auto &KV : ExpressionToClass) { if (auto *SE = dyn_cast(KV.first)) { // Make sure a version that will conflict with loads is not already there - auto Res = - StoreExpressionSet.insert({SE->getOperand(0), SE->getMemoryLeader()}); - assert(Res.second && - "Stored expression conflict exists in expression table"); + auto Res = StoreExpressionSet.insert( + {SE->getOperand(0), std::make_tuple(SE->getMemoryLeader(), KV.second, + SE->getStoredValue())}); + bool Okay = Res.second; + // It's okay to have the same expression already in there if it is + // identical in nature. + // This can happen when the leader of the stored value changes over time. + if (!Okay) { + Okay = Okay && std::get<1>(Res.first->second) == KV.second; + Okay = Okay && + lookupOperandLeader(std::get<2>(Res.first->second)) == + lookupOperandLeader(SE->getStoredValue()); + } + assert(Okay && "Stored expression conflict exists in expression table"); auto *ValueExpr = ValueToExpression.lookup(SE->getStoreInst()); assert(ValueExpr && ValueExpr->equals(*SE) && "StoreExpression in ExpressionToClass is not latest " diff --git a/test/Transforms/NewGVN/pr33461.ll b/test/Transforms/NewGVN/pr33461.ll new file mode 100644 index 00000000000..0a41d6834a4 --- /dev/null +++ b/test/Transforms/NewGVN/pr33461.ll @@ -0,0 +1,36 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py +;; Ensure the store verifier is not overzealous +; RUN: opt -newgvn -S %s | FileCheck %s +@b = external global i16, align 2 + +define void @patatino() { +; CHECK-LABEL: @patatino( +; CHECK-NEXT: entry: +; CHECK-NEXT: br i1 false, label [[FOR_COND1:%.*]], label [[FOR_INC:%.*]] +; CHECK: for.cond1: +; CHECK-NEXT: [[TMP0:%.*]] = phi i16 [ [[INC:%.*]], [[FOR_INC]] ], [ undef, [[ENTRY:%.*]] ] +; CHECK-NEXT: store i16 [[TMP0]], i16* @b, align 2 +; CHECK-NEXT: br label [[FOR_INC]] +; CHECK: for.inc: +; CHECK-NEXT: [[TMP1:%.*]] = load i16, i16* @b, align 2 +; CHECK-NEXT: [[INC]] = add i16 [[TMP1]], 1 +; CHECK-NEXT: store i16 [[INC]], i16* @b, align 2 +; CHECK-NEXT: br label [[FOR_COND1]] +; +entry: + br i1 false, label %for.cond1, label %for.inc + +for.cond1: + %e.0 = phi i16* [ %e.1, %for.inc ], [ null, %entry ] + %0 = load i16, i16* %e.0, align 2 + %add = add i16 %0, 0 + store i16 %add, i16* %e.0, align 2 + br label %for.inc + +for.inc: + %e.1 = phi i16* [ %e.0, %for.cond1 ], [ @b, %entry ] + %1 = load i16, i16* @b, align 2 + %inc = add i16 %1, 1 + store i16 %inc, i16* @b, align 2 + br label %for.cond1 +} -- 2.11.0