[llvm] r305657 - NewGVN: Fix PR 33461, caused by slightly overzealous verification.
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Sun Jun 18 17:24:00 PDT 2017
Author: dannyb
Date: Sun Jun 18 19:24:00 2017
New Revision: 305657
URL: http://llvm.org/viewvc/llvm-project?rev=305657&view=rev
Log:
NewGVN: Fix PR 33461, caused by slightly overzealous verification.
Added:
llvm/trunk/test/Transforms/NewGVN/pr33461.ll
Modified:
llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=305657&r1=305656&r2=305657&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Sun Jun 18 19:24:00 2017
@@ -1244,27 +1244,24 @@ const Expression *NewGVN::performSymboli
// 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<LoadInst>(lookupOperandLeader(SI->getValueOperand()))) {
+ if (auto *LI = dyn_cast<LoadInst>(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(Func
// a no-longer valid StoreExpression.
void NewGVN::verifyStoreExpressions() const {
#ifndef NDEBUG
- DenseSet<std::pair<const Value *, const Value *>> 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<const Value *,
+ std::tuple<const Value *, const CongruenceClass *, Value *>>>
+ StoreExpressionSet;
for (const auto &KV : ExpressionToClass) {
if (auto *SE = dyn_cast<StoreExpression>(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 "
Added: llvm/trunk/test/Transforms/NewGVN/pr33461.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/pr33461.ll?rev=305657&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/pr33461.ll (added)
+++ llvm/trunk/test/Transforms/NewGVN/pr33461.ll Sun Jun 18 19:24:00 2017
@@ -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
+ at 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
+}
More information about the llvm-commits
mailing list