Skip to content

Commit fbf6271

Browse files
OCHyamsOCHyams
committed
Reapply (2) [BranchFolding] Kill common hoisted debug instructions (#149999)
Reapply #140091. branch-folder hoists common instructions from TBB and FBB into their pred. Without this patch it achieves this by splicing the instructions from TBB and deleting the common ones in FBB. That moves the debug locations and debug instructions from TBB into the pred without modification, which is not ideal. Debug locations are handled in #140063. This patch handles debug instructions - in the simplest way possible, which is to just kill (undef) them. We kill and hoist the ones in FBB as well as TBB because otherwise the fact there's an assignment on the code path is deleted (which might lead to a prior location extending further than it should). There's possibly something we could do to preserve some variable locations in some cases, but this is the easiest not-incorrect thing to do. Note I had to replace the constant DBG_VALUEs to use registers in the test- it turns out setDebugValueUndef doesn't undef constant DBG_VALUEs... which feels wrong to me, but isn't something I want to touch right now. --- Fix end-iterator-dereference and add test.
1 parent d26ca8b commit fbf6271

File tree

3 files changed

+174
-21
lines changed

3 files changed

+174
-21
lines changed

‎llvm/lib/CodeGen/BranchFolding.cpp

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2083,22 +2083,54 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
20832083
if (TBB == FBB) {
20842084
MBB->splice(Loc, TBB, TBB->begin(), TIB);
20852085
} else {
2086+
// Merge the debug locations, and hoist and kill the debug instructions from
2087+
// both branches. FIXME: We could probably try harder to preserve some debug
2088+
// instructions (but at least this isn't producing wrong locations).
2089+
MachineInstrBuilder MIRBuilder(*MBB->getParent(), Loc);
2090+
auto HoistAndKillDbgInstr = [MBB, Loc](MachineBasicBlock::iterator DI) {
2091+
assert(DI->isDebugInstr() && "Expected a debug instruction");
2092+
if (DI->isDebugRef()) {
2093+
const TargetInstrInfo *TII =
2094+
MBB->getParent()->getSubtarget().getInstrInfo();
2095+
const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
2096+
DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
2097+
DI->getDebugVariable(), DI->getDebugExpression());
2098+
MBB->insert(Loc, &*DI);
2099+
return;
2100+
}
2101+
// Deleting a DBG_PHI results in an undef at the referenced DBG_INSTR_REF.
2102+
if (DI->isDebugPHI()) {
2103+
DI->eraseFromParent();
2104+
return;
2105+
}
2106+
2107+
DI->setDebugValueUndef();
2108+
DI->moveBefore(&*Loc);
2109+
};
2110+
20862111
// TIB and FIB point to the end of the regions to hoist/merge in TBB and
20872112
// FBB.
20882113
MachineBasicBlock::iterator FE = FIB;
20892114
MachineBasicBlock::iterator FI = FBB->begin();
20902115
for (MachineBasicBlock::iterator TI :
20912116
make_early_inc_range(make_range(TBB->begin(), TIB))) {
2092-
// Move debug instructions and pseudo probes without modifying them.
2093-
// FIXME: This is the wrong thing to do for debug locations, which
2094-
// should at least be killed (and hoisted from BOTH blocks).
2095-
if (TI->isDebugOrPseudoInstr()) {
2096-
TI->moveBefore(&*Loc);
2117+
// Hoist and kill debug instructions from FBB. After this loop FI points
2118+
// to the next non-debug instruction to hoist (checked in assert after the
2119+
// TBB debug instruction handling code).
2120+
while (FI != FE && FI->isDebugInstr())
2121+
HoistAndKillDbgInstr(FI++);
2122+
2123+
// Kill debug instructions before moving.
2124+
if (TI->isDebugInstr()) {
2125+
HoistAndKillDbgInstr(TI);
20972126
continue;
20982127
}
20992128

2100-
// Get the next non-meta instruction in FBB.
2101-
FI = skipDebugInstructionsForward(FI, FE, false);
2129+
// FI and TI now point to identical non-debug instructions.
2130+
assert(FI != FE && "Unexpected end of FBB range");
2131+
// Pseudo probes are excluded from the range when identifying foldable
2132+
// instructions, so we don't expect to see one now.
2133+
assert(!TI->isPseudoProbe() && "Unexpected pseudo probe in range");
21022134
// NOTE: The loop above checks CheckKillDead but we can't do that here as
21032135
// it modifies some kill markers after the check.
21042136
assert(TI->isIdenticalTo(*FI, MachineInstr::CheckDefs) &&
@@ -2111,6 +2143,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
21112143
++FI;
21122144
}
21132145
}
2146+
21142147
FBB->erase(FBB->begin(), FIB);
21152148

21162149
if (UpdateLiveIns)

‎llvm/test/DebugInfo/X86/branch-folder-dbg-after-end.mir

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
# RUN: llc %s --start-before=branch-folder --stop-after=branch-folder -o - \
2+
# RUN: | FileCheck %s --implicit-check-not=DBG_PHI
3+
4+
## Common instructions are hoisted. Check that trailing debug instructions in
5+
## the range are also hoisted, and don't cause a crash.
6+
##
7+
## Note the MIR doesn't match the IR as it's modified from:
8+
## /home/och/dev/llvm-project/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
9+
10+
# CHECK: bb.0
11+
# CHECK: CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
12+
## --- Start splice from bb.2.if.else (and debug instructions from bb.1.if.then) ---
13+
# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-location !DILocation(line: 0, scope: ![[#]])
14+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location
15+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location
16+
## --- End splice ------------------------------------------------------------------
17+
# CHECK-NEXT: TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
18+
# CHECK-NEXT: JCC_1 %bb.2, 8, implicit $eflags
19+
# CHECK: bb.1
20+
21+
--- |
22+
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
23+
target triple = "x86_64-unknown-linux-gnu"
24+
25+
declare dso_local noundef i64 @f() local_unnamed_addr
26+
27+
define dso_local noundef i32 @g() local_unnamed_addr !dbg !7 {
28+
%call = tail call noundef i64 @f()
29+
%cmp1 = icmp sgt i64 0, %call
30+
%conv2 = trunc i64 0 to i32
31+
br i1 %cmp1, label %if.then, label %if.else
32+
33+
if.then: ; preds = %0
34+
tail call void @_Z3fooii(i32 noundef %conv2, i32 noundef 0), !dbg !14
35+
br label %if.end, !dbg !15
36+
37+
if.else: ; preds = %0
38+
tail call void @_Z3barii(i32 noundef %conv2, i32 noundef 1), !dbg !16
39+
br label %if.end, !dbg !17
40+
41+
if.end: ; preds = %if.else, %if.then
42+
ret i32 2
43+
}
44+
45+
declare void @_Z3fooii(i32 noundef, i32 noundef) local_unnamed_addr
46+
47+
declare void @_Z3barii(i32 noundef, i32 noundef) local_unnamed_addr
48+
49+
!llvm.module.flags = !{!0, !1}
50+
!llvm.ident = !{!2}
51+
!llvm.dbg.cu = !{!3}
52+
!llvm.debugify = !{!5, !6}
53+
54+
!0 = !{i32 7, !"Dwarf Version", i32 5}
55+
!1 = !{i32 2, !"Debug Info Version", i32 3}
56+
!2 = !{!"clang version 21.0.0"}
57+
!3 = distinct !DICompileUnit(language: DW_LANG_C, file: !4, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
58+
!4 = !DIFile(filename: "test.nodbg.ll", directory: "/")
59+
!5 = !{i32 15}
60+
!6 = !{i32 7}
61+
!7 = distinct !DISubprogram(name: "g", linkageName: "g", scope: null, file: !4, line: 1, type: !8, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !3, retainedNodes: !10)
62+
!8 = !DISubroutineType(types: !9)
63+
!9 = !{}
64+
!10 = !{!11}
65+
!11 = !DILocalVariable(name: "1", scope: !7, file: !4, line: 3, type: !12)
66+
!12 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
67+
!13 = !DILocation(line: 3, column: 1, scope: !7)
68+
!14 = !DILocation(line: 9, column: 1, scope: !7)
69+
!15 = !DILocation(line: 10, column: 1, scope: !7)
70+
!16 = !DILocation(line: 11, column: 1, scope: !7)
71+
!17 = !DILocation(line: 12, column: 1, scope: !7)
72+
...
73+
---
74+
name: g
75+
tracksRegLiveness: true
76+
isSSA: false
77+
body: |
78+
bb.0 (%ir-block.0):
79+
successors: %bb.1(0x40000000), %bb.2(0x40000000)
80+
81+
frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
82+
frame-setup CFI_INSTRUCTION def_cfa_offset 16
83+
CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
84+
TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
85+
JCC_1 %bb.2, 9, implicit killed $eflags
86+
JMP_1 %bb.1
87+
88+
bb.1.if.then:
89+
successors: %bb.3(0x80000000)
90+
91+
$edi = MOV32r0 implicit-def dead $eflags, debug-location !14
92+
DBG_VALUE $edi, $noreg, !11, !DIExpression(), debug-location !13
93+
94+
bb.3.if.end:
95+
$eax = MOV32ri 2
96+
$rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
97+
frame-destroy CFI_INSTRUCTION def_cfa_offset 8
98+
RET 0, $eax
99+
100+
bb.2.if.else:
101+
successors: %bb.3(0x80000000)
102+
103+
$edi = MOV32r0 implicit-def dead $eflags, debug-location !16
104+
DBG_VALUE $edi, $noreg, !11, !DIExpression(), debug-location !13
105+
CALL64pcrel32 target-flags(x86-plt) @_Z3barii, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit killed $edi, implicit-def $rsp, implicit-def $ssp, debug-location !16
106+
JMP_1 %bb.3, debug-location !15
107+
108+
...

‎llvm/test/DebugInfo/X86/branch-folder-dbg.mir

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,24 @@
1-
# RUN: llc %s --start-before=branch-folder --stop-after=branch-folder -o - | FileCheck %s
1+
# RUN: llc %s --start-before=branch-folder --stop-after=branch-folder -o - \
2+
# RUN: | FileCheck %s --implicit-check-not=DBG_PHI
23

34
## Check that common instructions hoisted from `if.then` and `if.else` into
4-
## common pred `entry` get merged debug locations.
5-
6-
## FIXME: The debug instructions handling here is wrong.
5+
## common pred `entry` get merged debug locations. The debug instructions from
6+
## both branches should get hoisted and killed.
7+
##
8+
## The MIR debug instructions have been modified by hand in order to check they
9+
## can be killed.
10+
##
11+
## Check DBG_PHIs are deleted rather than hoisted (implicit-check-not).
712

813
# CHECK: bb.0
914
# CHECK: CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
10-
## --- Start splice from bb.2.if.else ---
11-
# CHECK-NEXT: DBG_VALUE 2, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
12-
# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-location !DILocation(line: 0, scope: ![[#]])
13-
## --- End splice --------------
15+
## --- Start splice from bb.2.if.else (and debug instructions from bb.1.if.then) ---
16+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
17+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
18+
# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !DILocation(line: 0, scope: ![[#]])
19+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0), debug-location ![[#]]
20+
# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0), debug-location ![[#]]
21+
## --- End splice ------------------------------------------------------------------
1422
# CHECK-NEXT: TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
1523
# CHECK-NEXT: JCC_1 %bb.2, 9, implicit killed $eflags
1624
# CHECK: bb.1
@@ -73,6 +81,8 @@
7381
...
7482
---
7583
name: g
84+
tracksRegLiveness: true
85+
isSSA: false
7686
body: |
7787
bb.0 (%ir-block.0):
7888
successors: %bb.1(0x40000000), %bb.2(0x40000000)
@@ -87,21 +97,23 @@ body: |
8797
bb.1.if.then:
8898
successors: %bb.3(0x80000000)
8999
90-
DBG_VALUE 0, $noreg, !11, !DIExpression(), debug-location !13
91-
$edi = MOV32r0 implicit-def dead $eflags, debug-location !14
100+
DBG_PHI $esp, 3
101+
DBG_VALUE $esi, $noreg, !11, !DIExpression(), debug-location !13
102+
$edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 1, debug-location !14
103+
DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !13
92104
$esi = MOV32r0 implicit-def dead $eflags, debug-location !14
93105
CALL64pcrel32 target-flags(x86-plt) @_Z3fooii, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit killed $esi, implicit-def $rsp, implicit-def $ssp, debug-location !14
94-
DBG_VALUE 1, $noreg, !11, !DIExpression(), debug-location !13
95106
JMP_1 %bb.3, debug-location !15
96107
97108
bb.2.if.else:
98109
successors: %bb.3(0x80000000)
99110
100-
DBG_VALUE 2, $noreg, !11, !DIExpression(), debug-location !13
101-
$edi = MOV32r0 implicit-def dead $eflags, debug-location !16
111+
DBG_PHI $esp, 4
112+
DBG_VALUE $esp, $noreg, !11, !DIExpression(), debug-location !13
113+
$edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !16
114+
DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(2, 0), debug-location !13
102115
$esi = MOV32ri 1, debug-location !16
103116
CALL64pcrel32 target-flags(x86-plt) @_Z3barii, csr_64, implicit $rsp, implicit $ssp, implicit killed $edi, implicit killed $esi, implicit-def $rsp, implicit-def $ssp, debug-location !16
104-
DBG_VALUE 3, $noreg, !11, !DIExpression(), debug-location !13
105117
106118
bb.3.if.end:
107119
$eax = MOV32ri 2

0 commit comments

Comments
 (0)