[DebugInfo] Do not extend range for physreg in LiveDebugVariables

[bjope]

Summary

A DBG_VALUE that is referring to a physical register is
valid up until the next def of the register, or the end
of the basic block that it belongs to.

LiveDebugVariables is computing live intervals (slot index
ranges) for DBG_VALUE instructions, before regalloc, in order
to be able to re-insert DBG_VALUE instructions again after
regalloc. When the DBG_VALUE is mapping a variable to a
physical register we do not need to compute the range. We
should simply re-insert the DBG_VALUE at the start position.

The problem that was found, resulting in this patch, was a
situation when the DBG_VALUE was the last real use of the
physical register. The computeIntervals/extendDef methods
extended the range to cover the whole basic block, even though
the physical register very well could be allocated to some
virtual register inside the basic block. So the extended
range could not be trusted.

This patch is more or less a preparation for https://reviews.llvm.org/D38229,
where the goal is to insert DBG_VALUE after each new definition
of a variable, even if the virtual registers that the variable
was connected to has been coalesced into using the same physical
register (e.g. due to two address instructions). For more info
see https://bugs.llvm.org/show_bug.cgi?id=34545

[aprantl]

This seems reasonable, thanks!
Could we make the test check the MIR instead of the debug output?

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir

2

If you really need to check the debug output of the pass (could we check the IR instead?) this needs a REQUIRES: asserts.

[rnk]

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir

152

I think this is testable with dwarfdump. The range for argc should run exactly up until it is clobbered by a0, so the location lists will look like:

DW_TAG_formal_parameter
  DW_AT_location (0xNNN
    0x000 - [[SET_RDI:0x.*]]: RDI)
  DW_AT_name: "argc"
DW_TAG_variable
  DW_AT_location (0xNNN
    [[SET_RDI]] - {{.*}}: RDI)
  DW_AT_name: "a0"

[bjope]

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir

2

Well, this patch does not impact the IR/MIR by itself. So in order to have a test case that fails without the patch LiveDebugVariables I need to look at the debug printout (I do not think that it is possible to get the VRM and LDV data structures dumped in the MIR yet?).

I can however prepare a test case that would fail if we land https://reviews.llvm.org/D38229 without landing this patch first.

[bjope]

Added REQUIRES asserts to the test case.
Added a second RUN line to also verify the MIR.

[bjope]

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir

152

Sure we could probably use dwarfdump, but I think that would make this test case dependent on the behavior of the later passes emitting the dwarf, as well as dwarfdump. By stopping before livedebugvalues I will limit the test case to verify the behavior of LiveDebugVariables (and inevitably some other RA passes run together with "greedy"). I think it will make the test case more robust if we limit the scope of the test.

If we need to test the whole chain of passes (integration test of several passes), then we should add a test in the debuginfo-tests repo instead. Or what do you think?

[aprantl]

live-debug-variables happens so late in the pipeline that in practice checking for dwarfdump output hasn't caused much churn in the testsuite. I would prefer a -filetype=obj -start-before=greedy | llvm-dwarfdump- test over one that checks DEBUG output (which will only be tested by bots that build with assertions enabled).

[bjope]

Narrowed the test case even further by stopping already after virtregrewriter.

[bjope]

In D38140#881660, @aprantl wrote:

live-debug-variables happens so late in the pipeline that in practice checking for dwarfdump output hasn't caused much churn in the testsuite. I would prefer a -filetype=obj -start-before=greedy | llvm-dwarfdump- test over one that checks DEBUG output (which will only be tested by bots that build with assertions enabled).

This patch is to make sure that the output from the virtregrewriter is correct (i.e. the input to later passes such as LiveDebugValues and DwarfDebug). I want to do the checks as early as possible to catch faults in virtregrewriter without the risk of later passes hiding the fault.
My feeling is that the DBG_VALUE handling in the backend is quite unstable today. I think we need to define what the expectation is from each pass. Unless we start adding test cases to verify how each pass is handling DBG_VALUEs it will become hard to make things better.

One idea is to remove the CHECKDBG checks again in https://reviews.llvm.org/D38229 (and by that loosing some kind if regression tests on how LiveDebugVariables is calculating slot index ranges for physical registers). If D38229 had been pushed before this patch,. then this patch would become a bugfix and I would probably have settled with only adding the CHECKMIR checks.
Or maybe I need to write a class test for LiveDebugVariables.

[aprantl]

Okay, let's do this: Split the CHECKDBG test into a separate .test file that has the REQUIRES: asserts and re-uses the same input. This way we get some testing in all configurations and full testing in asserts builds.

[bjope]

In D38140#882296, @aprantl wrote:

Okay, let's do this: Split the CHECKDBG test into a separate .test file that has the REQUIRES: asserts and re-uses the same input. This way we get some testing in all configurations and full testing in asserts builds.

Good idea. I'll split the test case and then I will land this. Thanks!

lib/CodeGen/LiveDebugVariables.cpp

