[DebugInfo][DAG] Always emit DBG_VALUEs of FrameIndexes regardless of their SDNodes

[jmorse]

Summary

This patch recognises dbg.values of Alloca/FrameIndexes and processes them independently of other dbg.value lowering rules. There are two advantages:

• We can create FrameIndex SDNodeDbgValue records even if the Value is never lowered to an SDNode in the current block.
• We do not attach the SDNodeDbgValue to an SDNode: the variable location is still valid even if the SDNode is invalidated.

This catches two sets of circumstances where dbg.values are currently dropped, respectively:

• dbg.values of frame index Values that are not referenced in the current block,
• dbg.values of frame index Values where corresponding SDNode is folded into the operand of a memory instruction: such nodes are marked Invalidated and emitted as undef DBG_VALUEs.

The attached new test replicates both of these circumstances, with current trunk both dbg.values become undef DBG_VALUEs. There's a corresponding DebugInfo test (without the '2' suffix) that tests for FrameIndexes being lowered, however it tests a circumstance where the SDNode for the frameindex is not invalidated.

The NVPTX test changes because DBG_VALUEs are now emitted in the correct order. In the source file the order of dbg.values have types {const, const, vreg, frameindex}, but the test was originally getting {frameindex, const, const, vreg} as output. This patch restores it to the correct order. I'm 99% certain the reason it was originally misordered is the ProcessSourceNode matter mentioned at the end of D57163, which is now bypassed by the FrameIndex SDNodeDbgValue not being attached to an SDNode.

The assumption of this patch is that FrameIndexes are always a valid target of DBG_VALUEs: it bypasses the various ways in which SDNodes might not be emitted to machine instructions. There's a risk that the patch is incorrect if FrameIndexes are later garbage collected and the DBG_VALUEs not erased, however I don't currently know where to look to check for this.

There's an explanation for why getFrameIndexDbgValue gets passed 'false' for IsIndirect in SelectionDAGBuilder::getDbgValue, it might be worth copying that comment into the patched code?

Finally, using r351324 llvm/clang and building clang-3.4 with and without this patch, I get improvements:

• %variable coverage, 66% -> 75%
• %scope bytes covered, 39% -> 44%

Which... seems a little too good to be true, thus extra scrutiny may be in order. (My numbers have never lined up with the series on lnt.llvm.org).

[aprantl]

We can create FrameIndex SDNodeDbgValue records even if the Value is never lowered to an SDNode in the current block.

That sounds awesome.

If a dbg.value pointing to a frame index and a dbg.value with a constant (both describing the same variable) appear in the same basic block, is the relative order between them lost?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5326

I'd probably use auto here.

[jmorse]

Make use of auto, add constant-value dbg.value's to test for their correct ordering with FrameIndex dbg.values.

Adrian wrote:

If a dbg.value pointing to a frame index and a dbg.value with a constant (both describing the same variable) appear in the same basic block, is the relative order between them lost?

The order should not be lost -- they will both retain their IR location number, and ScheduleDAGSDNodes::EmitSchedule should emit them in that numbered order. I think the worst case scenario is that intervening instructions are completely optimised out, but even in that case they will still retain their ordering.

I've added a couple of constant dbg.values to the new test to check the ordering is preserved.

[aprantl]

Thanks!

[jmorse]

After hitting the buildbots, a bunch of PowerPC builders started reporting XPASS for test/DebugInfo/Generic/incorrect-variable-debugloc1.ll. That XFAIL has an associated bug report:

https://bugs.llvm.org/show_bug.cgi?id=21881

Which appears to precisely document a scenario that this patch would solve. I've removed the XFAIL, written up my view on this in the bug report, and hopefully someone will confirm the PowerPC location list looks correct.

[jmorse]

Step change in the LNT debuginfo tracking, marked up as a "regression" though:

http://lnt.llvm.org/db_default/v4/nts/regressions/19927

[aprantl]

In D57328#1375284, @jmorse wrote:

Step change in the LNT debuginfo tracking, marked up as a "regression" though:

http://lnt.llvm.org/db_default/v4/nts/regressions/19927

Nice!
@cmatthews: What would we need to do to make LNT mark the percent regions covered metric as higher-is-better? (This isn't necessarily true, but most of the time, it's what we expect.)

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::dbg_value: {
     SDDbgValue *SDV;
     if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V) ||
         isa<ConstantPointerNull>(V)) {
       SDV = DAG.getConstantDbgValue(Variable, Expression, V, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, nullptr, false);
       return nullptr;
     }
 
