[llvm] r303453 - NewGVN: Fix PR32838.
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Fri May 19 13:22:20 PDT 2017
Author: dannyb
Date: Fri May 19 15:22:20 2017
New Revision: 303453
URL: http://llvm.org/viewvc/llvm-project?rev=303453&view=rev
Log:
NewGVN: Fix PR32838.
This is a complicated bug involving two issues:
1. What do we do with phi nodes when we prove all arguments are not
live?
2. When is it safe to use value leaders to determine if we can ignore
an argumnet?
Added:
llvm/trunk/test/Transforms/NewGVN/pr32838.ll
Modified:
llvm/trunk/include/llvm/Transforms/Scalar/GVNExpression.h
llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
Modified: llvm/trunk/include/llvm/Transforms/Scalar/GVNExpression.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Scalar/GVNExpression.h?rev=303453&r1=303452&r2=303453&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Scalar/GVNExpression.h (original)
+++ llvm/trunk/include/llvm/Transforms/Scalar/GVNExpression.h Fri May 19 15:22:20 2017
@@ -40,6 +40,7 @@ enum ExpressionType {
ET_Base,
ET_Constant,
ET_Variable,
+ ET_Dead,
ET_Unknown,
ET_BasicStart,
ET_Basic,
@@ -515,6 +516,17 @@ public:
}
};
+class DeadExpression final : public Expression {
+public:
+ DeadExpression() : Expression(ET_Dead) {}
+ DeadExpression(const DeadExpression &) = delete;
+ DeadExpression &operator=(const DeadExpression &) = delete;
+
+ static bool classof(const Expression *E) {
+ return E->getExpressionType() == ET_Dead;
+ }
+};
+
class VariableExpression final : public Expression {
private:
Value *VariableValue;
Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=303453&r1=303452&r2=303453&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Fri May 19 15:22:20 2017
@@ -529,6 +529,13 @@ class NewGVN {
using ExpressionClassMap = DenseMap<const Expression *, CongruenceClass *>;
ExpressionClassMap ExpressionToClass;
+ // We have a single expression that represents currently DeadExpressions.
+ // For dead expressions we can prove will stay dead, we mark them with
+ // DFS number zero. However, it's possible in the case of phi nodes
+ // for us to assume/prove all arguments are dead during fixpointing.
+ // We use DeadExpression for that case.
+ DeadExpression *SingletonDeadExpression = nullptr;
+
// Which values have changed as a result of leader changes.
SmallPtrSet<Value *, 8> LeaderChanges;
@@ -583,6 +590,7 @@ private:
Value *) const;
PHIExpression *createPHIExpression(Instruction *, bool &HasBackEdge,
bool &OriginalOpsConstant) const;
+ const DeadExpression *createDeadExpression() const;
const VariableExpression *createVariableExpression(Value *) const;
const ConstantExpression *createConstantExpression(Constant *) const;
const Expression *createVariableOrConstant(Value *V) const;
@@ -855,10 +863,14 @@ PHIExpression *NewGVN::createPHIExpressi
HasBackedge = HasBackedge || isBackedge(BB, PHIBlock);
OriginalOpsConstant =
OriginalOpsConstant && isa<Constant>(*U);
-
- // Don't try to transform self-defined phis.
+ // Use nullptr to distinguish between things that were
+ // originally self-defined and those that have an operand
+ // leader that is self-defined.
if (*U == PN)
- return PN;
+ return nullptr;
+ // Things in TOPClass are equivalent to everything.
+ if (ValueToClass.lookup(*U) == TOPClass)
+ return nullptr;
return lookupOperandLeader(*U);
});
return E;
@@ -1055,6 +1067,12 @@ NewGVN::createAggregateValueExpression(I
llvm_unreachable("Unhandled type of aggregate value operation");
}
+const DeadExpression *NewGVN::createDeadExpression() const {
+ // DeadExpression has no arguments and all DeadExpression's are the same,
+ // so we only need one of them.
+ return SingletonDeadExpression;
+}
+
const VariableExpression *NewGVN::createVariableExpression(Value *V) const {
auto *E = new (ExpressionAllocator) VariableExpression(V);
E->setOpcode(V->getValueID());
@@ -1126,7 +1144,7 @@ bool NewGVN::someEquivalentDominates(con
Value *NewGVN::lookupOperandLeader(Value *V) const {
CongruenceClass *CC = ValueToClass.lookup(V);
if (CC) {
- // Everything in TOP is represneted by undef, as it can be any value.
+ // Everything in TOP is represented by undef, as it can be any value.
// We do have to make sure we get the type right though, so we can't set the
// RepLeader to undef.
if (CC == TOPClass)
@@ -1564,10 +1582,9 @@ const Expression *NewGVN::performSymboli
// True if one of the incoming phi edges is a backedge.
bool HasBackedge = false;
// All constant tracks the state of whether all the *original* phi operands
- // were constant. This is really shorthand for "this phi cannot cycle due
- // to forward propagation", as any change in value of the phi is guaranteed
- // not to later change the value of the phi.
- // IE it can't be v = phi(undef, v+1)
+ // This is really shorthand for "this phi cannot cycle due to forward
+ // change in value of the phi is guaranteed not to later change the value of
+ // the phi. IE it can't be v = phi(undef, v+1)
bool AllConstant = true;
auto *E =
cast<PHIExpression>(createPHIExpression(I, HasBackedge, AllConstant));
@@ -1575,8 +1592,16 @@ const Expression *NewGVN::performSymboli
// See if all arguments are the same.
// We track if any were undef because they need special handling.
bool HasUndef = false;
- auto Filtered = make_filter_range(E->operands(), [&](const Value *Arg) {
- if (Arg == I)
+ bool CycleFree = isCycleFree(cast<PHINode>(I));
+ auto Filtered = make_filter_range(E->operands(), [&](Value *Arg) {
+ if (Arg == nullptr)
+ return false;
+ // Original self-operands are already eliminated during expression creation.
+ // We can only eliminate value-wise self-operands if it's cycle
+ // free. Otherwise, eliminating the operand can cause our value to change,
+ // which can cause us to not eliminate the operand, which changes the value
+ // back to what it was before, cycling forever.
+ if (CycleFree && Arg == I)
return false;
if (isa<UndefValue>(Arg)) {
HasUndef = true;
@@ -1584,20 +1609,19 @@ const Expression *NewGVN::performSymboli
}
return true;
});
- // If we are left with no operands, it's undef
+ // If we are left with no operands, it's dead.
if (Filtered.begin() == Filtered.end()) {
- DEBUG(dbgs() << "Simplified PHI node " << *I << " to undef"
- << "\n");
+ DEBUG(dbgs() << "No arguments of PHI node " << *I << " are live\n");
deleteExpression(E);
- return createConstantExpression(UndefValue::get(I->getType()));
+ return createDeadExpression();
}
unsigned NumOps = 0;
Value *AllSameValue = *(Filtered.begin());
++Filtered.begin();
// Can't use std::equal here, sadly, because filter.begin moves.
- if (llvm::all_of(Filtered, [AllSameValue, &NumOps](const Value *V) {
+ if (llvm::all_of(Filtered, [&](Value *Arg) {
++NumOps;
- return V == AllSameValue;
+ return Arg == AllSameValue;
})) {
// In LLVM's non-standard representation of phi nodes, it's possible to have
// phi nodes with cycles (IE dependent on other phis that are .... dependent
@@ -1616,7 +1640,7 @@ const Expression *NewGVN::performSymboli
// constants, or all operands are ignored but the undef, it also must be
// cycle free.
if (!AllConstant && HasBackedge && NumOps > 0 &&
- !isa<UndefValue>(AllSameValue) && !isCycleFree(I))
+ !isa<UndefValue>(AllSameValue) && !CycleFree)
return E;
// Only have to check for instructions
@@ -2150,7 +2174,8 @@ void NewGVN::markPhiOfOpsChanged(const H
void NewGVN::performCongruenceFinding(Instruction *I, const Expression *E) {
// This is guaranteed to return something, since it will at least find
// TOP.
- CongruenceClass *IClass = ValueToClass[I];
+
+ CongruenceClass *IClass = ValueToClass.lookup(I);
assert(IClass && "Should have found a IClass");
// Dead classes should have been eliminated from the mapping.
assert(!IClass->isDead() && "Found a dead class");
@@ -2159,7 +2184,10 @@ void NewGVN::performCongruenceFinding(In
HashedExpression HE(E);
if (const auto *VE = dyn_cast<VariableExpression>(E)) {
EClass = ValueToClass.lookup(VE->getVariableValue());
- } else {
+ } else if (isa<DeadExpression>(E)) {
+ EClass = TOPClass;
+ }
+ if (!EClass) {
auto lookupResult = ExpressionToClass.insert_as({E, nullptr}, HE);
// If it's not in the value table, create a new congruence class.
@@ -3053,6 +3081,7 @@ bool NewGVN::runGVN() {
bool Changed = false;
NumFuncArgs = F.arg_size();
MSSAWalker = MSSA->getWalker();
+ SingletonDeadExpression = new (ExpressionAllocator) DeadExpression();
// Count number of instructions for sizing of hash tables, and come
// up with a global dfs numbering for instructions.
@@ -3537,13 +3566,15 @@ bool NewGVN::eliminateInstructions(Funct
continue;
// Everything still in the TOP class is unreachable or dead.
if (CC == TOPClass) {
-#ifndef NDEBUG
- for (auto M : *CC)
+ for (auto M : *CC) {
+ auto *VTE = ValueToExpression.lookup(M);
+ if (VTE && isa<DeadExpression>(VTE))
+ markInstructionForDeletion(cast<Instruction>(M));
assert((!ReachableBlocks.count(cast<Instruction>(M)->getParent()) ||
InstructionsToErase.count(cast<Instruction>(M))) &&
"Everything in TOP should be unreachable or dead at this "
"point");
-#endif
+ }
continue;
}
Added: llvm/trunk/test/Transforms/NewGVN/pr32838.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/pr32838.ll?rev=303453&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/pr32838.ll (added)
+++ llvm/trunk/test/Transforms/NewGVN/pr32838.ll Fri May 19 15:22:20 2017
@@ -0,0 +1,157 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+;RUN: opt -newgvn -S < %s | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.12.0"
+;; Ensure we don't infinite loop when all phi arguments are really unreachable or self-defined
+define void @fn1(i64 %arg) {
+; CHECK-LABEL: @fn1(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br i1 undef, label [[IF_THEN:%.*]], label [[COND_TRUE:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: br i1 false, label [[FIRSTPHIBLOCK:%.*]], label [[TEMP:%.*]]
+; CHECK: firstphiblock:
+; CHECK-NEXT: br i1 undef, label %for.cond17thread-pre-split, label [[SECONDPHIBLOCK:%.*]]
+; CHECK: secondphiblock:
+; CHECK-NEXT: [[SECONDPHI:%.*]] = phi i64 [ [[THIRDPHI:%.*]], [[THIRDPHIBLOCK:%.*]] ], [ undef, [[FIRSTPHIBLOCK]] ]
+; CHECK-NEXT: br i1 undef, label [[FIRSTPHIBLOCK]], label [[THIRDPHIBLOCK]]
+; CHECK: thirdphiblock:
+; CHECK-NEXT: [[THIRDPHI]] = phi i64 [ [[SECONDPHI]], [[SECONDPHIBLOCK]] ], [ [[DIV:%.*]], [[COND_TRUE]] ]
+; CHECK-NEXT: br label [[SECONDPHIBLOCK]]
+; CHECK: for.cond17thread-pre-split:
+; CHECK-NEXT: br label [[COND_TRUE]]
+; CHECK: cond.true:
+; CHECK-NEXT: [[DIV]] = sdiv i64 [[ARG:%.*]], 4
+; CHECK-NEXT: br label [[THIRDPHIBLOCK]]
+; CHECK: temp:
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 undef, label %if.then, label %cond.true
+if.then:
+ br i1 false, label %firstphiblock, label %temp
+firstphiblock:
+ %firstphi = phi i64 [ %arg, %if.then ], [ undef, %secondphiblock ]
+ br i1 undef, label %for.cond17thread-pre-split, label %secondphiblock
+secondphiblock:
+ %secondphi = phi i64 [ %thirdphi, %thirdphiblock ], [ %firstphi, %firstphiblock ]
+ br i1 undef, label %firstphiblock, label %thirdphiblock
+thirdphiblock:
+ %thirdphi = phi i64 [ %secondphi, %secondphiblock ], [ %div, %cond.true ]
+ br label %secondphiblock
+for.cond17thread-pre-split:
+ br label %cond.true
+cond.true:
+ %fourthphi = phi i64 [ %arg, %entry ], [ %firstphi, %for.cond17thread-pre-split ]
+ %div = sdiv i64 %fourthphi, 4
+ br label %thirdphiblock
+temp:
+ ret void
+}
+define void @fn2(i64 %arg) {
+; CHECK-LABEL: @fn2(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br i1 undef, label [[IF_THEN:%.*]], label [[COND_TRUE:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: br i1 false, label [[FIRSTPHIBLOCK:%.*]], label [[TEMP:%.*]]
+; CHECK: firstphiblock:
+; CHECK-NEXT: [[FIRSTPHI:%.*]] = phi i64 [ undef, [[IF_THEN]] ], [ [[SECONDPHI:%.*]], [[SECONDPHIBLOCK:%.*]] ]
+; CHECK-NEXT: br i1 undef, label %for.cond17thread-pre-split, label [[SECONDPHIBLOCK]]
+; CHECK: secondphiblock:
+; CHECK-NEXT: [[SECONDPHI]] = phi i64 [ [[THIRDPHI:%.*]], [[THIRDPHIBLOCK:%.*]] ], [ [[FIRSTPHI]], [[FIRSTPHIBLOCK]] ]
+; CHECK-NEXT: br i1 undef, label [[FIRSTPHIBLOCK]], label [[THIRDPHIBLOCK]]
+; CHECK: thirdphiblock:
+; CHECK-NEXT: [[THIRDPHI]] = phi i64 [ [[SECONDPHI]], [[SECONDPHIBLOCK]] ], [ [[DIV:%.*]], [[COND_TRUE]] ]
+; CHECK-NEXT: br label [[SECONDPHIBLOCK]]
+; CHECK: for.cond17thread-pre-split:
+; CHECK-NEXT: br label [[COND_TRUE]]
+; CHECK: cond.true:
+; CHECK-NEXT: [[FOURTHPHI:%.*]] = phi i64 [ [[ARG:%.*]], [[ENTRY:%.*]] ], [ [[FIRSTPHI]], %for.cond17thread-pre-split ]
+; CHECK-NEXT: [[DIV]] = sdiv i64 [[FOURTHPHI]], 4
+; CHECK-NEXT: br label [[THIRDPHIBLOCK]]
+; CHECK: temp:
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 undef, label %if.then, label %cond.true
+if.then:
+ br i1 false, label %firstphiblock, label %temp
+firstphiblock:
+ %firstphi = phi i64 [ %arg, %if.then ], [ %secondphi, %secondphiblock ]
+ br i1 undef, label %for.cond17thread-pre-split, label %secondphiblock
+secondphiblock:
+ %secondphi = phi i64 [ %thirdphi, %thirdphiblock ], [ %firstphi, %firstphiblock ]
+ br i1 undef, label %firstphiblock, label %thirdphiblock
+thirdphiblock:
+ %thirdphi = phi i64 [ %secondphi, %secondphiblock ], [ %div, %cond.true ]
+ br label %secondphiblock
+for.cond17thread-pre-split:
+ br label %cond.true
+cond.true:
+ %fourthphi = phi i64 [ %arg, %entry ], [ %firstphi, %for.cond17thread-pre-split ]
+ %div = sdiv i64 %fourthphi, 4
+ br label %thirdphiblock
+temp:
+ ret void
+}
+ at b = external global i32, align 4
+ at a = external global i32, align 4
+define void @fn3() {
+; CHECK-LABEL: @fn3(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[L1:%.*]]
+; CHECK: l1.loopexit:
+; CHECK-NEXT: br label [[L1]]
+; CHECK: l1:
+; CHECK-NEXT: [[F_0:%.*]] = phi i32* [ @b, [[ENTRY:%.*]] ], [ @a, [[L1_LOOPEXIT:%.*]] ]
+; CHECK-NEXT: br label [[FOR_COND:%.*]]
+; CHECK: for.cond.loopexit:
+; CHECK-NEXT: store i8 undef, i8* null
+; CHECK-NEXT: br label [[FOR_COND]]
+; CHECK: for.cond:
+; CHECK-NEXT: br i1 undef, label [[FOR_END14:%.*]], label [[FOR_COND1_PREHEADER:%.*]]
+; CHECK: for.cond1.preheader:
+; CHECK-NEXT: br label [[FOR_BODY3:%.*]]
+; CHECK: for.cond1:
+; CHECK-NEXT: br label [[L2:%.*]]
+; CHECK: for.body3:
+; CHECK-NEXT: br i1 undef, label [[FOR_COND1:%.*]], label [[L1_LOOPEXIT]]
+; CHECK: l2:
+; CHECK-NEXT: [[G_4:%.*]] = phi i32* [ @b, [[FOR_END14]] ], [ @a, [[FOR_COND1]] ]
+; CHECK-NEXT: [[F_2:%.*]] = phi i32* [ [[F_0]], [[FOR_END14]] ], [ @a, [[FOR_COND1]] ]
+; CHECK-NEXT: br label [[FOR_INC:%.*]]
+; CHECK: for.inc:
+; CHECK-NEXT: br i1 false, label [[FOR_COND_LOOPEXIT:%.*]], label [[FOR_INC]]
+; CHECK: for.end14:
+; CHECK-NEXT: br label [[L2]]
+;
+entry:
+ br label %l1
+l1.loopexit:
+ %g.223.lcssa = phi i32* [ @b, %for.body3 ]
+ br label %l1
+l1:
+ %g.0 = phi i32* [ undef, %entry ], [ %g.223.lcssa, %l1.loopexit ]
+ %f.0 = phi i32* [ @b, %entry ], [ @a, %l1.loopexit ]
+ br label %for.cond
+for.cond.loopexit:
+ br label %for.cond
+for.cond:
+ %g.1 = phi i32* [ %g.0, %l1 ], [ %g.4, %for.cond.loopexit ]
+ %f.1 = phi i32* [ %f.0, %l1 ], [ %f.2, %for.cond.loopexit ]
+ br i1 undef, label %for.end14, label %for.cond1.preheader
+for.cond1.preheader:
+ br label %for.body3
+for.cond1:
+ br label %l2
+for.body3:
+ br i1 undef, label %for.cond1, label %l1.loopexit
+l2:
+ %g.4 = phi i32* [ %g.1, %for.end14 ], [ @a, %for.cond1 ]
+ %f.2 = phi i32* [ %f.1, %for.end14 ], [ @a, %for.cond1 ]
+ br label %for.inc
+for.inc:
+ br i1 false, label %for.cond.loopexit, label %for.inc
+for.end14:
+ br label %l2
+}
+
More information about the llvm-commits
mailing list