Bail out jump threading on indirect branches

[hiraditya]

The bug was introduced by #68473

Fixes: #102351

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: AdityaK (hiraditya)

Changes

The bug was introduced by #68473

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/Local.cpp (+5-2)
• (added) llvm/test/Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll (+82)

diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index efb02fdec56d7e..4b37656d069bbd 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -868,6 +868,8 @@ CanPropagatePredecessorsForPHIs(BasicBlock *BB, BasicBlock *Succ,
 
   LLVM_DEBUG(dbgs() << "Looking to fold " << BB->getName() << " into "
                     << Succ->getName() << "\n");
+  if (isa<IndirectBrInst>(Succ->getTerminator()))
+    return false;
   // Shortcut, if there is only a single predecessor it must be BB and merging
   // is always safe
   if (Succ->getSinglePredecessor())
@@ -1029,11 +1031,12 @@ CanRedirectPredsOfEmptyBBToSucc(BasicBlock *BB, BasicBlock *Succ,
     return false;
 
   // Get single common predecessors of both BB and Succ
+  // TODO: Replace this with a proper `set intersect` algorithm
   for (BasicBlock *SuccPred : SuccPreds) {
     if (BBPreds.count(SuccPred)) {
+      CommonPred = SuccPred;
       if (CommonPred)
         return false;
-      CommonPred = SuccPred;
     }
   }
 
@@ -1133,7 +1136,7 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB,
 
   bool BBKillable = CanPropagatePredecessorsForPHIs(BB, Succ, BBPreds);
 
-  // Even if we can not fold bB into Succ, we may be able to redirect the
+  // Even if we can not fold BB into Succ, we may be able to redirect the
   // predecessors of BB to Succ.
   bool BBPhisMergeable =
       BBKillable ||
diff --git a/llvm/test/Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll b/llvm/test/Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll
new file mode 100644
index 00000000000000..a715c4f85859f8
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll
@@ -0,0 +1,82 @@
+; RUN: opt < %s -passes=simplifycfg -S | FileCheck %s
+
+; CHECK: switch i32 %state.0, label %sw.epilog
+; CHECK-NEXT: i32 0, label %sw.bb
+; CHECK-NEXT: i32 1, label %VM__OP_1
+; CHECK-NEXT: i32 2, label %sw.bb4
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [23 x i8] c"OP_1:(instruction=%d)\0A\00", align 1
+@.str.1 = private unnamed_addr constant [28 x i8] c"TERMINATE:(instruction=%d)\0A\00", align 1
+
+; Function Attrs: mustprogress norecurse uwtable
+define dso_local noundef i32 @main() #0 {
+entry:
+  %bytecode = alloca [2 x ptr], align 16
+  store ptr blockaddress(@main, %VM__OP_1), ptr %bytecode, align 16, !tbaa !5
+  %arrayidx1 = getelementptr inbounds [2 x ptr], ptr %bytecode, i64 0, i64 1
+  store ptr blockaddress(@main, %VM__TERMINATE), ptr %arrayidx1, align 8, !tbaa !5
+  br label %while.body
+
+while.body:                                       ; preds = %entry, %sw.epilog
+  %state.0 = phi i32 [ 0, %entry ], [ %state.1, %sw.epilog ]
+  %index.0 = phi i32 [ 0, %entry ], [ %index.2, %sw.epilog ]
+  switch i32 %state.0, label %sw.epilog [
+    i32 0, label %sw.bb
+    i32 1, label %VM__OP_1
+    i32 2, label %sw.bb4
+  ]
+
+sw.bb:                                            ; preds = %while.body
+  br label %indirectgoto
+
+VM__OP_1:                                         ; preds = %while.body, %indirectgoto
+  %index.1 = phi i32 [ %index.3, %indirectgoto ], [ %index.0, %while.body ]
+  br label %sw.epilog
+
+sw.bb4:                                           ; preds = %while.body
+  %call = call i32 (ptr, ...) @printf(ptr noundef nonnull dereferenceable(1) @.str, i32 noundef %index.0)
+  %inc = add nsw i32 %index.0, 1
+  br label %indirectgoto
+
+sw.epilog:                                        ; preds = %while.body, %VM__OP_1
+  %state.1 = phi i32 [ %state.0, %while.body ], [ 2, %VM__OP_1 ]
+  %index.2 = phi i32 [ %index.0, %while.body ], [ %index.1, %VM__OP_1 ]
+  br label %while.body, !llvm.loop !9
+
+VM__TERMINATE:                                    ; preds = %indirectgoto
+  %call7 = call i32 (ptr, ...) @printf(ptr noundef nonnull dereferenceable(1) @.str.1, i32 noundef %index.3)
+  ret i32 0
+
+indirectgoto:                                     ; preds = %sw.bb4, %sw.bb
+  %index.3 = phi i32 [ %inc, %sw.bb4 ], [ %index.0, %sw.bb ]
+  %idxprom.pn = sext i32 %index.3 to i64
+  %indirect.goto.dest.in = getelementptr inbounds [2 x ptr], ptr %bytecode, i64 0, i64 %idxprom.pn
+  %indirect.goto.dest = load ptr, ptr %indirect.goto.dest.in, align 8, !tbaa !5
+  indirectbr ptr %indirect.goto.dest, [label %VM__OP_1, label %VM__TERMINATE]
+}
+declare i32 @printf(ptr noundef, ...) #1
+
+attributes #0 = { mustprogress norecurse uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.module.flags = !{!0, !1, !2, !3}
+!llvm.ident = !{!4}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 8, !"PIC Level", i32 2}
+!2 = !{i32 7, !"PIE Level", i32 2}
+!3 = !{i32 7, !"uwtable", i32 2}
+!4 = !{!"clang version 18.0.0git (https://github.com/llvm/llvm-project.git 67782d2de5ea9c8653b8f0110237a3c355291c0e)"}
+!5 = !{!6, !6, i64 0}
+!6 = !{!"any pointer", !7, i64 0}
+!7 = !{!"omnipotent char", !8, i64 0}
+!8 = !{!"Simple C++ TBAA"}
+!9 = !{!10, !10, i64 0}
+!10 = !{!"int", !7, i64 0}
+!11 = distinct !{!11, !12, !13}
+!12 = !{!"llvm.loop.mustprogress"}
+!13 = !{!"llvm.loop.unroll.disable"}
+

