[llvm] [BranchFolding] Kill common hoisted debug instructions (PR #140091)
Orlando Cazalet-Hyams via llvm-commits
llvm-commits at lists.llvm.org
Thu May 29 10:16:09 PDT 2025
https://github.com/OCHyams updated https://github.com/llvm/llvm-project/pull/140091
>From 7252b5a8186d1e7d0a2aa3e80d046ea342cffc11 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Thu, 15 May 2025 14:08:45 +0100
Subject: [PATCH 1/4] [BranchFolding] Kill common hoisted debug instructions
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.
---
llvm/lib/CodeGen/BranchFolding.cpp | 44 +++++++++++++++++--
llvm/test/DebugInfo/X86/branch-folder-dbg.mir | 34 ++++++++------
2 files changed, 60 insertions(+), 18 deletions(-)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index e0f7466ceacff..b4d48fdc9c260 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -25,6 +25,7 @@
#include "llvm/Analysis/ProfileSummaryInfo.h"
#include "llvm/CodeGen/Analysis.h"
#include "llvm/CodeGen/BranchFoldingPass.h"
+#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
#include "llvm/CodeGen/MBFIWrapper.h"
#include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
#include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
@@ -2076,19 +2077,53 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
if (TBB == FBB) {
MBB->splice(Loc, TBB, TBB->begin(), TIB);
} else {
+ // Merge the debug locations, and hoist and kill the debug instructions from
+ // both branches. FIXME: We could probably try harder to preserve some debug
+ // instructions (but at least this isn't producing wrong locations).
+ MachineIRBuilder MIRBuilder(*MBB, Loc);
+ auto HoistAndKillDbgInstr =
+ [&MIRBuilder](MachineBasicBlock::iterator DI,
+ MachineBasicBlock::iterator InsertBefore) {
+ assert(DI->isDebugInstr() && "Expected a debug instruction");
+ if (DI->isDebugRef()) {
+ MIRBuilder.setDebugLoc(DI->getDebugLoc());
+ MIRBuilder.buildDirectDbgValue(0, DI->getDebugVariable(),
+ DI->getDebugExpression());
+ return;
+ }
+
+ DI->setDebugValueUndef();
+ DI->moveBefore(&*InsertBefore);
+ };
+
// TIB and FIB point to the end of the regions to hoist/merge in TBB and
// FBB.
MachineBasicBlock::iterator FE = FIB;
MachineBasicBlock::iterator FI = FBB->begin();
for (MachineBasicBlock::iterator TI :
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 (and hoisted from BOTH blocks).
- if (TI->isDebugOrPseudoInstr()) {
+
+ // Hoist and kill debug instructions from FBB. Skip over pseudo
+ // instructions. After this loop FI points to the next non-debug
+ // instruction to hoist (checked in assert after the TBB debug
+ // instruction handling code).
+ while (FI->isDebugOrPseudoInstr()) {
+ assert(FI != FE && "Unexpected end of FBB range");
+ MachineBasicBlock::iterator FINext = std::next(FI);
+ HoistAndKillDbgInstr(FI, Loc);
+ FI = FINext;
+ }
+
+ // Move pseudo probes without modifying them.
+ if (TI->isPseudoProbe()) {
TI->moveBefore(&*Loc);
continue;
}
+ // Kill debug instructions before moving.
+ if (TI->isDebugInstr()) {
+ HoistAndKillDbgInstr(TI, Loc);
+ continue;
+ }
// Get the next non-meta instruction in FBB.
FI = skipDebugInstructionsForward(FI, FE, false);
@@ -2104,6 +2139,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
++FI;
}
}
+ assert(FIB->getParent() == FBB && "Unexpectedly moved FIB?");
FBB->erase(FBB->begin(), FIB);
if (UpdateLiveIns)
diff --git a/llvm/test/DebugInfo/X86/branch-folder-dbg.mir b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
index 5c38fd2a43829..831b263fdccd4 100644
--- a/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
+++ b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
@@ -1,16 +1,21 @@
# RUN: llc %s --start-before=branch-folder --stop-after=branch-folder -o - | FileCheck %s
## Check that common instructions hoisted from `if.then` and `if.else` into
-## common pred `entry` get merged debug locations.
-
-## FIXME: The debug instructions handling here is wrong.
+## common pred `entry` get merged debug locations. The debug instructions from
+## both branches should get hoisted and killed.
+##
+## The MIR debug instructions have been modified by hand in order to check they
+## can be killed.
# CHECK: bb.0
# CHECK: CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
-## --- Start splice from bb.2.if.else ---
-# CHECK-NEXT: DBG_VALUE 2, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
-# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-location !DILocation(line: 0, scope: ![[#]])
-## --- End splice --------------
+## --- Start splice from bb.2.if.else (and debug instructions from bb.1.if.then) ---
+# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
+# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(), debug-location ![[#]]
+# CHECK-NEXT: $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !DILocation(line: 0, scope: ![[#]])
+# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0), debug-location ![[#]]
+# CHECK-NEXT: DBG_VALUE $noreg, $noreg, ![[#]], !DIExpression(DW_OP_LLVM_arg, 0), debug-location ![[#]]
+## --- End splice ------------------------------------------------------------------
# CHECK-NEXT: TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
# CHECK-NEXT: JCC_1 %bb.2, 9, implicit killed $eflags
# CHECK: bb.1
@@ -73,6 +78,8 @@
...
---
name: g
+tracksRegLiveness: true
+isSSA: false
body: |
bb.0 (%ir-block.0):
successors: %bb.1(0x40000000), %bb.2(0x40000000)
@@ -86,22 +93,21 @@ body: |
bb.1.if.then:
successors: %bb.3(0x80000000)
-
- DBG_VALUE 0, $noreg, !11, !DIExpression(), debug-location !13
- $edi = MOV32r0 implicit-def dead $eflags, debug-location !14
+ DBG_VALUE $esi, $noreg, !11, !DIExpression(), debug-location !13
+ $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 1, debug-location !14
+ DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !13
$esi = MOV32r0 implicit-def dead $eflags, debug-location !14
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
- DBG_VALUE 1, $noreg, !11, !DIExpression(), debug-location !13
JMP_1 %bb.3, debug-location !15
bb.2.if.else:
successors: %bb.3(0x80000000)
- DBG_VALUE 2, $noreg, !11, !DIExpression(), debug-location !13
- $edi = MOV32r0 implicit-def dead $eflags, debug-location !16
+ DBG_VALUE $esp, $noreg, !11, !DIExpression(), debug-location !13
+ $edi = MOV32r0 implicit-def dead $eflags, debug-instr-number 2, debug-location !16
+ DBG_INSTR_REF !11, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(2, 0), debug-location !13
$esi = MOV32ri 1, debug-location !16
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
- DBG_VALUE 3, $noreg, !11, !DIExpression(), debug-location !13
bb.3.if.end:
$eax = MOV32ri 2
>From 5984744d00aaa5e244e8d977dabb21170434ed09 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Thu, 29 May 2025 18:12:03 +0100
Subject: [PATCH 2/4] replace MachineIRBuilder with BuildMI
---
llvm/lib/CodeGen/BranchFolding.cpp | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index b4d48fdc9c260..c2d583a8c5b17 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -25,7 +25,6 @@
#include "llvm/Analysis/ProfileSummaryInfo.h"
#include "llvm/CodeGen/Analysis.h"
#include "llvm/CodeGen/BranchFoldingPass.h"
-#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
#include "llvm/CodeGen/MBFIWrapper.h"
#include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
#include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
@@ -48,6 +47,8 @@
#include "llvm/IR/Function.h"
#include "llvm/InitializePasses.h"
#include "llvm/MC/LaneBitmask.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCInstrInfo.h"
#include "llvm/MC/MCRegisterInfo.h"
#include "llvm/Pass.h"
#include "llvm/Support/BlockFrequency.h"
@@ -2080,15 +2081,18 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
// Merge the debug locations, and hoist and kill the debug instructions from
// both branches. FIXME: We could probably try harder to preserve some debug
// instructions (but at least this isn't producing wrong locations).
- MachineIRBuilder MIRBuilder(*MBB, Loc);
+ MachineInstrBuilder MIRBuilder(*MBB->getParent(), Loc);
auto HoistAndKillDbgInstr =
- [&MIRBuilder](MachineBasicBlock::iterator DI,
- MachineBasicBlock::iterator InsertBefore) {
+ [MBB](MachineBasicBlock::iterator DI,
+ MachineBasicBlock::iterator InsertBefore) {
assert(DI->isDebugInstr() && "Expected a debug instruction");
if (DI->isDebugRef()) {
- MIRBuilder.setDebugLoc(DI->getDebugLoc());
- MIRBuilder.buildDirectDbgValue(0, DI->getDebugVariable(),
- DI->getDebugExpression());
+ const TargetInstrInfo *TII =
+ MBB->getParent()->getSubtarget().getInstrInfo();
+ const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
+ DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
+ DI->getDebugVariable(), DI->getDebugExpression());
+ MBB->insert(InsertBefore, &*DI);
return;
}
>From c7d4b4507c7e82dd559478817d9cd6cbcbbb2c6b Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Thu, 29 May 2025 18:13:50 +0100
Subject: [PATCH 3/4] use capture instead of param
---
llvm/lib/CodeGen/BranchFolding.cpp | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index c2d583a8c5b17..d8658405e3601 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2083,8 +2083,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
// instructions (but at least this isn't producing wrong locations).
MachineInstrBuilder MIRBuilder(*MBB->getParent(), Loc);
auto HoistAndKillDbgInstr =
- [MBB](MachineBasicBlock::iterator DI,
- MachineBasicBlock::iterator InsertBefore) {
+ [MBB, Loc](MachineBasicBlock::iterator DI) {
assert(DI->isDebugInstr() && "Expected a debug instruction");
if (DI->isDebugRef()) {
const TargetInstrInfo *TII =
@@ -2092,12 +2091,12 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
DI->getDebugVariable(), DI->getDebugExpression());
- MBB->insert(InsertBefore, &*DI);
+ MBB->insert(Loc, &*DI);
return;
}
DI->setDebugValueUndef();
- DI->moveBefore(&*InsertBefore);
+ DI->moveBefore(&*Loc);
};
// TIB and FIB point to the end of the regions to hoist/merge in TBB and
@@ -2114,7 +2113,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
while (FI->isDebugOrPseudoInstr()) {
assert(FI != FE && "Unexpected end of FBB range");
MachineBasicBlock::iterator FINext = std::next(FI);
- HoistAndKillDbgInstr(FI, Loc);
+ HoistAndKillDbgInstr(FI);
FI = FINext;
}
@@ -2125,7 +2124,7 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
}
// Kill debug instructions before moving.
if (TI->isDebugInstr()) {
- HoistAndKillDbgInstr(TI, Loc);
+ HoistAndKillDbgInstr(TI);
continue;
}
>From 58676b6ca2451daf0864421b36ef88821ed86522 Mon Sep 17 00:00:00 2001
From: Orlando Cazalet-Hyams <orlando.hyams at sony.com>
Date: Thu, 29 May 2025 18:15:58 +0100
Subject: [PATCH 4/4] format
---
llvm/lib/CodeGen/BranchFolding.cpp | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index d8658405e3601..c5eb03e19f493 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2082,22 +2082,21 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
// both branches. FIXME: We could probably try harder to preserve some debug
// instructions (but at least this isn't producing wrong locations).
MachineInstrBuilder MIRBuilder(*MBB->getParent(), Loc);
- auto HoistAndKillDbgInstr =
- [MBB, Loc](MachineBasicBlock::iterator DI) {
- assert(DI->isDebugInstr() && "Expected a debug instruction");
- if (DI->isDebugRef()) {
- const TargetInstrInfo *TII =
- MBB->getParent()->getSubtarget().getInstrInfo();
- const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
- DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
- DI->getDebugVariable(), DI->getDebugExpression());
- MBB->insert(Loc, &*DI);
- return;
- }
+ auto HoistAndKillDbgInstr = [MBB, Loc](MachineBasicBlock::iterator DI) {
+ assert(DI->isDebugInstr() && "Expected a debug instruction");
+ if (DI->isDebugRef()) {
+ const TargetInstrInfo *TII =
+ MBB->getParent()->getSubtarget().getInstrInfo();
+ const MCInstrDesc &DBGV = TII->get(TargetOpcode::DBG_VALUE);
+ DI = BuildMI(*MBB->getParent(), DI->getDebugLoc(), DBGV, false, 0,
+ DI->getDebugVariable(), DI->getDebugExpression());
+ MBB->insert(Loc, &*DI);
+ return;
+ }
- DI->setDebugValueUndef();
- DI->moveBefore(&*Loc);
- };
+ DI->setDebugValueUndef();
+ DI->moveBefore(&*Loc);
+ };
// TIB and FIB point to the end of the regions to hoist/merge in TBB and
// FBB.
More information about the llvm-commits
mailing list