[BranchFolding] Kill common hoisted debug instructions #140091
Merged
+59
−20
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-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesPlease ignore the first 2 commits - they're from #140063 branch-folder hoists common instructions from TBB and FBB into their This patch handles debug instructions - in the simplest way possible, which is There's possibly something we could do to preserve some variable locations in Note I had to replace the constant DBG_VALUEs to use registers in the test- it Full diff: https://github.com/llvm/llvm-project/pull/140091.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 6f5afbd2a996a..329c46de5e450 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"
@@ -2071,7 +2072,68 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
if (!HasDups)
return false;
- MBB->splice(Loc, TBB, TBB->begin(), TIB);
+ // Hoist the instructions from [T.begin, TIB) and then delete [F.begin, FIB).
+ // 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))) {
+
+ // 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;
+ }
+
+ assert(TI->isIdenticalTo(*FI, MachineInstr::CheckKillDead) &&
+ "Expected non-debug lockstep");
+
+ // Merge debug locs on hoisted instructions.
+ TI->setDebugLoc(
+ DILocation::getMergedLocation(TI->getDebugLoc(), FI->getDebugLoc()));
+ TI->moveBefore(&*Loc);
+ ++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
new file mode 100644
index 0000000000000..831b263fdccd4
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
@@ -0,0 +1,117 @@
+# 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. 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 (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
+
+--- |
+ 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"
+ target triple = "x86_64-unknown-linux-gnu"
+
+ declare dso_local noundef i64 @f() local_unnamed_addr
+
+ define dso_local noundef i32 @g() local_unnamed_addr !dbg !7 {
+ %call = tail call noundef i64 @f()
+ %cmp1 = icmp sgt i64 0, %call
+ %conv2 = trunc i64 0 to i32
+ br i1 %cmp1, label %if.then, label %if.else
+
+ if.then: ; preds = %0
+ #dbg_value(i64 0, !11, !DIExpression(), !13)
+ tail call void @_Z3fooii(i32 noundef %conv2, i32 noundef 0), !dbg !14
+ #dbg_value(i64 1, !11, !DIExpression(), !13)
+ br label %if.end, !dbg !15
+
+ if.else: ; preds = %0
+ #dbg_value(i64 2, !11, !DIExpression(), !13)
+ tail call void @_Z3barii(i32 noundef %conv2, i32 noundef 1), !dbg !16
+ #dbg_value(i64 3, !11, !DIExpression(), !13)
+ br label %if.end, !dbg !17
+
+ if.end: ; preds = %if.else, %if.then
+ ret i32 2
+ }
+
+ declare void @_Z3fooii(i32 noundef, i32 noundef) local_unnamed_addr
+
+ declare void @_Z3barii(i32 noundef, i32 noundef) local_unnamed_addr
+
+ !llvm.module.flags = !{!0, !1}
+ !llvm.ident = !{!2}
+ !llvm.dbg.cu = !{!3}
+ !llvm.debugify = !{!5, !6}
+
+ !0 = !{i32 7, !"Dwarf Version", i32 5}
+ !1 = !{i32 2, !"Debug Info Version", i32 3}
+ !2 = !{!"clang version 21.0.0"}
+ !3 = distinct !DICompileUnit(language: DW_LANG_C, file: !4, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+ !4 = !DIFile(filename: "test.nodbg.ll", directory: "/")
+ !5 = !{i32 15}
+ !6 = !{i32 7}
+ !7 = distinct !DISubprogram(name: "g", linkageName: "g", scope: null, file: !4, line: 1, type: !8, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !3, retainedNodes: !10)
+ !8 = !DISubroutineType(types: !9)
+ !9 = !{}
+ !10 = !{!11}
+ !11 = !DILocalVariable(name: "1", scope: !7, file: !4, line: 3, type: !12)
+ !12 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+ !13 = !DILocation(line: 3, column: 1, scope: !7)
+ !14 = !DILocation(line: 9, column: 1, scope: !7)
+ !15 = !DILocation(line: 10, column: 1, scope: !7)
+ !16 = !DILocation(line: 11, column: 1, scope: !7)
+ !17 = !DILocation(line: 12, column: 1, scope: !7)
+...
+---
+name: g
+tracksRegLiveness: true
+isSSA: false
+body: |
+ bb.0 (%ir-block.0):
+ successors: %bb.1(0x40000000), %bb.2(0x40000000)
+
+ frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
+ frame-setup CFI_INSTRUCTION def_cfa_offset 16
+ CALL64pcrel32 @f, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
+ TEST64rr killed renamable $rax, renamable $rax, implicit-def $eflags
+ JCC_1 %bb.2, 9, implicit killed $eflags
+ JMP_1 %bb.1
+
+ bb.1.if.then:
+ successors: %bb.3(0x80000000)
+ 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
+ JMP_1 %bb.3, debug-location !15
+
+ bb.2.if.else:
+ successors: %bb.3(0x80000000)
+
+ 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
+
+ bb.3.if.end:
+ $eax = MOV32ri 2
+ $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+ frame-destroy CFI_INSTRUCTION def_cfa_offset 8
+ RET 0, $eax
+...
diff --git a/obj b/obj
new file mode 100644
index 0000000000000..6fd99f7766296
Binary files /dev/null and b/obj differ
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SLTozer
reviewed
May 21, 2025
llvm/lib/CodeGen/BranchFolding.cpp
Outdated
| @@ -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" | |||
There was a problem hiding this comment.
Use of a GlobalISel class here - the post-ISel choice I think should be MachineInstrBuilder.h
OCHyams
force-pushed
the
branch-fold-dbg-instr
branch
from
7d9e272 to
c7d4b45
Compare
|
Re-opened and ready for review |
OCHyams
force-pushed
the
branch-fold-dbg-instr
branch
from
58676b6 to
82c3f60
Compare
|
Rebased, ping |
jmorse
reviewed
Jul 18, 2025
llvm/lib/CodeGen/BranchFolding.cpp
Outdated
| // 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()) { |
There was a problem hiding this comment.
HoistAndKillDbgInst asserts that only debug-instrs are fed into it, should this condition should be downgraded?
There was a problem hiding this comment.
Ah good point, and it turns out the range of instructions is never going to include a probe. The foldable-range-finding-loop above bails if it sees a probe because they're not skipped over like debug instructions and then the isSafeToMove check returns false for them.
Updated.
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 llvm#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.
OCHyams
force-pushed
the
branch-fold-dbg-instr
branch
from
cbedb86 to
98f22a5
Compare
|
Rebased, re-running premerge CI |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/23775 Here is the relevant piece of the build log for the reference |
Not exactly sure what this bot is testing, or how my change is interacting with it. It looks like it goes green a few builds after this though, so possibly just flaky. |
|
this causes |
OCHyams
added a commit
that referenced
this pull request
Jul 21, 2025
…)" This reverts commit 8ba341e.
OCHyams
added a commit
that referenced
this pull request
Jul 21, 2025
) Reverts #140091 due to crash (see comments for reproducer)
|
Thanks for the reproducer. I can't look just now so I've reverted (29af8e5) in the mean time & will fix and re-land tomorrow. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 21, 2025
…ions" (#149845) Reverts llvm/llvm-project#140091 due to crash (see comments for reproducer)
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.
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.