[DebugInfo][DAG] Allow creation of DBG_VALUEs in blocks where the operand is not used

[jmorse]

Summary

This patch allows DBG_VALUEs to be created by SelectionDAGBuilder referring to Values that aren't used in the current basic block. This is done by allowing any Value that has a VReg allocated but no SDNode in this block, to be referred to as a DBG_VALUE of that VReg.

Observe the attached test case (IR only, the rest of the DebugInfo is lifted from a different test). Currently, the dbg.value of %1 will not generate a DBG_VALUE in the "nextbb" block because %1 isn't used there, and thus doesn't get an SDNode. Instead the debug record is left in SelectionDAG's DanglingDebugInfo across BBs until %1 is used. That's unfortunate for the user because the variables location range is un-necessarily shortened. I've seen many circumstances where inlining and CSE effectively hoists the computation of a value out of a block, leaving a dbg.value in this situation, to the inconvenience of the user.

We already have code in SelectionDAGBuilder that creates debug records for VRegs: this patch just allows it to happen for most Values (any Value used across BBs gets a VReg). Some observations:

• We can be sure the produced DBG_VALUE will refer to the correct VReg def, because (I believe) SelectionDAG produces machine code still in SSA form,
• There's a risk that SelectionDAG re-orders instructions, but that's no different for VReg debug records than SDNode debug records,
• Some of the DBG_VALUEs generated by this change will be of dead / non-live VRegs, AFAIK this isn't something that LLVM currently cares too much about,
• Some of the DBG_VALUES could also be debug use-before-defs, but that'll be the fault of other optimisation passes.

I've filtered out any Argument Values that have an associated VReg: otherwise DebugInfo/X86/sdag-split-arg.ll fails. Otherwise it seems DBG_VALUEs end up in one group in the middle of the BB in that test, rather than being interleaved with the argument copies. Filtering out Argument Values allows them to reach SelectionDAGBuilder::EmitFuncArgumentDbgValue where there's special-case behaviour.

(This patch doesn't currently lead to variable coverage improvement due to CodeGenPrepare::placeDbgValues juggling dbg.values, however this patch enables progress in the near-future. In fact, I suspect the use-case of this patch hasn't been encountered, due to placeDbgValues. See PR38754 for why it needs to go away).

See also r331182 / D46129 for the discussion when this VReg debug record facility was originally added to SelectionDAGBuilder.

[bjope]

Interesting. If I remember correctly this is similar to something I played around with half a year ago (as a preparation before prohibiting CodeGenPrepare::placeDbgValues to move DBG_VALUE between basic blocks (or even to disable CodeGenPrepare::placeDbgValues completely)). I'll have a closer look at this when I'm at the office. I'll also try to remember if I had some specific reasons for not moving forward with my attempt at doing this - it has been bothering me that I never found the time to finalize that patch.

[aprantl]

Seems okay to me, but it would be good to wait Björn's LGTM.

[bjope]

LGTM!

PS. I found my old git branch where I played around with this, and I think that I never figured out that I needed the !isa<Argument> check. Maybe that is why I had mixed feeling about the result back in the days.

[jmorse]

Thanks for the reviews,

In D56678#1359924, @bjope wrote:

PS. I found my old git branch where I played around with this, and I think that I never figured out that I needed the !isa<Argument> check. Maybe that is why I had mixed feeling about the result back in the days.

It's a well hidden code path, and I suspect there's extra pain with argument DBG_VALUEs given examples such as [0], alas :/

[0] https://bugs.llvm.org/show_bug.cgi?id=40188

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::dbg_value: {
     if (N.getNode()) {
       if (EmitFuncArgumentDbgValue(V, Variable, Expression, dl, false, N))
         return nullptr;
       SDV = getDbgValue(N, Variable, Expression, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, N.getNode(), false);
       return nullptr;
     }
 
-    // PHI nodes have already been selected, so we should know which VReg that
-    // is assigns to already.
-    if (isa<PHINode>(V)) {
+    // The value is not used in this block yet (or it would have an SDNode).
+    // We still want the value to appear for the user if possible -- if it has
+    // an associated VReg, we can refer to that instead.
+    if (!isa<Argument>(V)) {
       auto VMI = FuncInfo.ValueMap.find(V);
       if (VMI != FuncInfo.ValueMap.end()) {
         unsigned Reg = VMI->second;
-        // The PHI node may be split up into several MI PHI nodes (in
-        // FunctionLoweringInfo::set).
+        // If this is a PHI node, it may be split up into several MI PHI nodes
+        // (in FunctionLoweringInfo::set).
         RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), Reg,
                          V->getType(), None);
         if (RFV.occupiesMultipleRegs()) {
           unsigned Offset = 0;
           unsigned BitsToDescribe = 0;
           if (auto VarSize = Variable->getSizeInBits())
             BitsToDescribe = *VarSize;
           if (auto Fragment = Expression->getFragmentInfo())

test/DebugInfo/X86/sdag-dbgvalue-ssareg.ll

+; RUN: llc -start-after=codegenprepare -stop-before expand-isel-pseudos -o - %s | FileCheck %s
+
+; Test that dbg.values of an SSA variable that's not used in a basic block,
+; is converted to a DBG_VALUE in that same basic block. We know that %1 is
+; live from the entry bb to the exit bb, is allocated a vreg because it's
+; used across basic blocks. We should be able to produce a DBG_VALUE that
+; refers to it in the nextbb block.
+
+source_filename = "debug.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind uwtable
+define dso_local i32 @main(i32 %arg0, i32 %arg1) local_unnamed_addr !dbg !11 {
+entry:
+  %0 = add i32 %arg0, 42, !dbg !26
+  %1 = add i32 %arg1, 101, !dbg !26
+  %cmp = icmp eq i32 %1, 0
+  br i1 %cmp, label %nextbb, label %exit, !dbg !26
+
+nextbb:
+; CHECK-LABEL: bb.{{.*}}.nextbb
+  %2 = mul i32 %0, %arg1, !dbg !26
+; CHECK: IMUL32rr
+  call void @llvm.dbg.value(metadata i32 %1, metadata !16, metadata !DIExpression()), !dbg !27
+; CHECK-NEXT: DBG_VALUE
+  br label %exit, !dbg !26
+
+; CHECK-LABEL: bb.{{.*}}.exit
+exit:
+  %3 = phi i32 [ 12, %entry ], [ %2, %nextbb ], !dbg !26
+  %4 = add i32 %1, %3, !dbg !26
+  ret i32 %4, !dbg !26
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7, !8, !9}
+!llvm.ident = !{!10}
+
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 7.0.0 (x)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4)
+!3 = !DIFile(filename: "debug.c", directory: "")
+!4 = !{}
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !{i32 2, !"Dwarf Version", i32 4}
+!8 = !{i32 2, !"Debug Info Version", i32 3}
+!9 = !{i32 1, !"wchar_size", i32 4}
+!10 = !{!"clang version 7.0.0 (x)"}
+!11 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 3, type: !12, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: true, unit: !2, retainedNodes: !14)
+!12 = !DISubroutineType(types: !13)
+!13 = !{!6}
+!14 = !{!16}
+!16 = !DILocalVariable(name: "y", scope: !11, file: !3, line: 5, type: !6)
+!26 = !DILocation(line: 4, column: 7, scope: !11)
+!27 = !DILocation(line: 5, column: 7, scope: !11)
+!31 = !{!32, !32, i64 0}
+!32 = !{!"int", !33, i64 0}
+!33 = !{!"omnipotent char", !34, i64 0}
+!34 = !{!"Simple C/C++ TBAA"}