[llvm] [IfConversion] Fix bug related to !HasFallThrough (PR #145471)

Björn Pettersson via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 24 01:59:21 PDT 2025


https://github.com/bjope updated https://github.com/llvm/llvm-project/pull/145471

>From 7e657f236ce09f5ae3a2ec9e27781190ee8d5982 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Tue, 24 Jun 2025 09:01:54 +0200
Subject: [PATCH 1/2] [IfConversion] Pre-commit testcase for !HasFallThrough
 bug. NFC

Adding a test case showing that we can't assume that
!HasFallThrough implies that there is no fallthrough exit
in case analyzeBranch returned true (true == "could not analyze").
---
 .../ARM/ifcvt_unanalyzable_fallthrough.mir    | 114 ++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir

diff --git a/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
new file mode 100644
index 0000000000000..a69689b54205e
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
@@ -0,0 +1,114 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=thumbv7-apple-ios -run-pass=if-converter %s -o - | FileCheck %s
+
+# Testcase with unanalyzable branches (that may fallthrough) in the BB
+# following the diamond/triangle.
+
+# Goal here is to showcase a problem seen in the IfConverter when
+# AnalyzeBranch is indicating that the branches couldn't be analyzed. Problem
+# was originally seen for an out-of-tree target, and here we use ARM and a
+# MBB with two conditional jumps to make AnalyzeBranch return false.
+#
+# The problem was that if-converter when analyzing branches was using a
+# variable named HasFallThrough, to remember that an MBB could fallthrough to
+# the textual successor. When HasFallThrough is set we know that there are
+# fallthrough exits, but the opposite is not guaranteed. If
+# HasFallThrough==false there could still be fallthrough exists in situations
+# when analyzeBranch found unanalyzable branches. There were however a couple
+# of places in the code that checked !HasFallThrough assuming that it would
+# imply that there was no fallthrough exit.
+#
+# As a consequence we could end up merging blocks at the end of a converted
+# diamond/triangle and while doing that we messed up when fixing up the CFG
+# related to fallthrough edges. For the test cases below we incorrectly ended
+# up ended up with a fallthrough from the MBBs with two Bcc instructions to
+# the MBB with the STRH after if conversion.
+#
+---
+name:            avoidMergeBlockDiamond
+body:             |
+  ; CHECK-LABEL: name: avoidMergeBlockDiamond
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 0 /* CC::eq */, $cpsr, implicit $sp
+  ; CHECK-NEXT:   $sp = tADDspi $sp, 3, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2:
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  bb.0:
+    tBcc %bb.2, 1, $cpsr
+
+  bb.1:
+    $sp = tADDspi $sp, 1, 14, _
+    tB %bb.4, 14, $noreg
+
+  bb.2:
+    $sp = tADDspi $sp, 2, 14, _
+    tB %bb.4, 14, $noreg
+
+  bb.3:
+    STRH $sp, $sp, $noreg, 0, 14, $noreg
+    tB %bb.3, 14, $noreg
+
+  bb.4:
+    $sp = tADDspi $sp, 3, 14, _
+    tBcc %bb.5, 1, $cpsr
+    tBcc %bb.5, 1, $cpsr
+
+  bb.5:
+  successors:
+    tBX_RET 14, _
+...
+
+# Similar to the above, but with a triangle.
+---
+name:            avoidMergeBlockTriangle
+body:             |
+  ; CHECK-LABEL: name: avoidMergeBlockTriangle
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2:
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  bb.0:
+    tBcc %bb.1, 1, $cpsr
+    tB %bb.3, 14, $noreg
+
+  bb.1:
+    $sp = tADDspi $sp, 1, 14, _
+    tB %bb.3, 14, $noreg
+
+  bb.2:
+    STRH $sp, $sp, $noreg, 0, 14, $noreg
+    tB %bb.2, 14, $noreg
+
+  bb.3:
+    $sp = tADDspi $sp, 2, 14, _
+    tBcc %bb.4, 1, $cpsr
+    tBcc %bb.4, 1, $cpsr
+
+  bb.4:
+  successors:
+    tBX_RET 14, _
+...

>From b52fdebd825968f76125083fbfe34c7d88a10e02 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Tue, 24 Jun 2025 09:03:37 +0200
Subject: [PATCH 2/2] [IfConversion] Fix bug related to !HasFallThrough

We can not trust that !HasFallThrough implies that there is not
fallthrough exit in cases when analyzeBranch failed.

Adding a new blockNeverFallThrough helper to make the tests on
!HasFallThrough safe by also checking IsBrAnalyzable. We also
try to prove no-fallthrough by inspecting the successor list. If
the textual successor isn't in the successor list we know that
there is no fallthrough.
---
 llvm/lib/CodeGen/IfConversion.cpp             | 38 +++++++++++++++----
 .../ARM/ifcvt_unanalyzable_fallthrough.mir    | 36 +++++++++---------
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/llvm/lib/CodeGen/IfConversion.cpp b/llvm/lib/CodeGen/IfConversion.cpp
index 5265bd74d2dbf..7bfffdba9fa44 100644
--- a/llvm/lib/CodeGen/IfConversion.cpp
+++ b/llvm/lib/CodeGen/IfConversion.cpp
@@ -117,7 +117,11 @@ namespace {
     /// IsAnalyzed      - True if BB has been analyzed (info is still valid).
     /// IsEnqueued      - True if BB has been enqueued to be ifcvt'ed.
     /// IsBrAnalyzable  - True if analyzeBranch() returns false.
-    /// HasFallThrough  - True if BB may fallthrough to the following BB.
+    /// HasFallThrough  - True if BB has fallthrough to the following BB.
+    ///                   Note that BB may have a fallthrough if both
+    ///                   !HasFallThrough and !IsBrAnalyzable is true. Also note
+    ///                   that blockNeverFallThrough() can be used to prove that
+    ///                   there is no fall through.
     /// IsUnpredicable  - True if BB is known to be unpredicable.
     /// ClobbersPred    - True if BB could modify predicates (e.g. has
     ///                   cmp, call, etc.)
@@ -125,7 +129,10 @@ namespace {
     /// ExtraCost       - Extra cost for multi-cycle instructions.
     /// ExtraCost2      - Some instructions are slower when predicated
     /// BB              - Corresponding MachineBasicBlock.
-    /// TrueBB / FalseBB- See analyzeBranch().
+    /// TrueBB / FalseBB- See analyzeBranch(), but note that FalseBB can be set
+    ///                   by AnalyzeBranches even if there is a fallthrough. So
+    ///                   it doesn't correspond exactly to the result from
+    ///                   TTI::analyzeBranch.
     /// BrCond          - Conditions for end of block conditional branches.
     /// Predicate       - Predicate used in the BB.
     struct BBInfo {
@@ -397,6 +404,24 @@ namespace {
       return BBI.IsBrAnalyzable && BBI.TrueBB == nullptr;
     }
 
+    /// Returns true if Block is known not to fallthrough to the following BB.
+    bool blockNeverFallThrough(BBInfo &BBI) const {
+      // Trust "HasFallThrough" if we could analyze branches.
+      if (BBI.IsBrAnalyzable)
+        return !BBI.HasFallThrough;
+      // Empty successor list implies that there is no fallthrough.
+      if (BBI.BB->succ_empty())
+        return true;
+      // If the textual successor isn't in the successor list, then there is no
+      // fallthrough.
+      MachineFunction::iterator PI = BBI.BB->getIterator();
+      MachineFunction::iterator I = std::next(PI);
+      if (I != BBI.BB->getParent()->end() && !PI->isSuccessor(&*I))
+        return true;
+      // Could not prove that there is no fallthrough.
+      return false;
+    }
+
     /// Used to sort if-conversion candidates.
     static bool IfcvtTokenCmp(const std::unique_ptr<IfcvtToken> &C1,
                               const std::unique_ptr<IfcvtToken> &C2) {
@@ -1715,9 +1740,8 @@ bool IfConverter::IfConvertTriangle(BBInfo &BBI, IfcvtKind Kind) {
     // Only merge them if the true block does not fallthrough to the false
     // block. By not merging them, we make it possible to iteratively
     // ifcvt the blocks.
-    if (!HasEarlyExit &&
-        NextMBB.pred_size() == 1 && !NextBBI->HasFallThrough &&
-        !NextMBB.hasAddressTaken()) {
+    if (!HasEarlyExit && NextMBB.pred_size() == 1 &&
+        blockNeverFallThrough(*NextBBI) && !NextMBB.hasAddressTaken()) {
       MergeBlocks(BBI, *NextBBI);
       FalseBBDead = true;
     } else {
@@ -2052,8 +2076,8 @@ bool IfConverter::IfConvertDiamond(BBInfo &BBI, IfcvtKind Kind,
     BBI.BB->removeSuccessor(FalseBBI.BB, true);
 
     BBInfo &TailBBI = BBAnalysis[TailBB->getNumber()];
-    bool CanMergeTail = !TailBBI.HasFallThrough &&
-      !TailBBI.BB->hasAddressTaken();
+    bool CanMergeTail =
+        blockNeverFallThrough(TailBBI) && !TailBBI.BB->hasAddressTaken();
     // The if-converted block can still have a predicated terminator
     // (e.g. a predicated return). If that is the case, we cannot merge
     // it with the tail block.
diff --git a/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
index a69689b54205e..d2673c36f0f4c 100644
--- a/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
+++ b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
@@ -21,30 +21,30 @@
 # As a consequence we could end up merging blocks at the end of a converted
 # diamond/triangle and while doing that we messed up when fixing up the CFG
 # related to fallthrough edges. For the test cases below we incorrectly ended
-# up ended up with a fallthrough from the MBBs with two Bcc instructions to
-# the MBB with the STRH after if conversion.
+# up with a fallthrough from the MBBs with two Bcc instructions to the MBB
+# with the STRH after if conversion.
 #
 ---
 name:            avoidMergeBlockDiamond
 body:             |
   ; CHECK-LABEL: name: avoidMergeBlockDiamond
   ; CHECK: bb.0:
-  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 0 /* CC::eq */, $cpsr, implicit $sp
   ; CHECK-NEXT:   $sp = tADDspi $sp, 3, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1:
-  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2:
-  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.2, 14 /* CC::al */, $noreg
   bb.0:
     tBcc %bb.2, 1, $cpsr
 
@@ -76,21 +76,21 @@ name:            avoidMergeBlockTriangle
 body:             |
   ; CHECK-LABEL: name: avoidMergeBlockTriangle
   ; CHECK: bb.0:
-  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   $sp = tADDspi $sp, 1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT:   $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
-  ; CHECK-NEXT:   tBcc %bb.2, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
+  ; CHECK-NEXT:   tBcc %bb.1, 1 /* CC::ne */, $cpsr
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1:
-  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
-  ; CHECK-NEXT: {{  $}}
-  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
-  ; CHECK-NEXT:   tB %bb.1, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.2:
-  ; CHECK-NEXT:   tBX_RET 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
+  ; CHECK-NEXT:   tB %bb.2, 14 /* CC::al */, $noreg
   bb.0:
     tBcc %bb.1, 1, $cpsr
     tB %bb.3, 14, $noreg



More information about the llvm-commits mailing list