[Sink] Fix bugs of sinking unreachable BB from phi

[XChy]

Resolve #68529.
Sink pass doesn't consider whether Basicblock from phi is unreachable.
This patch moves unreachability check after checking phi.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Changes

Resolve #68529.
JumpThreading calls TryToSimplifyUncondBranchFromEmptyBlock without checking/removing unreachable pred, which results in incorrectly redirecting value from unreachable predecessor.
This patch adds check for it.

---

Full diff: https://github.com/llvm/llvm-project/pull/68576.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/JumpThreading.cpp (+8)
• (modified) llvm/test/Transforms/JumpThreading/bb-unreachable-from-entry.ll (+35)
• (modified) llvm/test/Transforms/JumpThreading/unreachable-loops.ll (+6-2)

diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index ed69f1938ec7921..fc10243cff19964 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -368,6 +368,13 @@ bool JumpThreadingPass::runImpl(Function &F_, FunctionAnalysisManager *FAM_,
         continue;
       }
 
+      auto PredsAreReachable = [&Unreachable](BasicBlock *BB) -> bool {
+        for (auto *Pred : predecessors(BB))
+          if (Unreachable.count(Pred))
+            return false;
+        return true;
+      };
+
       // processBlock doesn't thread BBs with unconditional TIs. However, if BB
       // is "almost empty", we attempt to merge BB with its sole successor.
       auto *BI = dyn_cast<BranchInst>(BB.getTerminator());
@@ -379,6 +386,7 @@ bool JumpThreadingPass::runImpl(Function &F_, FunctionAnalysisManager *FAM_,
             // Don't alter Loop headers and latches to ensure another pass can
             // detect and transform nested loops later.
             !LoopHeaders.count(&BB) && !LoopHeaders.count(Succ) &&
+            PredsAreReachable(&BB) &&
             TryToSimplifyUncondBranchFromEmptyBlock(&BB, DTU.get())) {
           RemoveRedundantDbgInstrs(Succ);
           // BB is valid for cleanup here because we passed in DTU. F remains
diff --git a/llvm/test/Transforms/JumpThreading/bb-unreachable-from-entry.ll b/llvm/test/Transforms/JumpThreading/bb-unreachable-from-entry.ll
index 2a9765233ec1043..e8fa399ff8a6d99 100644
--- a/llvm/test/Transforms/JumpThreading/bb-unreachable-from-entry.ll
+++ b/llvm/test/Transforms/JumpThreading/bb-unreachable-from-entry.ll
@@ -27,3 +27,38 @@ exit1:
 exit2:
   ret void
 }
+
+define i32 @bar(i32 noundef %a) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[DOTNOT:%.*]] = icmp eq i32 [[A:%.*]], 0
+; CHECK-NEXT:    br i1 [[DOTNOT]], label [[END:%.*]], label [[ELSE:%.*]]
+; CHECK:       else:
+; CHECK-NEXT:    [[B:%.*]] = and i32 undef, 65535
+; CHECK-NEXT:    br label [[B2:%.*]]
+; CHECK:       dead:
+; CHECK-NEXT:    br label [[B2]]
+; CHECK:       B2:
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    [[DOT0:%.*]] = phi i32 [ [[A]], [[ENTRY:%.*]] ], [ [[B]], [[B2]] ]
+; CHECK-NEXT:    ret i32 [[DOT0]]
+;
+entry:
+  %.not = icmp eq i32 %a, 0
+  br i1 %.not, label %end, label %else
+
+  else:                                                ; preds = %1
+  %b = and i32 undef, 65535
+  br label %B2
+
+dead:                                                ; No predecessors!
+  br label %B2
+
+B2:                                                ; preds = %2, %4
+  br label %end
+
+end:                                                ; preds = %5, %1
+  %.0 = phi i32 [ %a, %entry ], [ %b, %B2 ]
+  ret i32 %.0
+}
diff --git a/llvm/test/Transforms/JumpThreading/unreachable-loops.ll b/llvm/test/Transforms/JumpThreading/unreachable-loops.ll
index 8d649761700a90c..7e1dcfb3e4c78a9 100644
--- a/llvm/test/Transforms/JumpThreading/unreachable-loops.ll
+++ b/llvm/test/Transforms/JumpThreading/unreachable-loops.ll
@@ -15,9 +15,11 @@ define void @unreachable_single_bb_loop() {
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[TMP4:%.*]] = icmp ne i32 [[TMP]], 1
 ; CHECK-NEXT:    switch i1 [[TMP4]], label [[BB2:%.*]] [
-; CHECK-NEXT:    i1 false, label [[BB8]]
+; CHECK-NEXT:    i1 false, label [[BB7:%.*]]
 ; CHECK-NEXT:    i1 true, label [[BB8]]
 ; CHECK-NEXT:    ]
+; CHECK:       bb7:
+; CHECK-NEXT:    br label [[BB8]]
 ; CHECK:       bb8:
 ; CHECK-NEXT:    ret void
 ;
@@ -57,9 +59,11 @@ define void @unreachable_multi_bbs_loop() {
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[TMP4:%.*]] = icmp ne i32 [[TMP]], 1
 ; CHECK-NEXT:    switch i1 [[TMP4]], label [[BB3:%.*]] [
-; CHECK-NEXT:    i1 false, label [[BB8]]
+; CHECK-NEXT:    i1 false, label [[BB7:%.*]]
 ; CHECK-NEXT:    i1 true, label [[BB8]]
 ; CHECK-NEXT:    ]
+; CHECK:       bb7:
+; CHECK-NEXT:    br label [[BB8]]
 ; CHECK:       bb8:
 ; CHECK-NEXT:    ret void
 ;

[nikic]

Unreachable blocks are dominated by all blocks, so this is well-defined IR. If a pass fails to handle it correctly, that pass needs to be fixed.

[XChy]

Unreachable blocks are dominated by all blocks, so this is well-defined IR. If a pass fails to handle it correctly, that pass needs to be fixed.

Does it mean value in phi [%value, %unreachable] can be everything defined?
If so, I think that's the problem of Sink Pass. I'll try to fix it.

[nikic]

Unreachable blocks are dominated by all blocks, so this is well-defined IR. If a pass fails to handle it correctly, that pass needs to be fixed.

Does it mean value in phi [%value, %unreachable] can be everything defined?

Yes, that's right.

[nikic]

LGTM