if (TargetRegisterInfo::isVirtualRegister(Loc.getReg())) {
       }
       SmallVector<SlotIndex, 16> Kills;
       extendDef(Idx, LocNo, LI, VNI, &Kills, LIS);
       if (LI)
         addDefsFromCopies(LI, LocNo, Kills, Defs, MRI, LIS);
       continue;
     }
 
-    // For physregs, use the live range of the first regunit as a guide.
-    unsigned Unit = *MCRegUnitIterator(Loc.getReg(), &TRI);
-    LiveRange *LR = &LIS.getRegUnit(Unit);
-    const VNInfo *VNI = LR->getVNInfoAt(Idx);
-    // Don't track copies from physregs, it is too expensive.
-    extendDef(Idx, LocNo, LR, VNI, nullptr, LIS);
+    // For physregs, we only mark the start slot idx. DwarfDebug will see it
+    // as if the DBG_VALUE is valid up until the end of the basic block, or
+    // the next def of the physical register. So we do not need to extend the
+    // range. It might actually happen that the DBG_VALUE is the last use of
+    // the physical register (e.g. if this is an unused input argument to a
+    // function).
   }
 
   // Erase all the undefs.
   for (LocMap::iterator I = locInts.begin(); I.valid();)
     if (I.value() == UndefLocNo)
       I.erase();
     else
       ++I;

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir

