[llvm] [IfConversion] Fix bug related to !HasFallThrough (PR #145471)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 24 00:43:38 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-arm
Author: Björn Pettersson (bjope)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/145471.diff
3 Files Affected:
- (modified) llvm/lib/CodeGen/IfConversion.cpp (+18-7)
- (modified) llvm/test/CodeGen/ARM/ifcvt_triangleWoCvtToNextEdge.mir (+12-10)
- (added) llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir (+114)
``````````diff
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
new file mode 100644
index 0000000000000..72b887afdad96
--- /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.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.1, 1 /* CC::ne */, $cpsr
+ ; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; 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
+
+ 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.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.1, 1 /* CC::ne */, $cpsr
+ ; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; 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
+
+ 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, _
+...
``````````
</details>
https://github.com/llvm/llvm-project/pull/145471
More information about the llvm-commits
mailing list