[NewGVN] do phi(undef, x) -> x only if x is not poison

phi([undef, A], [x, B]) -> x is only correct x is guaranteed to be
a non-poison value.
Otherwise we would be changing an undef to poison in the branch A.

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

Author: nunoplopes

Diff for: llvm/lib/Transforms/Scalar/NewGVN.cpp

@@ -77,6 +77,7 @@
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constant.h"
@@ -1736,18 +1737,18 @@ NewGVN::performSymbolicPHIEvaluation(ArrayRef<ValPair> PHIOps,
   if (Filtered.empty()) {
     // If it has undef or poison at this point, it means there are no-non-undef
     // arguments, and thus, the value of the phi node must be undef.
-    if (HasPoison && !HasUndef) {
-      LLVM_DEBUG(
-          dbgs() << "PHI Node " << *I
-                 << " has no non-poison arguments, valuing it as poison\n");
-      return createConstantExpression(PoisonValue::get(I->getType()));
-    }
     if (HasUndef) {
       LLVM_DEBUG(
           dbgs() << "PHI Node " << *I
                  << " has no non-undef arguments, valuing it as undef\n");
       return createConstantExpression(UndefValue::get(I->getType()));
     }
+    if (HasPoison) {
+      LLVM_DEBUG(
+          dbgs() << "PHI Node " << *I
+                 << " has no non-poison arguments, valuing it as poison\n");
+      return createConstantExpression(PoisonValue::get(I->getType()));
+    }
 
     LLVM_DEBUG(dbgs() << "No arguments of PHI node " << *I << " are live\n");
     deleteExpression(E);
@@ -1757,15 +1758,20 @@ NewGVN::performSymbolicPHIEvaluation(ArrayRef<ValPair> PHIOps,
   ++Filtered.begin();
   // Can't use std::equal here, sadly, because filter.begin moves.
   if (llvm::all_of(Filtered, [&](Value *Arg) { return Arg == AllSameValue; })) {
+    // Can't fold phi(undef, X) -> X unless X can't be poison (thus X is undef
+    // in the worst case).
+    if (HasUndef && !isGuaranteedNotToBePoison(AllSameValue, AC, nullptr, DT))
+      return E;
+
     // 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.
+    // We also special case undef/poison, so that if we have an undef, we can't
+    // use the common value unless it dominates the phi block.
     if (HasPoison || HasUndef) {
       // If we have undef and at least one other value, this is really a
       // multivalued phi, and we need to know if it's cycle free in order to
@@ -2853,14 +2859,14 @@ NewGVN::makePossiblePHIOfOps(Instruction *I,
 }
 
 // The algorithm initially places the values of the routine in the TOP
-// congruence class. The leader of TOP is the undetermined value `undef`.
+// congruence class. The leader of TOP is the undetermined value `poison`.
 // When the algorithm has finished, values still in TOP are unreachable.
 void NewGVN::initializeCongruenceClasses(Function &F) {
   NextCongruenceNum = 0;
 
   // Note that even though we use the live on entry def as a representative
   // MemoryAccess, it is *not* the same as the actual live on entry def. We
-  // have no real equivalemnt to undef for MemoryAccesses, and so we really
+  // have no real equivalent to poison for MemoryAccesses, and so we really
   // should be checking whether the MemoryAccess is top if we want to know if it
   // is equivalent to everything.  Otherwise, what this really signifies is that
   // the access "it reaches all the way back to the beginning of the function"
@@ -3031,7 +3037,7 @@ void NewGVN::valueNumberMemoryPhi(MemoryPhi *MP) {
            !isMemoryAccessTOP(cast<MemoryAccess>(U)) &&
            ReachableEdges.count({MP->getIncomingBlock(U), PHIBlock});
   });
-  // If all that is left is nothing, our memoryphi is undef. We keep it as
+  // If all that is left is nothing, our memoryphi is poison. We keep it as
   // InitialClass.  Note: The only case this should happen is if we have at
   // least one self-argument.
   if (Filtered.begin() == Filtered.end()) {

Diff for: llvm/test/Transforms/NewGVN/completeness.ll

@@ -505,17 +505,16 @@ declare i32* @wombat()
 ;; change in the verifier, as it goes from a constant value to a
 ;; phi of [true, false]
 
-define void @test12() {
+define void @test12(i32* %p) {
 ; CHECK-LABEL: @test12(
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:    [[TMP:%.*]] = load i32, i32* null
+; CHECK-NEXT:    [[TMP:%.*]] = load i32, i32* %p
 ; CHECK-NEXT:    [[TMP1:%.*]] = icmp sgt i32 [[TMP]], 0
 ; CHECK-NEXT:    br i1 [[TMP1]], label [[BB2:%.*]], label [[BB8:%.*]]
 ; CHECK:       bb2:
 ; CHECK-NEXT:    br label [[BB3:%.*]]
 ; CHECK:       bb3:
-; CHECK-NEXT:    [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB2]] ], [ false, [[BB7:%.*]] ]
-; CHECK-NEXT:    br i1 [[PHIOFOPS]], label [[BB6:%.*]], label [[BB7]]
+; CHECK-NEXT:    br i1 true, label [[BB6:%.*]], label [[BB7]]
 ; CHECK:       bb6:
 ; CHECK-NEXT:    br label [[BB7]]
 ; CHECK:       bb7:
@@ -524,15 +523,15 @@ define void @test12() {
 ; CHECK-NEXT:    ret void
 ;
 bb:
-  %tmp = load i32, i32* null
+  %tmp = load i32, i32* %p
   %tmp1 = icmp sgt i32 %tmp, 0
   br i1 %tmp1, label %bb2, label %bb8
 
 bb2:                                              ; preds = %bb
   br label %bb3
 
 bb3:                                              ; preds = %bb7, %bb2
-  %tmp4 = phi i32 [ %tmp, %bb2 ], [ undef, %bb7 ]
+  %tmp4 = phi i32 [ %tmp, %bb2 ], [ poison, %bb7 ]
   %tmp5 = icmp sgt i32 %tmp4, 0
   br i1 %tmp5, label %bb6, label %bb7
 

Diff for: llvm/test/Transforms/NewGVN/phi-edge-handling.ll

@@ -119,6 +119,27 @@ define i8 @value_undef(i1 %cond, i8 %v) {
 ; CHECK:       B:
 ; CHECK-NEXT:    br label [[EXIT]]
 ; CHECK:       EXIT:
+; CHECK-NEXT:    [[R:%.*]] = phi i8 [ undef, [[A]] ], [ [[V:%.*]], [[B]] ]
+; CHECK-NEXT:    ret i8 [[R]]
+;
+  br i1 %cond, label %A, label %B
+A:
+  br label %EXIT
+B:
+  br label %EXIT
+EXIT:
+  %r = phi i8 [undef, %A], [%v, %B]
+  ret i8 %r
+}
+
+define i8 @value_undef_noundef(i1 %cond, i8 noundef %v) {
+; CHECK-LABEL: @value_undef_noundef(
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[A:%.*]], label [[B:%.*]]
+; CHECK:       A:
+; CHECK-NEXT:    br label [[EXIT:%.*]]
+; CHECK:       B:
+; CHECK-NEXT:    br label [[EXIT]]
+; CHECK:       EXIT:
 ; CHECK-NEXT:    ret i8 [[V:%.*]]
 ;
   br i1 %cond, label %A, label %B

Diff for: llvm/test/Transforms/NewGVN/pr31682.ll

@@ -24,7 +24,7 @@ bb:
   br label %bb2
 
 bb2:                                              ; preds = %bb2, %bb
-  %tmp3 = phi %struct.foo* [ undef, %bb ], [ %tmp6, %bb2 ]
+  %tmp3 = phi %struct.foo* [ poison, %bb ], [ %tmp6, %bb2 ]
   %tmp4 = getelementptr %struct.foo, %struct.foo* %tmp3, i64 0, i32 1
   %tmp5 = load i32, i32* %tmp4
   %tmp6 = load %struct.foo*, %struct.foo** @global

Diff for: llvm/test/Transforms/NewGVN/pr32403.ll

@@ -50,7 +50,7 @@ if.then17.i:                                      ; preds = %for.body8.i
   br label %for.inc24.i
 
 for.inc24.i:                                      ; preds = %if.then17.i, %for.body8.i
-  %nIdx.1.i = phi i32 [ undef, %if.then17.i ], [ %nIdx.052.i, %for.body8.i ]
+  %nIdx.1.i = phi i32 [ poison, %if.then17.i ], [ %nIdx.052.i, %for.body8.i ]
   br label %for.body8.i
 
 if.else58:                                        ; preds = %for.body

Diff for: llvm/test/Transforms/NewGVN/pr32838.ll

@@ -3,7 +3,7 @@
 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) {
+define void @fn1(i64 noundef %arg) {
 ; CHECK-LABEL: @fn1(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br i1 undef, label [[IF_THEN:%.*]], label [[COND_TRUE:%.*]]
@@ -47,7 +47,7 @@ cond.true:
 temp:
   ret void
 }
-define void @fn2(i64 %arg) {
+define void @fn2(i64 noundef %arg) {
 ; CHECK-LABEL: @fn2(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br i1 undef, label [[IF_THEN:%.*]], label [[COND_TRUE:%.*]]

Diff for: llvm/test/Transforms/NewGVN/pr34135.ll

@@ -3,7 +3,7 @@
 ;; Make sure we don't incorrectly use predicateinfo to simplify phi of ops cases
 source_filename = "pr34135.ll"
 
-define void @snork(i32 %arg) {
+define void @snork(i32 noundef %arg) {
 ; CHECK-LABEL: @snork(
 ; CHECK-NEXT:  bb:
 ; CHECK-NEXT:    [[TMP:%.*]] = sext i32 [[ARG:%.*]] to i64