[llvm] [IfConversion] Fix bug related to !HasFallThrough (PR #145471)
Björn Pettersson via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 24 00:43:11 PDT 2025
https://github.com/bjope created https://github.com/llvm/llvm-project/pull/145471
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 could add more logic to blockNeverFallThrough to also check
if the textual successor is absent from the MBBs successor list
(as that would indicate that there is no fallthrough edge). I'm
not sure it would make much difference though, and in this patch
focus is to just fix the bug.
>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 ce58d967fb4b9d695132b8ed6394da2f52de80c0 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 could add more logic to blockNeverFallThrough to also check
if the textual successor is absent from the MBBs successor list
(as that would indicate that there is no fallthrough edge). I'm
not sure it would make much difference though, and in this patch
focus is to just fix the bug.
---
llvm/lib/CodeGen/IfConversion.cpp | 25 +++++++++++----
.../ARM/ifcvt_triangleWoCvtToNextEdge.mir | 22 +++++++------
.../ARM/ifcvt_unanalyzable_fallthrough.mir | 32 +++++++++----------
3 files changed, 46 insertions(+), 33 deletions(-)
diff --git a/llvm/lib/CodeGen/IfConversion.cpp b/llvm/lib/CodeGen/IfConversion.cpp
index 5265bd74d2dbf..fea3de6ff5cab 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,11 @@ 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 {
+ return BBI.IsBrAnalyzable && !BBI.HasFallThrough;
+ }
+
/// Used to sort if-conversion candidates.
static bool IfcvtTokenCmp(const std::unique_ptr<IfcvtToken> &C1,
const std::unique_ptr<IfcvtToken> &C2) {
@@ -1715,9 +1727,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 +2063,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_triangleWoCvtToNextEdge.mir b/llvm/test/CodeGen/ARM/ifcvt_triangleWoCvtToNextEdge.mir
index 005b433904e30..0cab01e1bd8d0 100644
--- a/llvm/test/CodeGen/ARM/ifcvt_triangleWoCvtToNextEdge.mir
+++ b/llvm/test/CodeGen/ARM/ifcvt_triangleWoCvtToNextEdge.mir
@@ -21,16 +21,18 @@ name: foo
body: |
; CHECK-LABEL: name: foo
; CHECK: bb.0:
- ; CHECK: successors: %bb.2(0x40000000), %bb.1(0x40000000)
- ; CHECK: tBcc %bb.2, 1 /* CC::ne */, $cpsr
- ; CHECK: bb.1:
- ; CHECK: successors:
- ; CHECK: tBL 14 /* CC::al */, $cpsr, @__stack_chk_fail
- ; CHECK: bb.2:
- ; CHECK: tBL 1 /* CC::ne */, $cpsr, @__stack_chk_fail
- ; CHECK: $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
- ; CHECK: $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
- ; CHECK: tTAILJMPdND @bar, 14 /* CC::al */, $cpsr
+ ; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: tBcc %bb.2, 0 /* CC::eq */, $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: tBL 1 /* CC::ne */, $cpsr, @__stack_chk_fail
+ ; CHECK-NEXT: $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
+ ; CHECK-NEXT: $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
+ ; CHECK-NEXT: tTAILJMPdND @bar, 14 /* CC::al */, $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; CHECK-NEXT: tBL 14 /* CC::al */, $cpsr, @__stack_chk_fail
bb.0:
tBcc %bb.1, 1, $cpsr
diff --git a/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
index a69689b54205e..72b887afdad96 100644
--- a/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
+++ b/llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir
@@ -29,22 +29,22 @@ 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