+    // If the Value is a frame index, we can create a FrameIndex debug value
+    // without relying on the DAG at all.
+    if (const AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
+      auto SI = FuncInfo.StaticAllocaMap.find(AI);
+      if (SI != FuncInfo.StaticAllocaMap.end()) {
+        auto SDV =
+            DAG.getFrameIndexDbgValue(Variable, Expression, SI->second,
+                                      /*IsIndirect*/ false, dl, SDNodeOrder);
+        // Do not attach the SDNodeDbgValue to an SDNode: this variable location
+        // is still available even if the SDNode gets optimized out.
+        DAG.AddDbgValue(SDV, nullptr, false);
+        return nullptr;
+      }
+    }
+
     // Do not use getValue() in here; we don't want to generate code at
     // this point if it hasn't been done yet.
     SDValue N = NodeMap[V];
     if (!N.getNode() && isa<Argument>(V)) // Check unused arguments map.
       N = UnusedArgNodeMap[V];
     if (N.getNode()) {
       if (EmitFuncArgumentDbgValue(V, Variable, Expression, dl, false, N))
         return nullptr;

test/DebugInfo/NVPTX/dbg-value-const-byref.ll

 ;   f2(&i);
 ;   return 0;
 ; }
 ;
 ; Test that we generate valid debug info for optimized code,
 ; particularly variables that are described as constants and passed
 ; by reference.
 ;
-; CHECK: DEBUG_VALUE: foo:i <- [DW_OP_deref] $vrdepot
 ; CHECK: DEBUG_VALUE: foo:i <- 3
 ; CHECK: DEBUG_VALUE: foo:i <- 7
 ; CHECK: DEBUG_VALUE: foo:i <- %
+; CHECK: DEBUG_VALUE: foo:i <- [DW_OP_deref] $vrdepot
 
 ; Function Attrs: nounwind ssp uwtable
 define i32 @foo() #0 !dbg !4 {
 entry:
   %i = alloca i32, align 4
   call void @llvm.dbg.value(metadata i32 3, metadata !10, metadata !DIExpression()), !dbg !15
   %call = call i32 @f3(i32 3) #3, !dbg !16
   call void @llvm.dbg.value(metadata i32 7, metadata !10, metadata !DIExpression()), !dbg !18

test/DebugInfo/X86/dbg-value-frame-index-2.ll

+; RUN: llc -start-after=codegenprepare -stop-before=expand-isel-pseudos < %s -o - | FileCheck %s
+
+; Test that stack frame dbg.values are lowered to DBG_VALUEs, in blocks that
+; are local to the alloca, and elsewhere. Differs from dbg-value-frame-index.ll
+; because this test does not result in the frame-index being in a vreg,
+; instead it's exclusively referred to by memory operands of instructions.
+;
+; Additionally test that we don't re-order with constant values -- both are
+; independent of the order the instructions get lowered, but should not
+; interleave.
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-unknown"
+
+declare void @dud()
+
+; CHECK: [[BARVAR:![0-9]+]] = !DILocalVariable(name: "bar",
+
+define i32 @foo() !dbg !6 {
+; CHECK-LABEL: body
+
+; CHECK:      DBG_VALUE 0, $noreg, [[BARVAR]]
+; CHECK-NEXT: MOV32mi %[[STACKLOC:[a-zA-Z0-9\.]+]], 1, $noreg
+; CHECK-NEXT: DBG_VALUE %[[STACKLOC]], $noreg, [[BARVAR]]
+
+  %p1 = alloca i32
+  call void @llvm.dbg.value(metadata i32 *null, metadata !17, metadata !DIExpression()), !dbg !18
+  store i32 0, i32 *%p1
+  call void @llvm.dbg.value(metadata i32 *%p1, metadata !17, metadata !DIExpression()), !dbg !18
+  br label %foo
+
+foo:
+
+; CHECK-LABEL: bb.1.foo
+; CHECK:      DBG_VALUE %[[STACKLOC]], $noreg, [[BARVAR]]
+
+  call void @dud()
+  call void @llvm.dbg.value(metadata i32 *%p1, metadata !17, metadata !DIExpression()), !dbg !18
+  br label %bar
+
+bar:
+
+; CHECK-LABEL: bb.2.bar
+; CHECK:      DBG_VALUE %[[STACKLOC]], $noreg, [[BARVAR]]
+; CHECK-NEXT: ADJCALLSTACKDOWN
+; CHECK-NEXT: CALL
+; CHECK-NEXT: ADJCALLSTACKUP
+; CHECK-NEXT: DBG_VALUE 0, $noreg, [[BARVAR]]
+  call void @llvm.dbg.value(metadata i32 *%p1, metadata !17, metadata !DIExpression()), !dbg !18
+  call void @dud()
+  call void @llvm.dbg.value(metadata i32 *null, metadata !17, metadata !DIExpression()), !dbg !18
+  %loaded = load i32, i32 *%p1
+  ret i32 %loaded, !dbg !19
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #6
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !2)
+!1 = !DIFile(filename: "a.c", directory: "b")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!""}
+!6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 13, type: !7, isLocal: false, isDefinition: true, scopeLine: 14, isOptimized: false, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{!140}
+!140 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = !{!16}
+!16 = !DISubrange(count: 5)
+!17 = !DILocalVariable(name: "bar", scope: !6, line: 13, type: !140)
+!18 = !DILocation(line: 13, column: 23, scope: !6)
+!19 = !DILocation(line: 15, column: 5, scope: !6)
+!20 = !DILocation(line: 16, column: 1, scope: !6)
+!21 = !DILocalVariable(name: "baz", scope: !6, line: 13, type: !140)