[BranchFolding] Fix assertion failure in HoistCommonCodeInSuccs #141028
Conversation
Assertion failure introduced in llvm#140063, which didn't account for TBB and FBB being the same block.
|
@llvm/pr-subscribers-backend-arm Author: Orlando Cazalet-Hyams (OCHyams) ChangesAssertion failure introduced in #140063, which didn't account for TBB and FBB being the same block. Full diff: https://github.com/llvm/llvm-project/pull/141028.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index d10e6722f4548..41e66bb82650e 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2072,9 +2072,11 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
return false;
// Hoist the instructions from [T.begin, TIB) and then delete [F.begin, FIB).
- // Merge the debug locations. FIXME: We should do something with the
- // debug instructions too (from BOTH branches).
- {
+ // If we're hoisting from a single block then just splice. Else step through
+ // and merge the debug locations.
+ if (TBB == FBB) {
+ MBB->splice(Loc, TBB, TBB->begin(), TIB);
+ } else {
// TIB and FIB point to the end of the regions to hoist/merge in TBB and
// FBB.
MachineBasicBlock::iterator FE = FIB;
@@ -2083,7 +2085,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
make_early_inc_range(make_range(TBB->begin(), TIB))) {
// Move debug instructions and pseudo probes without modifying them.
// FIXME: This is the wrong thing to do for debug locations, which
- // should at least be killed.
+ // should at least be killed (and hoisted from BOTH blocks).
if (TI->isDebugOrPseudoInstr()) {
TI->moveBefore(&*Loc);
continue;
diff --git a/llvm/test/CodeGen/ARM/branch-folder-single-bb-crash.mir b/llvm/test/CodeGen/ARM/branch-folder-single-bb-crash.mir
new file mode 100644
index 0000000000000..56bcd3d73d616
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/branch-folder-single-bb-crash.mir
@@ -0,0 +1,80 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=armv7-apple-ios --run-pass=branch-folder %s -o - | FileCheck %s
+
+## Don't crash in branch-folder's HoistCommonCodeInSuccs when TBB and FBB are
+## the same block.
+
+...
+---
+name: f_1418_2054
+tracksRegLiveness: true
+noPhis: true
+isSSA: false
+noVRegs: true
+body: |
+ ; CHECK-LABEL: name: f_1418_2054
+ ; CHECK: bb.0:
+ ; CHECK-NEXT: successors: %bb.1(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: $r0 = MOVr undef $r0, 14 /* CC::al */, $noreg, $noreg
+ ; CHECK-NEXT: Bcc %bb.1, 1 /* CC::ne */, undef $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; CHECK-NEXT: successors: %bb.3(0x40000000), %bb.4(0x40000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: $r0 = MOVr undef $r0, 14 /* CC::al */, $noreg, $noreg
+ ; CHECK-NEXT: Bcc %bb.4, 1 /* CC::ne */, undef $cpsr
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.3:
+ ; CHECK-NEXT: BX_RET 14 /* CC::al */, $noreg
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.4:
+ ; CHECK-NEXT: TRAP
+ bb.0:
+ successors: %bb.1
+
+ bb.1:
+ successors: %bb.3, %bb.2
+
+ Bcc %bb.3, 1, undef $cpsr
+ B %bb.2
+
+ bb.2:
+ successors: %bb.5, %bb.6
+
+ Bcc %bb.5, 1, undef $cpsr
+ B %bb.6
+
+ bb.3:
+ successors: %bb.4
+
+ $r0 = MOVr undef $r0, 14, $noreg, $noreg
+
+ bb.4:
+ successors: %bb.1
+
+ B %bb.1
+
+ bb.5:
+ successors: %bb.6
+
+ bb.6:
+ successors: %bb.8, %bb.7
+
+ $r0 = MOVr undef $r0, 14, $noreg, $noreg
+ $r0 = MOVr undef $r0, 14, $noreg, $noreg
+
+ Bcc %bb.7, 1, undef $cpsr
+ B %bb.8
+
+ bb.7:
+ successors:
+
+ TRAP
+
+ bb.8:
+ BX_RET 14, _
+...
|
There was a problem hiding this comment.
Looks good; however I'm not familiar with ARM assembly for the test, @mikaelholmen would you be able to confirm the output is alright?
I've no idea about ARM either, but I can confirm that the patch fixes the crash I saw for our downstream target. The ARM reproducer was just something I crafted from a minimal mir-reproducer for our target to have an in-tree reproducer for the crash. |
|
Also cannot interpret the ARM very well. I just grabbed the reproducer from @mikaelholmen :D I can try to find an x86 reproducer that we can all read if needed but the test is just there to detect a crash regression quickly more than anything. (Thanks again for the reproducer) |
There was a problem hiding this comment.
/me squints -- I think it's merging the load-to-r0 from bb.3 and bb.6 into bb.1, and leaving the second load-to-r0 in bb.6. It also simplifies away the self-loop into being a single-block-self-loop. This seems alright. It should be fine to land, but @OCHyams could you confirm that it produces the same output as before #140063 before landing?
| ## Don't crash in branch-folder's HoistCommonCodeInSuccs when TBB and FBB are | ||
| ## the same block. |
There was a problem hiding this comment.
I think this comment needs to be detached from the implementation details and made broader -- "We hoist code from successor blocks, and in abnormal circumstances we can have control flow that branches to the same block -- test that we handle this correctly". Or something.
Confirmed that the output is the same before as before #140063. For additional confidence, you can see the splice I've added here is just a copy+paste of the one removed in #140063. I've improved the test comment. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/22936 Here is the relevant piece of the build log for the reference |
Seems like a flaky test (prev and next are green) |
Assertion failure introduced in #140063, which didn't account for TBB and FBB being the same block.