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?

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@303453 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Transforms/Scalar/GVNExpression.h b/include/llvm/Transforms/Scalar/GVNExpression.h
index 2d4f4d9bef891e1e15228091188e98d80da787ad..a971df975b6fa7a21f7cccb30dd458faadbf27ed 100644 (file)
--- a/include/llvm/Transforms/Scalar/GVNExpression.h
+++ b/include/llvm/Transforms/Scalar/GVNExpression.h
@@ -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;

diff --git a/lib/Transforms/Scalar/NewGVN.cpp b/lib/Transforms/Scalar/NewGVN.cpp
index 133049004ab8ffe42aeeba241b73c0e6bb76850a..9059fc73a10b3d82add4cb34a23d8ae570216c06 100644 (file)
--- a/lib/Transforms/Scalar/NewGVN.cpp
+++ b/lib/Transforms/Scalar/NewGVN.cpp
@@ -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::createPHIExpression(Instruction *I, bool &HasBackedge,
                    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(Instruction *I) const {
   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(const Instruction *Inst,
 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::performSymbolicPHIEvaluation(Instruction *I) const {
   // 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::performSymbolicPHIEvaluation(Instruction *I) const {
   // 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::performSymbolicPHIEvaluation(Instruction *I) const {
     }
     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::performSymbolicPHIEvaluation(Instruction *I) const {
       // 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 HashedExpression &HE) {
 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(Instruction *I, const Expression *E) {
   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(Function &F) {
       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;
     }
 

diff --git a/test/Transforms/NewGVN/pr32838.ll b/test/Transforms/NewGVN/pr32838.ll
new file mode 100644 (file)
index 0000000..b6b7b0d
--- /dev/null
+++ b/test/Transforms/NewGVN/pr32838.ll
@@ -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
+}
+@b = external global i32, align 4
+@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
+}
+