[llvm] r291308 - NewGVN: Fix PR 31501.

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 6 16:01:42 PST 2017


Author: dannyb
Date: Fri Jan  6 18:01:42 2017
New Revision: 291308

URL: http://llvm.org/viewvc/llvm-project?rev=291308&view=rev
Log:
NewGVN: Fix PR 31501.

Summary: LLVM's non-standard notion of phi nodes means we can't both try to substitute for undef in phi nodes *and* use phi nodes as leaders all the time. This changes NewGVN to use the same semantics as SimplifyPHINode to decide which phi nodes are equivalent.

Reviewers: davide

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D28312

Added:
    llvm/trunk/test/Transforms/NewGVN/cyclic-phi-handling.ll
    llvm/trunk/test/Transforms/NewGVN/pr31501.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=291308&r1=291307&r2=291308&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Fri Jan  6 18:01:42 2017
@@ -390,10 +390,10 @@ INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrap
 INITIALIZE_PASS_END(NewGVN, "newgvn", "Global Value Numbering", false, false)
 
 PHIExpression *NewGVN::createPHIExpression(Instruction *I) {
-  BasicBlock *PhiBlock = I->getParent();
+  BasicBlock *PHIBlock = I->getParent();
   auto *PN = cast<PHINode>(I);
-  auto *E = new (ExpressionAllocator)
-      PHIExpression(PN->getNumOperands(), I->getParent());
+  auto *E =
+      new (ExpressionAllocator) PHIExpression(PN->getNumOperands(), PHIBlock);
 
   E->allocateOperands(ArgRecycler, ExpressionAllocator);
   E->setType(I->getType());
@@ -408,10 +408,10 @@ PHIExpression *NewGVN::createPHIExpressi
 
   std::transform(Filtered.begin(), Filtered.end(), op_inserter(E),
                  [&](const Use &U) -> Value * {
-                   // Don't try to transform self-defined phis
+                   // Don't try to transform self-defined phis.
                    if (U == PN)
                      return PN;
-                   const BasicBlockEdge BBE(PN->getIncomingBlock(U), PhiBlock);
+                   const BasicBlockEdge BBE(PN->getIncomingBlock(U), PHIBlock);
                    return lookupOperandLeader(U, I, BBE);
                  });
   return E;
@@ -810,36 +810,50 @@ bool NewGVN::setMemoryAccessEquivTo(Memo
 const Expression *NewGVN::performSymbolicPHIEvaluation(Instruction *I,
                                                        const BasicBlock *B) {
   auto *E = cast<PHIExpression>(createPHIExpression(I));
-  if (E->op_empty()) {
+  // We match the semantics of SimplifyPhiNode from InstructionSimplify here.
+
+  // See if all arguaments 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)
+      return false;
+    if (isa<UndefValue>(Arg)) {
+      HasUndef = true;
+      return false;
+    }
+    return true;
+  });
+  // If we are left with no operands, it's undef
+  if (Filtered.begin() == Filtered.end()) {
     DEBUG(dbgs() << "Simplified PHI node " << *I << " to undef"
                  << "\n");
     E->deallocateOperands(ArgRecycler);
     ExpressionAllocator.Deallocate(E);
     return createConstantExpression(UndefValue::get(I->getType()));
   }
-
-  Value *AllSameValue = E->getOperand(0);
-
-  // See if all arguments are the same, ignoring undef arguments, because we can
-  // choose a value that is the same for them.
-  for (const Value *Arg : E->operands())
-    if (Arg != AllSameValue && !isa<UndefValue>(Arg)) {
-      AllSameValue = nullptr;
-      break;
+  Value *AllSameValue = *(Filtered.begin());
+  ++Filtered.begin();
+  // Can't use std::equal here, sadly, because filter.begin moves.
+  if (llvm::all_of(Filtered, [AllSameValue](const Value *V) {
+        return V == 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
+    // on the original phi node), especially in weird CFG's where some arguments
+    // are unreachable, or uninitialized along certain paths.  This can cause
+    // infinite loops during evaluation. We work around this by not trying to
+    // really evaluate them independently, but instead using a variable
+    // expression to say if one is equivalent to the other.
+    // We also special case undef, so that if we have an undef, we can't use the
+    // common value unless it dominates the phi block.
+    if (HasUndef) {
+      // Only have to check for instructions
+      if (Instruction *AllSameInst = dyn_cast<Instruction>(AllSameValue))
+        if (!DT->dominates(AllSameInst, I))
+          return E;
     }
 
-  if (AllSameValue) {
-    // It's possible to have phi nodes with cycles (IE dependent on
-    // other phis that are .... dependent on the original phi node),
-    // especially in weird CFG's where some arguments are unreachable, or
-    // uninitialized along certain paths.
-    // This can cause infinite loops  during evaluation (even if you disable
-    // the recursion below, you will simply ping-pong between congruence
-    // classes). If a phi node symbolically evaluates to another phi node,
-    // just leave it alone. If they are really the same, we will still
-    // eliminate them in favor of each other.
-    if (isa<PHINode>(AllSameValue))
-      return E;
     NumGVNPhisAllSame++;
     DEBUG(dbgs() << "Simplified PHI node " << *I << " to " << *AllSameValue
                  << "\n");
@@ -1867,7 +1881,6 @@ private:
   SmallVector<std::pair<int, int>, 8> DFSStack;
 };
 }
-
 bool NewGVN::eliminateInstructions(Function &F) {
   // This is a non-standard eliminator. The normal way to eliminate is
   // to walk the dominator tree in order, keeping track of available
@@ -1984,9 +1997,13 @@ bool NewGVN::eliminateInstructions(Funct
           Value *Member = C.Val;
           Use *MemberUse = C.U;
 
-          // We ignore void things because we can't get a value from them.
-          if (Member && Member->getType()->isVoidTy())
-            continue;
+          if (Member) {
+            // We ignore void things because we can't get a value from them.
+            // FIXME: We could actually use this to kill dead stores that are
+            // dominated by equivalent earlier stores.
+            if (Member->getType()->isVoidTy())
+              continue;
+          }
 
           if (EliminationStack.empty()) {
             DEBUG(dbgs() << "Elimination Stack is empty\n");
@@ -1995,8 +2012,6 @@ bool NewGVN::eliminateInstructions(Funct
                          << EliminationStack.dfs_back().first << ","
                          << EliminationStack.dfs_back().second << ")\n");
           }
-          if (Member && isa<Constant>(Member))
-            assert(isa<Constant>(CC->RepLeader));
 
           DEBUG(dbgs() << "Current DFS numbers are (" << MemberDFSIn << ","
                        << MemberDFSOut << ")\n");
@@ -2037,11 +2052,8 @@ bool NewGVN::eliminateInstructions(Funct
             continue;
           Value *Result = EliminationStack.back();
 
-          // Don't replace our existing users with ourselves, and don't replace
-          // phi node arguments with the result of the same phi node.
-          // IE tmp = phi(tmp11, undef); tmp11 = foo -> tmp = phi(tmp, undef)
-          if (MemberUse->get() == Result ||
-              (isa<PHINode>(Result) && MemberUse->getUser() == Result))
+          // Don't replace our existing users with ourselves.
+          if (MemberUse->get() == Result)
             continue;
 
           DEBUG(dbgs() << "Found replacement " << *Result << " for "

Added: llvm/trunk/test/Transforms/NewGVN/cyclic-phi-handling.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/cyclic-phi-handling.ll?rev=291308&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/cyclic-phi-handling.ll (added)
+++ llvm/trunk/test/Transforms/NewGVN/cyclic-phi-handling.ll Fri Jan  6 18:01:42 2017
@@ -0,0 +1,37 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -basicaa -newgvn -S | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @foo(i32 %arg, i32 %arg1, i32 (i32, i32)* %arg2) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br label %bb3
+; CHECK:       bb3:
+; CHECK-NEXT:    [[TMP:%.*]] = phi i32 [ %arg1, %bb ], [ [[TMP:%.*]]4, %bb7 ]
+; CHECK-NEXT:    [[TMP4:%.*]] = phi i32 [ %arg, %bb ], [ [[TMP]], %bb7 ]
+; CHECK-NEXT:    [[TMP5:%.*]] = call i32 %arg2(i32 [[TMP4]], i32 [[TMP]])
+; CHECK-NEXT:    [[TMP6:%.*]] = icmp ne i32 [[TMP5]], 0
+; CHECK-NEXT:    br i1 [[TMP6]], label %bb7, label %bb8
+; CHECK:       bb7:
+; CHECK-NEXT:    br label %bb3
+; CHECK:       bb8:
+; CHECK-NEXT:    ret void
+;
+bb:
+  br label %bb3
+
+;; While non-standard, llvm allows mutually dependent phi nodes
+;; Ensure we do not infinite loop trying to process them
+bb3:                                              ; preds = %bb7, %bb
+  %tmp = phi i32 [ %arg1, %bb ], [ %tmp4, %bb7 ]
+  %tmp4 = phi i32 [ %arg, %bb ], [ %tmp, %bb7 ]
+  %tmp5 = call i32 %arg2(i32 %tmp4, i32 %tmp)
+  %tmp6 = icmp ne i32 %tmp5, 0
+  br i1 %tmp6, label %bb7, label %bb8
+
+bb7:                                              ; preds = %bb3
+  br label %bb3
+
+bb8:                                              ; preds = %bb3
+  ret void
+}

Added: llvm/trunk/test/Transforms/NewGVN/pr31501.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/NewGVN/pr31501.ll?rev=291308&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/NewGVN/pr31501.ll (added)
+++ llvm/trunk/test/Transforms/NewGVN/pr31501.ll Fri Jan  6 18:01:42 2017
@@ -0,0 +1,136 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -basicaa -newgvn -S | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+
+%struct.foo = type { %struct.wombat.28*, %struct.zot, %struct.wombat.28* }
+%struct.zot = type { i64 }
+%struct.barney = type <{ %struct.wombat.28*, %struct.wibble, %struct.snork, %struct.quux.4, %struct.snork.10, %struct.ham.16*, %struct.wobble.23*, i32, i8, i8, [2 x i8] }>
+%struct.wibble = type { %struct.pluto, %struct.bar }
+%struct.pluto = type { %struct.quux }
+%struct.quux = type { %struct.eggs }
+%struct.eggs = type { %struct.zot.0, %struct.widget }
+%struct.zot.0 = type { i8*, i8*, i8* }
+%struct.widget = type { %struct.barney.1 }
+%struct.barney.1 = type { [8 x i8] }
+%struct.bar = type { [3 x %struct.widget] }
+%struct.snork = type <{ %struct.wobble, %struct.bar.3, [7 x i8] }>
+%struct.wobble = type { %struct.wombat }
+%struct.wombat = type { %struct.zot.2 }
+%struct.zot.2 = type { %struct.zot.0, %struct.ham }
+%struct.ham = type { %struct.barney.1 }
+%struct.bar.3 = type { i8 }
+%struct.quux.4 = type <{ %struct.quux.5, %struct.snork.9, [7 x i8] }>
+%struct.quux.5 = type { %struct.widget.6 }
+%struct.widget.6 = type { %struct.spam }
+%struct.spam = type { %struct.zot.0, %struct.ham.7 }
+%struct.ham.7 = type { %struct.barney.8 }
+%struct.barney.8 = type { [24 x i8] }
+%struct.snork.9 = type { i8 }
+%struct.snork.10 = type <{ %struct.foo.11, %struct.spam.15, [7 x i8] }>
+%struct.foo.11 = type { %struct.snork.12 }
+%struct.snork.12 = type { %struct.wombat.13 }
+%struct.wombat.13 = type { %struct.zot.0, %struct.wibble.14 }
+%struct.wibble.14 = type { %struct.barney.8 }
+%struct.spam.15 = type { i8 }
+%struct.ham.16 = type { %struct.pluto.17, %struct.pluto.17 }
+%struct.pluto.17 = type { %struct.bar.18 }
+%struct.bar.18 = type { %struct.baz*, %struct.zot.20, %struct.barney.22 }
+%struct.baz = type { %struct.wibble.19* }
+%struct.wibble.19 = type <{ %struct.baz, %struct.wibble.19*, %struct.baz*, i8, [7 x i8] }>
+%struct.zot.20 = type { %struct.ham.21 }
+%struct.ham.21 = type { %struct.baz }
+%struct.barney.22 = type { %struct.blam }
+%struct.blam = type { i64 }
+%struct.wobble.23 = type { %struct.spam.24, %struct.barney* }
+%struct.spam.24 = type { %struct.bar.25, %struct.zot.26* }
+%struct.bar.25 = type <{ i32 (...)**, i8, i8 }>
+%struct.zot.26 = type { i32 (...)**, i32, %struct.widget.27* }
+%struct.widget.27 = type { %struct.zot.26, %struct.zot.26* }
+%struct.wombat.28 = type <{ i32 (...)**, i8, i8, [6 x i8] }>
+
+; Function Attrs: norecurse nounwind ssp uwtable
+define weak_odr hidden %struct.foo* @quux(%struct.barney* %arg, %struct.wombat.28* %arg1) local_unnamed_addr #0 align 2 {
+; CHECK-LABEL: @quux(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[TMP:%.*]] = getelementptr inbounds %struct.barney, %struct.barney* %arg, i64 0, i32 3, i32 0, i32 0, i32 0
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast %struct.spam* [[TMP]] to %struct.foo**
+; CHECK-NEXT:    [[TMP3:%.*]] = load %struct.foo*, %struct.foo** [[TMP2]], align 8, !tbaa !2
+; CHECK-NEXT:    [[TMP4:%.*]] = getelementptr inbounds %struct.barney, %struct.barney* %arg, i64 0, i32 3, i32 0, i32 0, i32 0, i32 0, i32 1
+; CHECK-NEXT:    [[TMP5:%.*]] = bitcast i8** [[TMP4]] to %struct.foo**
+; CHECK-NEXT:    [[TMP6:%.*]] = load %struct.foo*, %struct.foo** [[TMP5]], align 8, !tbaa !7
+; CHECK-NEXT:    [[TMP7:%.*]] = icmp eq %struct.foo* [[TMP3]], [[TMP6]]
+; CHECK-NEXT:    br i1 [[TMP7]], label %bb21, label %bb8
+; CHECK:       bb8:
+; CHECK-NEXT:    br label %bb11
+; CHECK:       bb9:
+; CHECK-NEXT:    [[TMP10:%.*]] = icmp eq %struct.foo* [[TMP18:%.*]], [[TMP6]]
+; CHECK-NEXT:    br i1 [[TMP10]], label %bb19, label %bb11
+; CHECK:       bb11:
+; CHECK-NEXT:    [[TMP12:%.*]] = phi %struct.foo* [ [[TMP17:%.*]], %bb9 ], [ undef, %bb8 ]
+; CHECK-NEXT:    [[TMP13:%.*]] = phi %struct.foo* [ [[TMP18]], %bb9 ], [ [[TMP3]], %bb8 ]
+; CHECK-NEXT:    [[TMP14:%.*]] = getelementptr inbounds %struct.foo, %struct.foo* [[TMP13]], i64 0, i32 0
+; CHECK-NEXT:    [[TMP15:%.*]] = load %struct.wombat.28*, %struct.wombat.28** [[TMP14]], align 8, !tbaa !8
+; CHECK-NEXT:    [[TMP16:%.*]] = icmp eq %struct.wombat.28* [[TMP15]], %arg1
+; CHECK-NEXT:    [[TMP17]] = select i1 [[TMP16]], %struct.foo* [[TMP13]], %struct.foo* [[TMP12]]
+; CHECK-NEXT:    [[TMP18]] = getelementptr inbounds %struct.foo, %struct.foo* [[TMP13]], i64 1
+; CHECK-NEXT:    br i1 [[TMP16]], label %bb19, label %bb9
+; CHECK:       bb19:
+; CHECK-NEXT:    [[TMP20:%.*]] = phi %struct.foo* [ null, %bb9 ], [ [[TMP17]], %bb11 ]
+; CHECK-NEXT:    br label %bb21
+; CHECK:       bb21:
+; CHECK-NEXT:    [[TMP22:%.*]] = phi %struct.foo* [ null, %bb ], [ [[TMP20]], %bb19 ]
+; CHECK-NEXT:    ret %struct.foo* [[TMP22]]
+;
+bb:
+  %tmp = getelementptr inbounds %struct.barney, %struct.barney* %arg, i64 0, i32 3, i32 0, i32 0, i32 0
+  %tmp2 = bitcast %struct.spam* %tmp to %struct.foo**
+  %tmp3 = load %struct.foo*, %struct.foo** %tmp2, align 8, !tbaa !2
+  %tmp4 = getelementptr inbounds %struct.barney, %struct.barney* %arg, i64 0, i32 3, i32 0, i32 0, i32 0, i32 0, i32 1
+  %tmp5 = bitcast i8** %tmp4 to %struct.foo**
+  %tmp6 = load %struct.foo*, %struct.foo** %tmp5, align 8, !tbaa !7
+  %tmp7 = icmp eq %struct.foo* %tmp3, %tmp6
+  br i1 %tmp7, label %bb21, label %bb8
+
+bb8:                                              ; preds = %bb
+  br label %bb11
+
+bb9:                                              ; preds = %bb11
+  %tmp10 = icmp eq %struct.foo* %tmp18, %tmp6
+  br i1 %tmp10, label %bb19, label %bb11
+
+bb11:                                             ; preds = %bb9, %bb8
+  %tmp12 = phi %struct.foo* [ %tmp17, %bb9 ], [ undef, %bb8 ]
+  %tmp13 = phi %struct.foo* [ %tmp18, %bb9 ], [ %tmp3, %bb8 ]
+  %tmp14 = getelementptr inbounds %struct.foo, %struct.foo* %tmp13, i64 0, i32 0
+  %tmp15 = load %struct.wombat.28*, %struct.wombat.28** %tmp14, align 8, !tbaa !8
+  %tmp16 = icmp eq %struct.wombat.28* %tmp15, %arg1
+  %tmp17 = select i1 %tmp16, %struct.foo* %tmp13, %struct.foo* %tmp12
+  %tmp18 = getelementptr inbounds %struct.foo, %struct.foo* %tmp13, i64 1
+  br i1 %tmp16, label %bb19, label %bb9
+
+bb19:                                             ; preds = %bb11, %bb9
+  %tmp20 = phi %struct.foo* [ null, %bb9 ], [ %tmp17, %bb11 ]
+  br label %bb21
+
+bb21:                                             ; preds = %bb19, %bb
+  %tmp22 = phi %struct.foo* [ null, %bb ], [ %tmp20, %bb19 ]
+  ret %struct.foo* %tmp22
+}
+
+attributes #0 = { norecurse nounwind ssp uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="penryn" "target-features"="+cx16,+fxsr,+mmx,+sse,+sse2,+sse3,+sse4.1,+ssse3,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 4.0.0 (http://llvm.org/git/clang.git b63fa9e2bb8aac0a80c3e3467991c6b1a4b01e6a) (llvm/trunk 290779)"}
+!2 = !{!3, !4, i64 0}
+!3 = !{!"_ZTSN4llvm15SmallVectorBaseE", !4, i64 0, !4, i64 8, !4, i64 16}
+!4 = !{!"any pointer", !5, i64 0}
+!5 = !{!"omnipotent char", !6, i64 0}
+!6 = !{!"Simple C++ TBAA"}
+!7 = !{!3, !4, i64 8}
+!8 = !{!9, !4, i64 0}
+!9 = !{!"_ZTSN4llvm9RecordValE", !4, i64 0, !10, i64 8, !4, i64 16}
+!10 = !{!"_ZTSN4llvm14PointerIntPairIPNS_5RecTyELj1EbNS_21PointerLikeTypeTraitsIS2_EENS_18PointerIntPairInfoIS2_Lj1ES4_EEEE", !11, i64 0}
+!11 = !{!"long", !5, i64 0}




More information about the llvm-commits mailing list