[SCCP] Don't mark edges feasible when resolving undefs

nikic · nikic · commit 1f88d804083a · 2022-06-22T10:28:27.000+02:00
As branch on undef is immediate undefined behavior, there is no need to mark one of the edges as feasible. We can leave all the edges non-feasible. In IPSCCP, we can replace the branch with an unreachable terminator. Differential Revision: https://reviews.llvm.org/D126962
diff --git a/llvm/lib/Transforms/Scalar/SCCP.cpp b/llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -371,7 +371,19 @@ static bool removeNonFeasibleEdges(const SCCPSolver &Solver, BasicBlock *BB,
           isa<IndirectBrInst>(TI)) &&
          "Terminator must be a br, switch or indirectbr");
 
-  if (FeasibleSuccessors.size() == 1) {
+  if (FeasibleSuccessors.size() == 0) {
+    // Branch on undef/poison, replace with unreachable.
+    SmallPtrSet<BasicBlock *, 8> SeenSuccs;
+    SmallVector<DominatorTree::UpdateType, 8> Updates;
+    for (BasicBlock *Succ : successors(BB)) {
+      Succ->removePredecessor(BB);
+      if (SeenSuccs.insert(Succ).second)
+        Updates.push_back({DominatorTree::Delete, BB, Succ});
+    }
+    TI->eraseFromParent();
+    new UnreachableInst(BB->getContext(), BB);
+    DTU.applyUpdatesPermissive(Updates);
+  } else if (FeasibleSuccessors.size() == 1) {
     // Replace with an unconditional branch to the only feasible successor.
     BasicBlock *OnlyFeasibleSuccessor = *FeasibleSuccessors.begin();
     SmallVector<DominatorTree::UpdateType, 8> Updates;
diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -1444,22 +1444,19 @@ void SCCPInstVisitor::solve() {
   }
 }
 
-/// resolvedUndefsIn - While solving the dataflow for a function, we assume
-/// that branches on undef values cannot reach any of their successors.
-/// However, this is not a safe assumption.  After we solve dataflow, this
-/// method should be use to handle this.  If this returns true, the solver
-/// should be rerun.
+/// While solving the dataflow for a function, we don't compute a result for
+/// operations with an undef operand, to allow undef to be lowered to a
+/// constant later. For example, constant folding of "zext i8 undef to i16"
+/// would result in "i16 0", and if undef is later lowered to "i8 1", then the
+/// zext result would become "i16 1" and would result into an overdefined
+/// lattice value once merged with the previous result. Not computing the
+/// result of the zext (treating undef the same as unknown) allows us to handle
+/// a later undef->constant lowering more optimally.
 ///
-/// This method handles this by finding an unresolved branch and marking it one
-/// of the edges from the block as being feasible, even though the condition
-/// doesn't say it would otherwise be.  This allows SCCP to find the rest of the
-/// CFG and only slightly pessimizes the analysis results (by marking one,
-/// potentially infeasible, edge feasible).  This cannot usefully modify the
-/// constraints on the condition of the branch, as that would impact other users
-/// of the value.
-///
-/// This scan also checks for values that use undefs. It conservatively marks
-/// them as overdefined.
+/// However, if the operand remains undef when the solver returns, we do need
+/// to assign some result to the instruction (otherwise we would treat it as
+/// unreachable). For simplicity, we mark any instructions that are still
+/// unknown/undef as overdefined.
 bool SCCPInstVisitor::resolvedUndefsIn(Function &F) {
   bool MadeChange = false;
   for (BasicBlock &BB : F) {
@@ -1520,91 +1517,6 @@ bool SCCPInstVisitor::resolvedUndefsIn(Function &F) {
       markOverdefined(&I);
       MadeChange = true;
     }
-
-    // Check to see if we have a branch or switch on an undefined value.  If so
-    // we force the branch to go one way or the other to make the successor
-    // values live.  It doesn't really matter which way we force it.
-    Instruction *TI = BB.getTerminator();
-    if (auto *BI = dyn_cast<BranchInst>(TI)) {
-      if (!BI->isConditional())
-        continue;
-      if (!getValueState(BI->getCondition()).isUnknownOrUndef())
-        continue;
-
-      // If the input to SCCP is actually branch on undef, fix the undef to
-      // false.
-      if (isa<UndefValue>(BI->getCondition())) {
-        BI->setCondition(ConstantInt::getFalse(BI->getContext()));
-        markEdgeExecutable(&BB, TI->getSuccessor(1));
-        MadeChange = true;
-        continue;
-      }
-
-      // Otherwise, it is a branch on a symbolic value which is currently
-      // considered to be undef.  Make sure some edge is executable, so a
-      // branch on "undef" always flows somewhere.
-      // FIXME: Distinguish between dead code and an LLVM "undef" value.
-      BasicBlock *DefaultSuccessor = TI->getSuccessor(1);
-      if (markEdgeExecutable(&BB, DefaultSuccessor))
-        MadeChange = true;
-
-      continue;
-    }
-
-    if (auto *IBR = dyn_cast<IndirectBrInst>(TI)) {
-      // Indirect branch with no successor ?. Its ok to assume it branches
-      // to no target.
-      if (IBR->getNumSuccessors() < 1)
-        continue;
-
-      if (!getValueState(IBR->getAddress()).isUnknownOrUndef())
-        continue;
-
-      // If the input to SCCP is actually branch on undef, fix the undef to
-      // the first successor of the indirect branch.
-      if (isa<UndefValue>(IBR->getAddress())) {
-        IBR->setAddress(BlockAddress::get(IBR->getSuccessor(0)));
-        markEdgeExecutable(&BB, IBR->getSuccessor(0));
-        MadeChange = true;
-        continue;
-      }
-
-      // Otherwise, it is a branch on a symbolic value which is currently
-      // considered to be undef.  Make sure some edge is executable, so a
-      // branch on "undef" always flows somewhere.
-      // FIXME: IndirectBr on "undef" doesn't actually need to go anywhere:
-      // we can assume the branch has undefined behavior instead.
-      BasicBlock *DefaultSuccessor = IBR->getSuccessor(0);
-      if (markEdgeExecutable(&BB, DefaultSuccessor))
-        MadeChange = true;
-
-      continue;
-    }
-
-    if (auto *SI = dyn_cast<SwitchInst>(TI)) {
-      if (!SI->getNumCases() ||
-          !getValueState(SI->getCondition()).isUnknownOrUndef())
-        continue;
-
-      // If the input to SCCP is actually switch on undef, fix the undef to
-      // the first constant.
-      if (isa<UndefValue>(SI->getCondition())) {
-        SI->setCondition(SI->case_begin()->getCaseValue());
-        markEdgeExecutable(&BB, SI->case_begin()->getCaseSuccessor());
-        MadeChange = true;
-        continue;
-      }
-
-      // Otherwise, it is a branch on a symbolic value which is currently
-      // considered to be undef.  Make sure some edge is executable, so a
-      // branch on "undef" always flows somewhere.
-      // FIXME: Distinguish between dead code and an LLVM "undef" value.
-      BasicBlock *DefaultSuccessor = SI->case_begin()->getCaseSuccessor();
-      if (markEdgeExecutable(&BB, DefaultSuccessor))
-        MadeChange = true;
-
-      continue;
-    }
   }
 
   return MadeChange;
diff --git a/llvm/test/Transforms/FunctionSpecialization/bug52821-use-after-free.ll b/llvm/test/Transforms/FunctionSpecialization/bug52821-use-after-free.ll
@@ -34,7 +34,7 @@ for.body:                                     ; preds = %for.cond
 
 for.cond2:                                     ; preds = %for.body2, %for.cond
   %phi2 = phi %mystruct* [ undef, %for.body2 ], [ null, %for.cond ]
-  br i1 undef, label %for.end, label %for.body2
+  br i1 false, label %for.end, label %for.body2
 
 for.body2:                                     ; preds = %for.cond2
   %arrayidx = getelementptr inbounds %mystruct, %mystruct* %phi2, i64 0, i32 1, i64 3
diff --git a/llvm/test/Transforms/FunctionSpecialization/bug55000-read-uninitialized-value.ll b/llvm/test/Transforms/FunctionSpecialization/bug55000-read-uninitialized-value.ll
@@ -10,7 +10,7 @@ declare hidden { i8, ptr } @getType(ptr) align 2
 define internal void @foo(ptr %TLI, ptr %DL, ptr %Ty, ptr %ValueVTs, ptr %Offsets, i64 %StartingOffset) {
 entry:
   %VT = alloca i64, align 8
-  br i1 undef, label %if.then, label %if.end4
+  br i1 false, label %if.then, label %if.end4
 
 if.then:                                          ; preds = %entry
   ret void
diff --git a/llvm/test/Transforms/SCCP/2004-12-10-UndefBranchBug.ll b/llvm/test/Transforms/SCCP/2004-12-10-UndefBranchBug.ll
@@ -1,15 +1,14 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=sccp -S | FileCheck %s
 
-; This function definitely returns 1, even if we don't know the direction
-; of the branch.
+; Branch on undef is UB, so the T block is never executed, and we can return
+; undef (IPSCCP would replace the block with unreachable).
 
 define i32 @foo() {
 ; CHECK-LABEL: @foo(
-; CHECK-NEXT:    br i1 false, label [[T:%.*]], label [[T]]
+; CHECK-NEXT:    br i1 undef, label [[T:%.*]], label [[T]]
 ; CHECK:       T:
-; CHECK-NEXT:    [[X:%.*]] = add i32 0, 1
-; CHECK-NEXT:    ret i32 [[X]]
+; CHECK-NEXT:    ret i32 undef
 ;
   br i1 undef, label %T, label %T
 T:
diff --git a/llvm/test/Transforms/SCCP/2008-01-27-UndefCorrelate.ll b/llvm/test/Transforms/SCCP/2008-01-27-UndefCorrelate.ll
@@ -7,21 +7,16 @@ define i32 @main() {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[BB:%.*]]
 ; CHECK:       bb:
-; CHECK-NEXT:    [[INDVAR:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[K:%.*]], [[BB_BACKEDGE:%.*]] ]
-; CHECK-NEXT:    [[K]] = add i32 [[INDVAR]], 1
-; CHECK-NEXT:    br i1 false, label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
+; CHECK-NEXT:    br i1 undef, label [[COND_TRUE:%.*]], label [[COND_FALSE:%.*]]
 ; CHECK:       cond_true:
-; CHECK-NEXT:    br i1 undef, label [[BB_BACKEDGE]], label [[BB12:%.*]]
+; CHECK-NEXT:    br i1 undef, label [[BB_BACKEDGE:%.*]], label [[BB12:%.*]]
 ; CHECK:       bb.backedge:
 ; CHECK-NEXT:    br label [[BB]]
 ; CHECK:       cond_false:
-; CHECK-NEXT:    [[TMP9:%.*]] = icmp slt i32 [[K]], 10
-; CHECK-NEXT:    br i1 [[TMP9]], label [[BB_BACKEDGE]], label [[BB12]]
+; CHECK-NEXT:    br i1 undef, label [[BB_BACKEDGE]], label [[BB12]]
 ; CHECK:       bb12:
-; CHECK-NEXT:    [[TMP14:%.*]] = icmp eq i32 [[K]], 10
-; CHECK-NEXT:    br i1 [[TMP14]], label [[COND_NEXT18:%.*]], label [[COND_TRUE17:%.*]]
+; CHECK-NEXT:    br i1 undef, label [[COND_NEXT18:%.*]], label [[COND_TRUE17:%.*]]
 ; CHECK:       cond_true17:
-; CHECK-NEXT:    tail call void @abort()
 ; CHECK-NEXT:    unreachable
 ; CHECK:       cond_next18:
 ; CHECK-NEXT:    ret i32 0
diff --git a/llvm/test/Transforms/SCCP/PR26044.ll b/llvm/test/Transforms/SCCP/PR26044.ll
@@ -5,14 +5,15 @@ target triple = "x86_64-unknown-linux-gnu"
 
 define void @fn2(i32* %P) {
 ; CHECK-LABEL: define {{[^@]+}}@fn2
-; CHECK-SAME: (i32* [[P:%.*]])
+; CHECK-SAME: (i32* [[P:%.*]]) {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[IF_END:%.*]]
 ; CHECK:       for.cond1:
-; CHECK-NEXT:    br i1 false, label [[IF_END]], label [[IF_END]]
+; CHECK-NEXT:    unreachable
 ; CHECK:       if.end:
-; CHECK-NEXT:    [[CALL:%.*]] = call i32 @fn1(i32 undef)
-; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]]
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* undef, align 4
+; CHECK-NEXT:    [[CALL:%.*]] = call i32 @fn1(i32 [[TMP0]])
+; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]], align 4
 ; CHECK-NEXT:    br label [[FOR_COND1:%.*]]
 ;
 entry:
@@ -31,10 +32,10 @@ if.end:                                           ; preds = %lbl, %for.cond1
 
 define internal i32 @fn1(i32 %p1) {
 ; CHECK-LABEL: define {{[^@]+}}@fn1
-; CHECK-SAME: (i32 [[P1:%.*]])
+; CHECK-SAME: (i32 [[P1:%.*]]) {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 undef, 0
-; CHECK-NEXT:    [[COND:%.*]] = select i1 [[TOBOOL]], i32 undef, i32 undef
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[P1]], 0
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[TOBOOL]], i32 [[P1]], i32 [[P1]]
 ; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
@@ -45,15 +46,15 @@ entry:
 
 define void @fn_no_null_opt(i32* %P) #0 {
 ; CHECK-LABEL: define {{[^@]+}}@fn_no_null_opt
-; CHECK-SAME: (i32* [[P:%.*]])
+; CHECK-SAME: (i32* [[P:%.*]]) #[[ATTR0:[0-9]+]] {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[IF_END:%.*]]
 ; CHECK:       for.cond1:
-; CHECK-NEXT:    br i1 false, label [[IF_END]], label [[IF_END]]
+; CHECK-NEXT:    unreachable
 ; CHECK:       if.end:
-; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* null, align 4
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* undef, align 4
 ; CHECK-NEXT:    [[CALL:%.*]] = call i32 @fn0(i32 [[TMP0]])
-; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]]
+; CHECK-NEXT:    store i32 [[CALL]], i32* [[P]], align 4
 ; CHECK-NEXT:    br label [[FOR_COND1:%.*]]
 ;
 entry:
@@ -72,7 +73,7 @@ if.end:                                           ; preds = %lbl, %for.cond1
 
 define internal i32 @fn0(i32 %p1) {
 ; CHECK-LABEL: define {{[^@]+}}@fn0
-; CHECK-SAME: (i32 [[P1:%.*]])
+; CHECK-SAME: (i32 [[P1:%.*]]) {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[P1]], 0
 ; CHECK-NEXT:    [[COND:%.*]] = select i1 [[TOBOOL]], i32 [[P1]], i32 [[P1]]
diff --git a/llvm/test/Transforms/SCCP/indirectbr.ll b/llvm/test/Transforms/SCCP/indirectbr.ll
@@ -74,15 +74,12 @@ BB1:
   ret void
 }
 
-; Make sure we eliminate BB1 as we pick the first successor on undef.
+; Branch on undef is UB, so we can convert the indirectbr to unreachable.
 
 define void @indbrtest4(i8** %Q) {
 ; CHECK-LABEL: @indbrtest4(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br label [[BB0:%.*]]
-; CHECK:       BB0:
-; CHECK-NEXT:    call void @BB0_f()
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    unreachable
 ;
 entry:
   indirectbr i8* undef, [label %BB0, label %BB1]
diff --git a/llvm/test/Transforms/SCCP/ipsccp-branch-unresolved-undef.ll b/llvm/test/Transforms/SCCP/ipsccp-branch-unresolved-undef.ll
@@ -13,9 +13,7 @@ define void @main() {
 define internal i1 @patatino(i1 %a) {
 ; CHECK-LABEL: define {{[^@]+}}@patatino
 ; CHECK-SAME: (i1 [[A:%.*]]) {
-; CHECK-NEXT:    br label [[ONFALSE:%.*]]
-; CHECK:       onfalse:
-; CHECK-NEXT:    ret i1 undef
+; CHECK-NEXT:    unreachable
 ;
   br i1 %a, label %ontrue, label %onfalse
 ontrue:
diff --git a/llvm/test/Transforms/SCCP/return-zapped.ll b/llvm/test/Transforms/SCCP/return-zapped.ll
@@ -1,22 +1,17 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
 ; RUN: opt < %s -S -passes=ipsccp | FileCheck %s
 
-; After the first round of Solver.Solve(), the return value of @testf still
-; undefined as we hit a branch on undef. Therefore the conditional branch on
-; @testf's return value in @bar is unknown. In ResolvedUndefsIn, we force the
-; false branch to be feasible. We later discover that @testf actually
-; returns true, so we end up with an unfolded "br i1 true".
+; testf() performs an unconditional branch on undef, as such the testf() return
+; value used in test1() will remain "unknown" and the following branch on it
+; replaced by unreachable. This is fine, as the call to testf() will already
+; trigger undefined behavior.
 define void @test1() {
 ; CHECK-LABEL: define {{[^@]+}}@test1() {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br label [[IF_THEN:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[FOO:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[NEXT:%.*]], [[IF_THEN]] ]
-; CHECK-NEXT:    [[NEXT]] = add i32 [[FOO]], 1
 ; CHECK-NEXT:    [[CALL:%.*]] = call i1 @testf()
-; CHECK-NEXT:    br i1 true, label [[IF_END:%.*]], label [[IF_THEN]]
-; CHECK:       if.end:
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    unreachable
 ;
 entry:
   br label %if.then
@@ -33,9 +28,7 @@ if.end:                                           ; preds = %if.then, %entry
 define internal i1 @testf() {
 ; CHECK-LABEL: define {{[^@]+}}@testf() {
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br label [[IF_END3:%.*]]
-; CHECK:       if.end3:
-; CHECK-NEXT:    ret i1 undef
+; CHECK-NEXT:    unreachable
 ;
 entry:
   br i1 undef, label %if.then1, label %if.end3
@@ -54,7 +47,7 @@ define i1 @test2() {
 ; CHECK-NEXT:    br label [[IF_END:%.*]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    [[CALL2:%.*]] = call i1 @testf()
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    ret i1 undef
 ;
 entry:
   br label %if.end
diff --git a/llvm/test/Transforms/SCCP/switch-constantfold-crash.ll b/llvm/test/Transforms/SCCP/switch-constantfold-crash.ll
diff --git a/llvm/test/Transforms/SCCP/switch-undef-constantfoldterminator.ll b/llvm/test/Transforms/SCCP/switch-undef-constantfoldterminator.ll
