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

Björn Pettersson via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 25 00:25:41 PDT 2025


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

>From a73af22d965126ea8a54d1889abd5c3cc4107e5b 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 1/3] [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

>From 6c0d3191580154899b741f8e21663ba53fbd868c Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Tue, 24 Jun 2025 20:41:52 +0200
Subject: [PATCH 2/3] Fixup: Assume that last MBB in MF never fallthrough.

---
 llvm/lib/CodeGen/IfConversion.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/llvm/lib/CodeGen/IfConversion.cpp b/llvm/lib/CodeGen/IfConversion.cpp
index 7bfffdba9fa44..362fe1b5432cb 100644
--- a/llvm/lib/CodeGen/IfConversion.cpp
+++ b/llvm/lib/CodeGen/IfConversion.cpp
@@ -412,11 +412,11 @@ namespace {
       // 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.
+      // If this is the last MBB in the function, or it 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))
+      if (I == BBI.BB->getParent()->end() || !PI->isSuccessor(&*I))
         return true;
       // Could not prove that there is no fallthrough.
       return false;

>From 428e40ae67b068641e46024ea790ae4dd7bd2831 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Wed, 25 Jun 2025 00:31:07 +0200
Subject: [PATCH 3/3] Fixup: Drop redundant succ_empty check

---
 llvm/lib/CodeGen/IfConversion.cpp | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/llvm/lib/CodeGen/IfConversion.cpp b/llvm/lib/CodeGen/IfConversion.cpp
index 362fe1b5432cb..f80e1e8b683b3 100644
--- a/llvm/lib/CodeGen/IfConversion.cpp
+++ b/llvm/lib/CodeGen/IfConversion.cpp
@@ -409,10 +409,7 @@ namespace {
       // 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 this is the last MBB in the function, or it the textual successor
+      // If this is the last MBB in the function, or 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);



More information about the llvm-commits mailing list