[BranchFolding] Merge debug locs on common hoisted code #140063
Merged
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) Changesbranch-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. The merged instructions should get merged debug locations for debugging and PGO purposes, which is handled in this patch. Debug instructions also need to be handled differently. That'll come in another patch. This issue was found by @omern1. I'm not sure how prevalent this pattern is in the compiler (splicing to merge instructions) - imo it's worth looking into further to see if there's a holistic fix we can introduce (e.g., add a new api for this case), but for now I think fixing up this case by itself is still worth it. Full diff: https://github.com/llvm/llvm-project/pull/140063.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index 6f5afbd2a996a..7dc87dbabf400 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2071,7 +2071,34 @@ 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. FIXME: We should do something with the
+ // debug instructions too (from BOTH branches).
+ {
+ // 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.
+ if (TI->isDebugOrPseudoInstr()) {
+ TI->moveBefore(&*Loc);
+ continue;
+ }
+
+ // Get the next non-meta instruction in FBB.
+ FI = skipDebugInstructionsForward(FI, FE, false);
+ assert(FI != FE && "Expected non-debug lockstep");
+
+ // Merge debug locs on hoisted instructions.
+ TI->setDebugLoc(
+ DILocation::getMergedLocation(TI->getDebugLoc(), FI->getDebugLoc()));
+ TI->moveBefore(&*Loc);
+ }
+ }
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..5c38fd2a43829
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/branch-folder-dbg.mir
@@ -0,0 +1,111 @@
+# 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.
+
+# 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 --------------
+# 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
+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 0, $noreg, !11, !DIExpression(), debug-location !13
+ $edi = MOV32r0 implicit-def dead $eflags, debug-location !14
+ $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
+ $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
+ $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+ frame-destroy CFI_INSTRUCTION def_cfa_offset 8
+ RET 0, $eax
+...
|
jmorse
approved these changes
May 15, 2025
Covered by existings tests e.g. test/CodeGen/ARM/aes-erratum-fix.ll
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this pull request
May 15, 2025
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.
… modifying some kill markers after its own isIdenticalTo checks
|
The pre-merge check failure |
|
Hi @OCHyams I started seeing crashes with this patch for my downstream target. I managed to reduce and then reproduce it for ARM with It crashes with |
I think the problem happens when we come to BranchFolder::HoistCommonCodeInSuccs and TBB and FBB are the same block and we still go ahead and hoist and then after FI suddenly points to an instruction in MBB. |
|
Thanks for the reproducer and notes - on it |
OCHyams
added a commit
to OCHyams/llvm-project
that referenced
this pull request
May 22, 2025
Assertion failure introduced in llvm#140063, which didn't account for TBB and FBB being the same block.
OCHyams
added a commit
that referenced
this pull request
May 22, 2025
) Assertion failure introduced in #140063, which didn't account for TBB and FBB being the same block.
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. The merged instructions should get merged debug locations for debugging and PGO purposes, which is handled in this patch. Debug instructions also need to be handled differently. That'll come in another patch.
This issue was found by @omern1. I'm not sure how prevalent this pattern is in the compiler (splicing to merge instructions) - imo it's worth looking into further to see if there's a holistic fix we can introduce (e.g., add a new api for this case), but for now I think fixing up this case by itself is still worth it.