[InstrRef] Skip clobbered EntryValue register recovery #142478
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Shubham Sandeep Rastogi (rastogishubham) ChangesThis changes the final stage of InstrRef, i.e. the TransferTracker (which combines the values locations with the variable values), so that it treats a DEBUG_VALUE of an EntryValue just like a DEBUG_VALUE of a constant: a location that is never clobbered and can be propagated to subsequent BBs as long as no other DEBUG_VALUE intrinsics updated the variable. We add two tests here:
This patch is trying to reland #77938 but also fixes the bug with InstrRef based LiveDebugValues, where entry values were not being propagated in a diamond-shaped CFG. Full diff: https://github.com/llvm/llvm-project/pull/142478.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index fdf50188fbcd8..85ecfebb0cf68 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -668,6 +668,16 @@ class TransferTracker {
auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
+ // If the expression is a DW_OP_entry_value, emit the variable location
+ // as-is.
+ if (DIExpr->isEntryValue()) {
+ Register Reg = MTracker->LocIdxToLocID[Num.getLoc()];
+ MachineOperand MO = MachineOperand::CreateReg(Reg, false);
+ PendingDbgValues.push_back(std::make_pair(
+ VarID, &*emitMOLoc(MO, Var, {DIExpr, Prop.Indirect, false})));
+ return true;
+ }
+
// Is the variable appropriate for entry values (i.e., is a parameter).
if (!isEntryValueVariable(Var, DIExpr))
return false;
@@ -694,7 +704,7 @@ class TransferTracker {
DebugVariableID VarID = DVMap.getDVID(Var);
// Ignore non-register locations, we don't transfer those.
- if (MI.isUndefDebugValue() ||
+ if (MI.isUndefDebugValue() || MI.getDebugExpression()->isEntryValue() ||
all_of(MI.debug_operands(),
[](const MachineOperand &MO) { return !MO.isReg(); })) {
auto It = ActiveVLocs.find(VarID);
diff --git a/llvm/test/DebugInfo/MIR/InstrRef/entry_value_clobbered_stack_copy.mir b/llvm/test/DebugInfo/MIR/InstrRef/entry_value_clobbered_stack_copy.mir
new file mode 100644
index 0000000000000..3461c40f5ad8c
--- /dev/null
+++ b/llvm/test/DebugInfo/MIR/InstrRef/entry_value_clobbered_stack_copy.mir
@@ -0,0 +1,50 @@
+# RUN: llc --run-pass=livedebugvalues -o - %s | FileCheck %s
+# REQUIRES: x86-registered-target
+
+--- |
+ target triple = "x86_64-"
+ define void @foo(ptr swiftasync %0) !dbg !4 {
+ call void @llvm.dbg.value(metadata ptr %0, metadata !9, metadata !DIExpression(DW_OP_LLVM_entry_value, 1)), !dbg !17
+ ret void
+ }
+ declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+ !llvm.module.flags = !{!0}
+ !llvm.dbg.cu = !{!1}
+
+ !0 = !{i32 2, !"Debug Info Version", i32 3}
+ !1 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !2, producer: "blah", isOptimized: true, flags: "blah", runtimeVersion: 5, emissionKind: FullDebug)
+ !2 = !DIFile(filename: "blah", directory: "blah")
+ !3 = !{}
+ !4 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !2, file: !2, line: 284, type: !7, unit: !1)
+ !7 = !DISubroutineType(types: !3)
+ !9 = !DILocalVariable(name: "self", arg: 3, scope: !4, file: !2, line: 328, type: !12, flags: DIFlagArtificial)
+ !12 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !2, file: !2, size: 64, elements: !3)
+ !17 = !DILocation(line: 328, column: 17, scope: !4)
+
+...
+---
+name: foo
+alignment: 16
+debugInstrRef: true
+tracksDebugUserValues: true
+liveins:
+ - { reg: '$r14', virtual-reg: '' }
+stack:
+ - { id: 0, name: '', type: spill-slot, offset: -64, size: 8, alignment: 8,
+ stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+ debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body: |
+ bb.0:
+ liveins: $r14
+ ; Put a copy of r14 on the stack.
+ MOV64mr $rbp, 1, $noreg, -48, $noreg, $r14 :: (store (s64) into %stack.0)
+ DBG_VALUE $r14, $noreg, !9, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location !17
+ MOV64mi32 $noreg, 1, $noreg, 0, $noreg, 0, debug-location !17 :: (store (s64) into `ptr null`)
+ $r14 = MOV64rr killed $r13
+ ; Clobber $r14
+ RETI64 24
+# CHECK: bb.0:
+# CHECK: MOV64mr $rbp, 1, $noreg, -48, $noreg, $r14
+# CHECK-NEXT: DBG_VALUE $r14, {{.*}}, !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK-NOT: DBG_VALUE
\ No newline at end of file
diff --git a/llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir b/llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir
new file mode 100644
index 0000000000000..292aa2d9c0a30
--- /dev/null
+++ b/llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir
@@ -0,0 +1,91 @@
+# RUN: llc --run-pass=livedebugvalues -o - %s | FileCheck %s
+# REQUIRES: x86-registered-target
+--- |
+ target triple = "x86_64-"
+
+ define i32 @baz(i32 swiftasync %arg1, i32 noundef %arg2, i1 %cond) !dbg !9 {
+ tail call void @llvm.dbg.value(metadata i32 %arg1, metadata !17, metadata !DIExpression(DW_OP_LLVM_entry_value, 1)), !dbg !19
+ br i1 %cond, label %if.then, label %if.else, !dbg !22
+ if.then:
+ %call = call i32 @foo(i32 noundef %arg1), !dbg !23
+ br label %if.end, !dbg !25
+ if.else:
+ %call1 = call i32 @foo(i32 noundef %arg2), !dbg !26
+ br label %if.end
+ if.end:
+ %temp.0 = phi i32 [ %call, %if.then ], [ %call1, %if.else ], !dbg !28
+ ret i32 %temp.0, !dbg !29
+ }
+
+ declare i32 @foo(i32)
+ declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+ !llvm.dbg.cu = !{!0}
+ !llvm.module.flags = !{!2, !3}
+
+ !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "ha", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+ !1 = !DIFile(filename: "test.c", directory: "hah")
+ !2 = !{i32 7, !"Dwarf Version", i32 4}
+ !3 = !{i32 2, !"Debug Info Version", i32 3}
+ !9 = distinct !DISubprogram(name: "baz", scope: !1, file: !1, line: 3, type: !10, scopeLine: 3, unit: !0, retainedNodes: !13)
+ !10 = !DISubroutineType(types: !11)
+ !11 = !{!12, !12, !12, !12}
+ !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+ !13 = !{!14, !15, !16, !17}
+ !14 = !DILocalVariable(name: "arg1", arg: 1, scope: !9, file: !1, line: 3, type: !12)
+ !15 = !DILocalVariable(name: "arg2", arg: 2, scope: !9, file: !1, line: 3, type: !12)
+ !16 = !DILocalVariable(name: "cond", arg: 3, scope: !9, file: !1, line: 3, type: !12)
+ !17 = !DILocalVariable(name: "local", scope: !9, file: !1, line: 4, type: !12)
+ !19 = !DILocation(line: 0, scope: !9)
+ !20 = !DILocation(line: 7, column: 7, scope: !21)
+ !21 = distinct !DILexicalBlock(scope: !9, file: !1, line: 7, column: 7)
+ !22 = !DILocation(line: 7, column: 7, scope: !9)
+ !23 = !DILocation(line: 8, column: 12, scope: !24)
+ !24 = distinct !DILexicalBlock(scope: !21, file: !1, line: 7, column: 13)
+ !25 = !DILocation(line: 9, column: 3, scope: !24)
+ !26 = !DILocation(line: 10, column: 12, scope: !27)
+ !27 = distinct !DILexicalBlock(scope: !21, file: !1, line: 9, column: 10)
+ !28 = !DILocation(line: 0, scope: !21)
+ !29 = !DILocation(line: 13, column: 3, scope: !9)
+
+...
+---
+name: baz
+alignment: 16
+debugInstrRef: true
+tracksDebugUserValues: true
+liveins:
+ - { reg: '$r14', virtual-reg: '' }
+ - { reg: '$edi', virtual-reg: '' }
+ - { reg: '$esi', virtual-reg: '' }
+ - { reg: '$edx', virtual-reg: '' }
+body: |
+ bb.0:
+ successors: %bb.2(0x40000000), %bb.1(0x40000000)
+ liveins: $r14, $edi, $edx, $esi
+ DBG_VALUE $r14, $noreg, !14, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location !19
+ $r14 = MOV64ri 0, debug-location !20
+ CMP32ri killed renamable $edx, 0, implicit-def $eflags, debug-location !20
+ JCC_1 %bb.2, 4, implicit killed $eflags, debug-location !22
+ bb.1.if.then:
+ successors: %bb.3(0x80000000)
+ liveins: $edi, $r13
+ CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit-def $eax, debug-location !23
+ JMP_1 %bb.3, debug-location !25
+ bb.2.if.else:
+ successors: %bb.3(0x80000000)
+ liveins: $esi, $r13
+ $edi = MOV32rr killed $esi, debug-location !26
+ CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit-def $eax, debug-location !26
+ bb.3.if.end:
+ liveins: $eax
+ $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !29
+ RET64 implicit $eax, debug-location !29
+# CHECK-LABEL: bb.0:
+# CHECK: DBG_VALUE $r14, {{.*}}, !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK-LABEL: bb.1.if.then:
+# CHECK: DBG_VALUE $r14, {{.*}}, !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK-LABEL: bb.2.if.else:
+# CHECK: DBG_VALUE $r14, {{.*}}, !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK-LABEL: bb.3.if.end:
+# CHECK: DBG_VALUE $r14, {{.*}}, !DIExpression(DW_OP_LLVM_entry_value, 1)
\ No newline at end of file
|
There was a problem hiding this comment.
I've got some nits inline, otherwise this looks good.
I suppose this works slightly differently to how I envisaged in past PRs, where the entry-value-ness of a variable location would be transformed into the value-numbers used to propagate variable-values around the function. I guess the differences are:
- In my mental model, LiveDebugValues would always work out that a value is an entry-value from first principles and add DW_OP_entry_value, requiring us to produce an expression with DW_OP_entry_value removed from it somewhere,
- In this model, the variable value with entry-value expression gets propagated around the function. We then don't try to re-generate the entry-value-expression when the location gets clobbered, because it's already an entry value.
This is fine -- with the side-effect that I reckon entry-value variable locations will be re-stated in certain basic blocks with clobbers. Your adjustment to TransferTracker stops the variable location (with DW_OP_entry_value) from being tracked in the transfer tracker when an entry-value DBG_VALUE is encountered -- however at the start of each block TransferTracker::loadInLocs takes a collection of variable values and directly tracks them. You might need to filter out entry-value-expression variable values there too.
As an example, note line 73 in entry_value_gets_propagated.mir that I've marked. I reckon if you sink the clobber of 14 to that block:
- You'll have an entry-value DBG_VALUE propagated to bb.1 as expected,
- TransferTracker will track that variable as a normal live-location,
- When r14 is clobbered in bb1, TransferTracker will try to recover the variable location, and emit another entry-value DBG_VALUE after the clobber.
This isn't incorrect, and I reckon it won't affect the variable location output at all, but you might want to tighten it up by putting a filter in loadVarInLoc or similar. (My confidence level on this: about 93%, I could be wrong).
llvm/test/DebugInfo/MIR/InstrRef/entry_value_clobbered_stack_copy.mir
Outdated
Show resolved
Hide resolved
llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir
Outdated
Show resolved
Hide resolved
llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir
Outdated
Show resolved
Hide resolved
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
aaea50a to
0dbe947
Compare
|
@jmorse Thanks for the feedback and the detailed explanation. You are correct, if we move the clobber to line 73 as you mentioned, we do see two DBG_VALUEs. I think filtering becomes complicated. We would have to filter only in the case where there is a clobber to the same register as the entry_value somewhere in the basic block. However, we want to ensure we don't filter for other basic blocks that don't get affected by the clobber at all, such as in a diamond cfg. 
If we look at the screenshot above, if the basic block 2 has the clobber, then 2 and 4 will have to propagate the entry_value via recovering the DBG_VALUE because of a clobber, however, 3 should propagate the entry value in the standard case due to Now, we could just leave it as-is, but I don't like that either. So I went a different direction. If we just treat an entry_value as a constant, something like a What do you think? Note: I have the new patch as a second commit to make it easier to review, I will squash the commits before I land it |
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
0dbe947 to
22740a4
Compare
|
@jmorse Just pinging you again, can you please take a look at the patch again to make sure this way of handling entry-values looks good? |
|
Hmmm, I'm less of a fan of the most recent refinement -- for the base patch, I think it's reasonable to use an entry-value DIExpression as part of the variable-value being propagated around a function, as it's easily understandable. Putting a register-location into the set of constant-value operands is much more confusing and moderate hack, when the pass is designed to avoid tracking register-locations until the last moment. I don't believe a filter in loadInLocs would be so hard (I appreciate this is all pretty much undocumented). We can rely on two things:
Following the diamond-diagram, assuming we have a DBG_VALUE setting a variable to an entry-value expression in block 1, and no other DBG_VALUEs for the variable, we'll begin blocks 2, 3, and 4 with a
It's the tracking at the end of the 2nd item that can be filtered: I think it's just a case of guarding the insertion to Through all the blocks, |
|
Hi @jmorse Thanks for the explanation, however, I have two questions:
The |
For this I believe we can make use of the differences between LDV and how
Thus, there's actually no difference in DBG_* instructions we want created for entry-value locations in a block, regardless of whether there's a clobber or not. Looking closely at that block, without a clobber: If we start this block with an entry value, we'll create something like Then we'll start the block by creating the same DBG_VALUE with an entry-value expression. And when we get to the clobber of $r14, we know that The first thing TransferTracker will do on encountering the DBG_INSTR_REF is remove the previous location of the variable from its tracking maps. Regardless of whether the DBG_INSTR_REF is before or after the clobber. Thus: I think the only thing we need to do for an entry-value location at the start of the block is produce the initial DBG_VALUE for it, there's no need to enter it into the value tracking maps through the block.
Ah, right you are, we would need to not enter the information into both ActiveVLocs and ActiveMLocs. I think both those things are handled at the end of |
|
I suppose one of the awkward things about this area is how the entry-value flag affects different pieces of code -- in an ideal world we would represent entry-value locations in some /other/ way than "a reference to a register that isn't really a reference". But this is how it works today! |
@jmorse Maybe we're using slightly different terminology here, but to me an entry value is not something that can be clobbered. It's an abstract concept that refers to a the value a register has at the beginning of a function that is divorced from the register inside the function. To an entry value of r0, it doesn't matter that r0 gets clobbered, because it doesn't refer to the current value of r0. That's why I think it makes more sense to treat incoming entry values similar to constants. They are not affected by clobbers. |
|
I guess I can see how that works -- this is becoming complicated because we're trying to bridge a constant-like-location that lives outside of "value liveness" and similar concepts, with portions of the pass that really want to consider liveness. Hence there are several bits of code that stop working well together. I also suppose you might say I'm letting perfect be the enemy of good here, and it's better to have something that does work versus creating more complexity. Part of the problem is that I can put my finger exactly on what "perfect" (to me) looks like -- instr-ref already supports describing entry values and recovering variable-locations at the appropriate time, what's abnormal here is that the variable-locations entering LiveDebugValues already have DW_OP_LLVM_entry_value attached to them. I've cooked up a prototype here [0] that "decompiles" the entry-value flag out of the DIExpression and enters the entry-value-variable-location into the usual tracking processes. The result is here [1] (input file then output file), where the entry-value-location gets propagated around the block like any other variable. However, LiveDebugValues prefers to create references to the live value in a register if it's available, instead of everything being an entry-value location. Is having an entry-value expression in all the variable locations, right from the entry block, part of the requirements here? (I'm assuming this is something swift-specific as AFAIUI LLVM doesn't generate these expressions). If we can't take the "let LiveDebugValues generate entry-values itself" route, I suppose it's cleanest to fall back on the "treat entry-values like a constant" and admit their register-locations to the operand store, rather than push the complexity into other bit sof code. [0] jmorse@68635db |
One thing that is important to point out here is that there are different kinds of entry value that may need different treatment.
I believe that (1) should be treated like a constant and just remain undisturbed by LDV. This doesn't prevent us from doing something more clever with (2).
|
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
22740a4 to
3c27ab1
Compare
|
I got the patch working how Jeremy wanted. We now use the |
There was a problem hiding this comment.
I can see how we end up with two kinds of entry value (i.e. pre and post LDV) that need to be supported. I'd much prefer it if there wasn't, but it's got a clear use case.
With that in mind, I suppose it's less important to fixate on whether the locations are considered constants or not; either the current patch is fine or the original -- although for the original I think we'd need some more comments on what the operand-store might contain ("Register locations entered into the operand store are considered constant-entry-values" or something like that).
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
3c27ab1 to
60f7bc6
Compare
llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated_aarch64.mir
Outdated
Show resolved
Hide resolved
llvm/test/DebugInfo/MIR/InstrRef/entry_value_gets_propagated.mir
Outdated
Show resolved
Hide resolved
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
1db2ddc to
b238411
Compare
| @@ -0,0 +1,53 @@ | |||
| # RUN: llc --run-pass=livedebugvalues -o - %s | FileCheck %s | |||
| # REQUIRES: x86-registered-target | |||
There was a problem hiding this comment.
This one also needs to be moved.
There was a problem hiding this comment.
Oh that's my bad, this is a duplicate
| @@ -0,0 +1,98 @@ | |||
| # RUN: llc --run-pass=livedebugvalues -o - %s | FileCheck %s --implicit-check-not=DBG_VALUE | |||
| # REQUIRES: x86-registered-target | |||
|
|
|||
There was a problem hiding this comment.
This is also a duplicate
This changes the final stage of InstrRef, i.e. the TransferTracker (which combines the values locations with the variable values), so that it treats a DEBUG_VALUE of an EntryValue just like a DEBUG_VALUE of a constant: a location that is never clobbered and can be propagated to subsequent BBs as long as no other DEBUG_VALUE intrinsics updated the variable. We add two tests here: 1. `entry_value_clobbered_stack_copy` that saves a register on the stack, uses this register as an entry value DBG_VALUE location, and then clobbers it. Prior to this patch, this test would crash because we would try to describe a new location for the variable in terms of what was saved on the stack, and use an invalid expression to do so. This is not needed as an EntryValue can never be clobbered. 2. `entry_value_gets_propagated`, that tests that an EntryValue DBG_VALUE is propagated in a diamond-shaped CFG. This patch is trying to reland llvm#77938 but also fixes the bug with InstrRef based LiveDebugValues, where entry values were not being propagated in a diamond-shaped CFG.
rastogishubham
force-pushed
the
EntryValFixInstrRef
branch
from
b238411 to
74ecf6a
Compare
This changes the final stage of InstrRef, i.e. the TransferTracker (which combines the values locations with the variable values), so that it treats a DEBUG_VALUE of an EntryValue just like a DEBUG_VALUE of a constant: a location that is never clobbered and can be propagated to subsequent BBs as long as no other DEBUG_VALUE intrinsics updated the variable. We add two tests here: 1. `entry_value_clobbered_stack_copy` that saves a register on the stack, uses this register as an entry value DBG_VALUE location, and then clobbers it. Prior to this patch, this test would crash because we would try to describe a new location for the variable in terms of what was saved on the stack, and use an invalid expression to do so. This is not needed as an EntryValue can never be clobbered. 2. `entry_value_gets_propagated`, that tests that an EntryValue DBG_VALUE is propagated in a diamond-shaped CFG. This patch is trying to reland llvm#77938 but also fixes the bug with InstrRef based LiveDebugValues, where entry values were not being propagated in a diamond-shaped CFG. (cherry picked from commit 32ef4ce)
This changes the final stage of InstrRef, i.e. the TransferTracker (which combines the values locations with the variable values), so that it treats a DEBUG_VALUE of an EntryValue just like a DEBUG_VALUE of a constant: a location that is never clobbered and can be propagated to subsequent BBs as long as no other DEBUG_VALUE intrinsics updated the variable. We add two tests here: 1. `entry_value_clobbered_stack_copy` that saves a register on the stack, uses this register as an entry value DBG_VALUE location, and then clobbers it. Prior to this patch, this test would crash because we would try to describe a new location for the variable in terms of what was saved on the stack, and use an invalid expression to do so. This is not needed as an EntryValue can never be clobbered. 2. `entry_value_gets_propagated`, that tests that an EntryValue DBG_VALUE is propagated in a diamond-shaped CFG. This patch is trying to reland llvm#77938 but also fixes the bug with InstrRef based LiveDebugValues, where entry values were not being propagated in a diamond-shaped CFG. (cherry picked from commit 32ef4ce)
This changes the final stage of InstrRef, i.e. the TransferTracker (which combines the values locations with the variable values), so that it treats a DEBUG_VALUE of an EntryValue just like a DEBUG_VALUE of a constant: a location that is never clobbered and can be propagated to subsequent BBs as long as no other DEBUG_VALUE intrinsics updated the variable.
We add two tests here:
entry_value_clobbered_stack_copythat saves a register on the stack, uses this register as an entry value DBG_VALUE location, and then clobbers it. Prior to this patch, this test would crash because we would try to describe a new location for the variable in terms of what was saved on the stack, and use an invalid expression to do so. This is not needed as an EntryValue can never be clobbered.entry_value_gets_propagated, that tests that an EntryValue DBG_VALUE is propagated in a diamond-shaped CFG.This patch is trying to reland #77938 but also fixes the bug with InstrRef based LiveDebugValues, where entry values were not being propagated in a diamond-shaped CFG.