[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