Skip to content

Commit 237b8de

Browse files
bjopebjope
authored
[IfConversion] Fix bug related to !HasFallThrough (#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 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.
1 parent 7c38ee2 commit 237b8de

File tree

2 files changed

+46
-25
lines changed

2 files changed

+46
-25
lines changed

‎llvm/lib/CodeGen/IfConversion.cpp‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -117,15 +117,22 @@ namespace {
117117
/// IsAnalyzed - True if BB has been analyzed (info is still valid).
118118
/// IsEnqueued - True if BB has been enqueued to be ifcvt'ed.
119119
/// IsBrAnalyzable - True if analyzeBranch() returns false.
120-
/// HasFallThrough - True if BB may fallthrough to the following BB.
120+
/// HasFallThrough - True if BB has fallthrough to the following BB.
121+
/// Note that BB may have a fallthrough if both
122+
/// !HasFallThrough and !IsBrAnalyzable is true. Also note
123+
/// that blockNeverFallThrough() can be used to prove that
124+
/// there is no fall through.
121125
/// IsUnpredicable - True if BB is known to be unpredicable.
122126
/// ClobbersPred - True if BB could modify predicates (e.g. has
123127
/// cmp, call, etc.)
124128
/// NonPredSize - Number of non-predicated instructions.
125129
/// ExtraCost - Extra cost for multi-cycle instructions.
126130
/// ExtraCost2 - Some instructions are slower when predicated
127131
/// BB - Corresponding MachineBasicBlock.
128-
/// TrueBB / FalseBB- See analyzeBranch().
132+
/// TrueBB / FalseBB- See analyzeBranch(), but note that FalseBB can be set
133+
/// by AnalyzeBranches even if there is a fallthrough. So
134+
/// it doesn't correspond exactly to the result from
135+
/// TTI::analyzeBranch.
129136
/// BrCond - Conditions for end of block conditional branches.
130137
/// Predicate - Predicate used in the BB.
131138
struct BBInfo {
@@ -397,6 +404,21 @@ namespace {
397404
return BBI.IsBrAnalyzable && BBI.TrueBB == nullptr;
398405
}
399406

407+
/// Returns true if Block is known not to fallthrough to the following BB.
408+
bool blockNeverFallThrough(BBInfo &BBI) const {
409+
// Trust "HasFallThrough" if we could analyze branches.
410+
if (BBI.IsBrAnalyzable)
411+
return !BBI.HasFallThrough;
412+
// If this is the last MBB in the function, or if the textual successor
413+
// isn't in the successor list, then there is no fallthrough.
414+
MachineFunction::iterator PI = BBI.BB->getIterator();
415+
MachineFunction::iterator I = std::next(PI);
416+
if (I == BBI.BB->getParent()->end() || !PI->isSuccessor(&*I))
417+
return true;
418+
// Could not prove that there is no fallthrough.
419+
return false;
420+
}
421+
400422
/// Used to sort if-conversion candidates.
401423
static bool IfcvtTokenCmp(const std::unique_ptr<IfcvtToken> &C1,
402424
const std::unique_ptr<IfcvtToken> &C2) {
@@ -1715,9 +1737,8 @@ bool IfConverter::IfConvertTriangle(BBInfo &BBI, IfcvtKind Kind) {
17151737
// Only merge them if the true block does not fallthrough to the false
17161738
// block. By not merging them, we make it possible to iteratively
17171739
// ifcvt the blocks.
1718-
if (!HasEarlyExit &&
1719-
NextMBB.pred_size() == 1 && !NextBBI->HasFallThrough &&
1720-
!NextMBB.hasAddressTaken()) {
1740+
if (!HasEarlyExit && NextMBB.pred_size() == 1 &&
1741+
blockNeverFallThrough(*NextBBI) && !NextMBB.hasAddressTaken()) {
17211742
MergeBlocks(BBI, *NextBBI);
17221743
FalseBBDead = true;
17231744
} else {
@@ -2052,8 +2073,8 @@ bool IfConverter::IfConvertDiamond(BBInfo &BBI, IfcvtKind Kind,
20522073
BBI.BB->removeSuccessor(FalseBBI.BB, true);
20532074

20542075
BBInfo &TailBBI = BBAnalysis[TailBB->getNumber()];
2055-
bool CanMergeTail = !TailBBI.HasFallThrough &&
2056-
!TailBBI.BB->hasAddressTaken();
2076+
bool CanMergeTail =
2077+
blockNeverFallThrough(TailBBI) && !TailBBI.BB->hasAddressTaken();
20572078
// The if-converted block can still have a predicated terminator
20582079
// (e.g. a predicated return). If that is the case, we cannot merge
20592080
// it with the tail block.

‎llvm/test/CodeGen/ARM/ifcvt_unanalyzable_fallthrough.mir‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -21,30 +21,30 @@
2121
# As a consequence we could end up merging blocks at the end of a converted
2222
# diamond/triangle and while doing that we messed up when fixing up the CFG
2323
# related to fallthrough edges. For the test cases below we incorrectly ended
24-
# up ended up with a fallthrough from the MBBs with two Bcc instructions to
25-
# the MBB with the STRH after if conversion.
24+
# up with a fallthrough from the MBBs with two Bcc instructions to the MBB
25+
# with the STRH after if conversion.
2626
#
2727
---
2828
name: avoidMergeBlockDiamond
2929
body: |
3030
; CHECK-LABEL: name: avoidMergeBlockDiamond
3131
; CHECK: bb.0:
32-
; CHECK-NEXT: successors: %bb.2(0x80000000)
32+
; CHECK-NEXT: successors: %bb.1(0x80000000)
3333
; CHECK-NEXT: {{ $}}
3434
; CHECK-NEXT: $sp = tADDspi $sp, 2, 1 /* CC::ne */, $cpsr
3535
; CHECK-NEXT: $sp = tADDspi $sp, 1, 0 /* CC::eq */, $cpsr, implicit $sp
3636
; CHECK-NEXT: $sp = tADDspi $sp, 3, 14 /* CC::al */, $noreg
37-
; CHECK-NEXT: tBcc %bb.2, 1 /* CC::ne */, $cpsr
38-
; CHECK-NEXT: tBcc %bb.2, 1 /* CC::ne */, $cpsr
37+
; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
38+
; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
3939
; CHECK-NEXT: {{ $}}
4040
; CHECK-NEXT: bb.1:
41-
; CHECK-NEXT: successors: %bb.1(0x80000000)
42-
; CHECK-NEXT: {{ $}}
43-
; CHECK-NEXT: STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
44-
; CHECK-NEXT: tB %bb.1, 14 /* CC::al */, $noreg
41+
; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
4542
; CHECK-NEXT: {{ $}}
4643
; CHECK-NEXT: bb.2:
47-
; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
44+
; CHECK-NEXT: successors: %bb.2(0x80000000)
45+
; CHECK-NEXT: {{ $}}
46+
; CHECK-NEXT: STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
47+
; CHECK-NEXT: tB %bb.2, 14 /* CC::al */, $noreg
4848
bb.0:
4949
tBcc %bb.2, 1, $cpsr
5050
@@ -76,21 +76,21 @@ name: avoidMergeBlockTriangle
7676
body: |
7777
; CHECK-LABEL: name: avoidMergeBlockTriangle
7878
; CHECK: bb.0:
79-
; CHECK-NEXT: successors: %bb.2(0x80000000)
79+
; CHECK-NEXT: successors: %bb.1(0x80000000)
8080
; CHECK-NEXT: {{ $}}
8181
; CHECK-NEXT: $sp = tADDspi $sp, 1, 1 /* CC::ne */, $cpsr
8282
; CHECK-NEXT: $sp = tADDspi $sp, 2, 14 /* CC::al */, $noreg
83-
; CHECK-NEXT: tBcc %bb.2, 1 /* CC::ne */, $cpsr
84-
; CHECK-NEXT: tBcc %bb.2, 1 /* CC::ne */, $cpsr
83+
; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
84+
; CHECK-NEXT: tBcc %bb.1, 1 /* CC::ne */, $cpsr
8585
; CHECK-NEXT: {{ $}}
8686
; CHECK-NEXT: bb.1:
87-
; CHECK-NEXT: successors: %bb.1(0x80000000)
88-
; CHECK-NEXT: {{ $}}
89-
; CHECK-NEXT: STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
90-
; CHECK-NEXT: tB %bb.1, 14 /* CC::al */, $noreg
87+
; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
9188
; CHECK-NEXT: {{ $}}
9289
; CHECK-NEXT: bb.2:
93-
; CHECK-NEXT: tBX_RET 14 /* CC::al */, $noreg
90+
; CHECK-NEXT: successors: %bb.2(0x80000000)
91+
; CHECK-NEXT: {{ $}}
92+
; CHECK-NEXT: STRH $sp, $sp, $noreg, 0, 14 /* CC::al */, $noreg
93+
; CHECK-NEXT: tB %bb.2, 14 /* CC::al */, $noreg
9494
bb.0:
9595
tBcc %bb.1, 1, $cpsr
9696
tB %bb.3, 14, $noreg

0 commit comments

Comments
 (0)