+# RUN: llc -O1 -start-before=greedy -stop-before=livedebugvalues -o /dev/null %s -debug-only=livedebugvars 2>&1 | FileCheck -check-prefix=CHECKDBG %s
+# RUN: llc -O1 -start-before=greedy -stop-before=livedebugvalues -o - %s | FileCheck -check-prefix=CHECKMIR %s
+
+# REQUIRES: asserts
+
+# This test case was generated by using the following c program:
+#   extern void foo(int, int);
+#
+#   int bar[2] = {1, 2};
+#
+#   int main(int argc, char** argv)
+#   {
+#     int a0 = bar[0];
+#     int a1 = bar[1];
+#     foo(a0, a1);
+#     return 0;
+#   }
+#
+# It was compiled with -g and -O1, and the mir was dumped before ra greedy.
+
+--- |
+  ; ModuleID = 'live-debug-vars-unused-arg.ll'
+  source_filename = "live-debug-vars-unused-arg.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  @bar = local_unnamed_addr global [2 x i32] [i32 1, i32 2], align 4, !dbg !0
+
+  ; Function Attrs: nounwind uwtable
+  define i32 @main(i32 %argc, i8** nocapture readnone %argv) local_unnamed_addr #0 !dbg !14 {
+  entry:
+    tail call void @llvm.dbg.value(metadata i32 %argc, metadata !21, metadata !DIExpression()), !dbg !25
+    tail call void @llvm.dbg.value(metadata i8** %argv, metadata !22, metadata !DIExpression()), !dbg !26
+    %0 = load i32, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @bar, i64 0, i64 0), align 4, !dbg !27, !tbaa !28
+    tail call void @llvm.dbg.value(metadata i32 %0, metadata !23, metadata !DIExpression()), !dbg !32
+    %1 = load i32, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @bar, i64 0, i64 1), align 4, !dbg !33, !tbaa !28
+    tail call void @llvm.dbg.value(metadata i32 %1, metadata !24, metadata !DIExpression()), !dbg !34
+    tail call void @foo(i32 %0, i32 %1) #2, !dbg !35
+    ret i32 0, !dbg !36
+  }
+
+  declare void @foo(i32, i32) local_unnamed_addr
+
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #2
+
+  attributes #0 = { nounwind uwtable }
+  attributes #1 = { nounwind readnone speculatable }
+  attributes #2 = { nounwind }
+
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!10, !11, !12}
+  !llvm.ident = !{!13}
+
+  !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+  !1 = distinct !DIGlobalVariable(name: "bar", scope: !2, file: !3, line: 3, type: !6, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 6.0.0 (trunk 313866) (llvm/trunk 313875)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5)
+  !3 = !DIFile(filename: "live-debug-vars-unused-arg.c", directory: "/repo/uabbpet/master")
+  !4 = !{}
+  !5 = !{!0}
+  !6 = !DICompositeType(tag: DW_TAG_array_type, baseType: !7, size: 64, elements: !8)
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{!9}
+  !9 = !DISubrange(count: 2)
+  !10 = !{i32 2, !"Dwarf Version", i32 4}
+  !11 = !{i32 2, !"Debug Info Version", i32 3}
+  !12 = !{i32 1, !"wchar_size", i32 4}
+  !13 = !{!"clang version 6.0.0 (trunk 313866) (llvm/trunk 313875)"}
+  !14 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 5, type: !15, isLocal: false, isDefinition: true, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: true, unit: !2, variables: !20)
+  !15 = !DISubroutineType(types: !16)
+  !16 = !{!7, !7, !17}
+  !17 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !18, size: 64)
+  !18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !19, size: 64)
+  !19 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+  !20 = !{!21, !22, !23, !24}
+  !21 = !DILocalVariable(name: "argc", arg: 1, scope: !14, file: !3, line: 5, type: !7)
+  !22 = !DILocalVariable(name: "argv", arg: 2, scope: !14, file: !3, line: 5, type: !17)
+  !23 = !DILocalVariable(name: "a0", scope: !14, file: !3, line: 7, type: !7)
+  !24 = !DILocalVariable(name: "a1", scope: !14, file: !3, line: 8, type: !7)
+  !25 = !DILocation(line: 5, column: 14, scope: !14)
+  !26 = !DILocation(line: 5, column: 27, scope: !14)
+  !27 = !DILocation(line: 7, column: 12, scope: !14)
+  !28 = !{!29, !29, i64 0}
+  !29 = !{!"int", !30, i64 0}
+  !30 = !{!"omnipotent char", !31, i64 0}
+  !31 = !{!"Simple C/C++ TBAA"}
+  !32 = !DILocation(line: 7, column: 7, scope: !14)
+  !33 = !DILocation(line: 8, column: 12, scope: !14)
+  !34 = !DILocation(line: 8, column: 7, scope: !14)
+  !35 = !DILocation(line: 9, column: 3, scope: !14)
+  !36 = !DILocation(line: 10, column: 3, scope: !14)
+
+...
+---
+name:            main
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr32, preferred-register: '' }
+  - { id: 1, class: gr64, preferred-register: '' }
+  - { id: 2, class: gr32, preferred-register: '' }
+  - { id: 3, class: gr32, preferred-register: '' }
+  - { id: 4, class: gr32, preferred-register: '' }
+liveins:
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        true
+  stackProtector:  ''
+  maxCallFrameSize: 4294967295
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:
+stack:
+constants:
+body:             |
+  bb.0.entry:
+    DBG_VALUE debug-use %edi, debug-use _, !21, !DIExpression(), debug-location !25
+    DBG_VALUE debug-use %rsi, debug-use _, !22, !DIExpression(), debug-location !26
+    %2 = MOV32rm %rip, 1, _, @bar, _, debug-location !27 :: (dereferenceable load 4 from `i32* getelementptr inbounds ([2 x i32], [2 x i32]* @bar, i64 0, i64 0)`, !tbaa !28)
+    DBG_VALUE debug-use %2, debug-use _, !23, !DIExpression(), debug-location !32
+    %3 = MOV32rm %rip, 1, _, @bar + 4, _, debug-location !33 :: (dereferenceable load 4 from `i32* getelementptr inbounds ([2 x i32], [2 x i32]* @bar, i64 0, i64 1)`, !tbaa !28)
+    DBG_VALUE debug-use %3, debug-use _, !24, !DIExpression(), debug-location !34
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !35
+    %edi = COPY %2, debug-location !35
+    %esi = COPY %3, debug-location !35
+    CALL64pcrel32 @foo, csr_64, implicit %rsp, implicit killed %edi, implicit killed %esi, implicit-def %rsp, debug-location !35
+    ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !35
+    %eax = MOV32r0 implicit-def dead %eflags, debug-location !36
+    RET 0, killed %eax, debug-location !36
+
+...
+
+# Let's verify that the slot index ranges for the unused variables argc/argv,
+# connected to physical regs %EDI and %RSI, does not overlap with the ranges
+# for %vreg2 and %vreg3. The register allocator is actually allocating the
+# virtual registers # to %EDI and %ESI, so the ranges for argc/argv should
+# not cover the whole BB.
+#
+# CHECKDBG-LABEL: ********** EMITTING LIVE DEBUG VARIABLES **********
+# CHECKDBG-NEXT: !"argc,5"        [0B;0e):0 Loc0=%EDI
+# CHECKDBG-NEXT:         [0B;0e):0 BB#0-160B
+# CHECKDBG-NEXT: !"argv,5"        [0B;0e):0 Loc0=%RSI
+# CHECKDBG-NEXT:         [0B;0e):0 BB#0-160B
+# CHECKDBG-NEXT: !"a0,7"  [16r;64r):0 Loc0=%vreg2
+# CHECKDBG-NEXT:         [16r;64r):0 BB#0-160B
+# CHECKDBG-NEXT: !"a1,8"  [32r;80r):0 Loc0=%vreg3
+# CHECKDBG-NEXT:         [32r;80r):0 BB#0-160B
+
+# Verify that we only get one DBG_VALUE for argc and one for argv.
+# CHECKMIR: ![[ARGC:[0-9]+]] = !DILocalVariable(name: "argc", arg: 1
+# CHECKMIR: ![[ARGV:[0-9]+]] = !DILocalVariable(name: "argv", arg: 2
+# CHECKMIR: name:            main
+# CHECKMIR: body:
+# CHECKMIR: DBG_VALUE debug-use %edi, debug-use _, ![[ARGC]]
+# CHECKMIR: DBG_VALUE debug-use %rsi, debug-use _, ![[ARGV]]
+# CHECKMIR-NOT: DBG_VALUE debug-use %{{.*}}, debug-use _, ![[ARGC]]
+# CHECKMIR-NOT: DBG_VALUE debug-use %{{.*}}, debug-use _, ![[ARGV]]
+