[dtcxzyw]

Please remove unnecessary attributes/metadata.

[dtcxzyw]

Failed Tests (1):
LLVM :: Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll

[hiraditya]

Failed Tests (1):
LLVM :: Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll

yeah, another bug that i still need to find.

[dtcxzyw]

Failed Tests (3):
LLVM :: Transforms/JumpThreading/pr40992-indirectbr-folding.ll
LLVM :: Transforms/SimplifyCFG/indirectbr.ll
LLVM :: Transforms/SimplifyCFG/switch-branch-fold-indirectbr-102351.ll

[nikic]

Please rebase this to current main (it looks like CI is broken on your base revision), minimize the test and use update_test_checks.

[hiraditya]

Please rebase this to current main (it looks like CI is broken on your base revision), minimize the test and use update_test_checks.

I minimized a few instructions, lmk if you need it to be smaller

[hiraditya]

Merging this assuming there are no further comments for changes. Please let me know any more changes are required and i'll follow up.

[SanjaLV]

Hey @tru is there any chance this fix goes into LLVM stable release?

This is regression from the LLVM 17 that miscompiles a lot of non-trivial indirect goto code.

We have verified that llvm-17.0.4 does not have this problem, the llvm-18.1.4/llvm-18.1.8 miscompiles the code, llvmorg-19.1.0-rc4 0c64156 also miscompiles the code, but cherry-picking this commit on top of it fixes the problem.

[tru]

Hi, since we are wrapping up this next release we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not.

Is this PR a fix for a regression or a critical issue?

What is the risk of accepting this into the release branch?

What is the risk of NOT accepting this into the release branch?

[nikic]

@tru This is likely to be reverted due to code review policy violation, so this should not be backported at this time.

[hiraditya]

This is likely to be reverted due to code review policy violation

It didn't occur to me that there would be any violation of some sorts. I pushed the latest change last week and there wasn't any feedback since then.

[aemerson]

ping @nikic @hiraditya what's going on with this then?