[IfConversion] Fix bug related to !HasFallThrough #145471
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-arm Author: Björn Pettersson (bjope) ChangesWe can not trust that !HasFallThrough implies that there is not Adding a new blockNeverFallThrough helper to make the tests on We could add more logic to blockNeverFallThrough to also check Full diff: https://github.com/llvm/llvm-project/pull/145471.diff 3 Files Affected:
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, _
+...
|
| # 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 |
There was a problem hiding this comment.
Please remove the second "ended up".
| ; 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) |
There was a problem hiding this comment.
Maybe mention something in the commit message about why we get a diff in this testcase? I suppose it was correct before too, just that now the condition in bb.0 and bb.1/bb.2 are swapped compared to before?
There was a problem hiding this comment.
I took another look at this. And I'll make an update to also check for empty successor list in blockNeverFallThrough, which would imply that there is no fallthrough. That way we can let IfConversion merge the blocks (just like it used to do here), and avoid the lit test diff.
Given that BlockFolding (called by IfConversion in the end) did a similar cleanup it did not matter much for this specific test case, but maybe there could be potential regressions in similar situations with no-successor blocks. So it is probably better to handle that as well.
(I had some doubts earlier about inspecting the successor lists in blockNeverFallThrough, but I actually had missed the check for empty successor lists in my earlier experiments. Now it makes more sense.)
llvm/lib/CodeGen/IfConversion.cpp
Outdated
| // fallthrough. | ||
| MachineFunction::iterator PI = BBI.BB->getIterator(); | ||
| MachineFunction::iterator I = std::next(PI); | ||
| if (I != BBI.BB->getParent()->end() && !PI->isSuccessor(&*I)) |
There was a problem hiding this comment.
I == BBI.BB->getParent()->end() implies this is the last block of the function, so it can't fallthrough.
There was a problem hiding this comment.
Right. I added a fixup to catch that.
There was a problem hiding this comment.
With that, can you also drop the redundant succ_empty() check?
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.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
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. The bug has probably been around for years. Found it when working on an out-of-tree target.
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
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. The bug has probably been around for years. Found it when working on an out-of-tree target.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.