[DebugInfo][SelectionDAG] Fix position of salvaged 'dangling' DBG_VALUEs #94458
Conversation
`SelectionDAGBuilder::handleDebugValue` has a parameter `Order` which represents the insert-at position for the new DBG_VALUE. Prior to this patch `SelectionDAGBuilder::SDNodeOrder` is used instead of the `Order` parameter. The only code-paths where `Order != SDNodeOrder` are the two calls calls to `handleDebugValue` from `salvageUnresolvedDbgValue`. `salvageUnresolvedDbgValue` is called from `resolveOrClearDbgInfo` and `dropDanglingDebugInfo`. The former is called after SelectionDAG completes one block. Some dbg.values can't be lowered to DBG_VALUEs right away. These get recorded as 'dangling' and get salvaged later through `dropDanglingDebugInfo`, or if we've still got dangling debug info once the whole block has been emitted, through `resolveOrClearDbgInfo`. Prior to this patch, DBG_VALUEs inserted using these functions are inserted at the "current" `SDNodeOrder` rather than the intended position recorded in `DanglingDebugInfo`. Fix and add test.
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-selectiondag Author: Orlando Cazalet-Hyams (OCHyams) Changes
The only code-paths where Some dbg.values can't be lowered to DBG_VALUEs right away. These get recorded as 'dangling' - their order-number is saved - and get salvaged later through Prior to this patch, DBG_VALUEs inserted using these functions are inserted at the "current" Fix and add test. Full diff: https://github.com/llvm/llvm-project/pull/94458.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index ba76456b5836a..a0de95de1653b 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -1684,7 +1684,7 @@ bool SelectionDAGBuilder::handleDebugValue(ArrayRef<const Value *> Values,
if (!FragmentExpr)
continue;
SDDbgValue *SDV = DAG.getVRegDbgValue(
- Var, *FragmentExpr, RegAndSize.first, false, DbgLoc, SDNodeOrder);
+ Var, *FragmentExpr, RegAndSize.first, false, DbgLoc, Order);
DAG.AddDbgValue(SDV, false);
Offset += RegisterSize;
}
@@ -1699,11 +1699,10 @@ bool SelectionDAGBuilder::handleDebugValue(ArrayRef<const Value *> Values,
}
// We have created a SDDbgOperand for each Value in Values.
- // Should use Order instead of SDNodeOrder?
assert(!LocationOps.empty());
SDDbgValue *SDV = DAG.getDbgValueList(Var, Expr, LocationOps, Dependencies,
/*IsIndirect=*/false, DbgLoc,
- SDNodeOrder, IsVariadic);
+ Order, IsVariadic);
DAG.AddDbgValue(SDV, /*isParameter=*/false);
return true;
}
diff --git a/llvm/test/DebugInfo/X86/sdag-order.ll b/llvm/test/DebugInfo/X86/sdag-order.ll
new file mode 100644
index 0000000000000..f959a80656791
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/sdag-order.ll
@@ -0,0 +1,46 @@
+; RUN: llc %s --stop-after=finalize-isel -o - | FileCheck %s
+
+;; Check the DBG_VALUE which is salvaged from the dbg.value using an otherwised
+;; unused value is emitted at the correct position in the function.
+;; Prior (-) to patching (+), these DBG_VALUEs would sink to the bottom of the
+;; function:
+;; │ bb.1.if.then:
+;; │- $rax = COPY %1
+;; │ DBG_VALUE 0, $noreg, !9, !DIExpression(DW_OP_plus_uconst, 4, DW_OP_stack_value)
+;; │+ $rax = COPY %1
+;; │ RET 0, $rax
+
+; CHECK: bb.1.if.then:
+; CHECK-NEXT: DBG_VALUE 0, $noreg, ![[#]], !DIExpression(DW_OP_plus_uconst, 4, DW_OP_stack_value)
+
+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"
+
+define void @badger(ptr sret(i64) %sret) !dbg !5 {
+entry:
+ %f.i = getelementptr i8, ptr null, i64 4
+ br label %if.then
+
+if.then: ; preds = %entry
+ tail call void @llvm.dbg.value(metadata ptr %f.i, metadata !9, metadata !DIExpression()), !dbg !11
+ ret void
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.ll", directory: "/")
+!2 = !{i32 3}
+!3 = !{i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "_ZNK1d1gEv", linkageName: "_ZNK1d1gEv", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 5, column: 1, scope: !5)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Hi, At google we see some increases of ~1.5% in DWP file sizes of some targets due to this change. Also, running |
|
Hi @asmok-g, It's expected that some variables will get greater variable location coverage, and some variables which previously didn't get variable locations at all now may get some coverage (reflected in b I haven't run size tests myself so I don't know if the numbers you're seeing are indicative of anything going wrong, but they sound plausible without indicating an error IMO. If you're able I'd suggest taking a look at a sample of some of the variable location changes and see if anything looks out of place. That said, I'm quite confident that this patch is doing the right thing and the size increase is just a consequence of us having more location data. |
SelectionDAGBuilder::handleDebugValuehas a parameterOrderwhich represents the insert-at position for the new DBG_VALUE. Prior to this patchSelectionDAGBuilder::SDNodeOrderis used instead of theOrderparameter.The only code-paths where
Order != SDNodeOrderare the two calls calls tohandleDebugValuefromsalvageUnresolvedDbgValue.salvageUnresolvedDbgValueis called fromresolveOrClearDbgInfoanddropDanglingDebugInfo. The former is called after SelectionDAG completes one block.Some dbg.values can't be lowered to DBG_VALUEs right away. These get recorded as 'dangling' - their order-number is saved - and get salvaged later through
dropDanglingDebugInfo, or if we've still got dangling debug info once the whole block has been emitted, throughresolveOrClearDbgInfo. Their saved order-number is passed tohandleDebugValue.Prior to this patch, DBG_VALUEs inserted using these functions are inserted at the "current"
SDNodeOrderrather than the intended position that is passed to the function.Fix and add test.