[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