[LiveDebugVariables] Stop trimming locations of non-inlined vars

[djtodoro]

Summary

The D35953, D62650 and D73691 introduced trimming of variables locations in LiveDebugVariables pass, since there are some cases where after the virtregrewrite we have exploded number of DBG_VALUEs created for some inlined variables. As it looks, all problematic cases were regarding inlined variables, so it seems reasonable to stop trimming the location ranges for non-inlined variables. It has very good impact on the llvm-locstats report.

By using GDB 7.11 compiled with -g -O2 as test bed, the numbers looks as follows:

(before this patch)

$ llvm-locstats ./gdb/gdb
 ================================================
            Debug Location Statistics
 ================================================
     cov%           samples         percentage(~)
 -----------------------------------------------
   0%                 2621                2%
   (0%,10%)           5675                5%
   [10%,20%)          5050                4%
   [20%,30%)          4997                4%
   [30%,40%)          4527                4%
   [40%,50%)          4237                4%
   [50%,60%)          4679                4%
   [60%,70%)          4737                4%
   [70%,80%)          5656                5%
   [80%,90%)          6481                6%
   [90%,100%)        12957               12%
   100%              41014               39%
 ================================================
 -the number of debug variables processed: 102631
 -PC ranges covered: 56%
 -----------------------------------------------
  -total availability: 86%
 ================================================

(after applying this patch)

$ llvm-locstats ./gdb/gdb
 ================================================
            Debug Location Statistics
 ================================================
     cov%           samples         percentage(~)
 -----------------------------------------------
   0%                 2620                2%
   (0%,10%)           5628                5%
   [10%,20%)          4972                4%
   [20%,30%)          4862                4%
   [30%,40%)          4431                4%
   [40%,50%)          4109                4%
   [50%,60%)          4539                4%
   [60%,70%)          4625                4%
   [70%,80%)          5517                5%
   [80%,90%)          6232                6%
   [90%,100%)        10154                9%
   100%              44942               43%
 ================================================
 -the number of debug variables processed: 102631
 -PC ranges covered: 57%
 -----------------------------------------------
 -total availability: 86%
 ================================================

[Orlando]

Hi @djtodoro,

It seems odd that not trimming the locations would have a positive impact on coverage since, IIUC, the only difference should be an increase in bytes covered outside the lexical scope (which is not counted towards "PC ranges covered" as of D85636). I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue. I have vague memories from my time working on D82129 that LexicalScope::getRanges doesn't include the prologue in any range or something like that. I'm unsure whether that is intended behaviour or not - I would be interested if anyone knows? I think my solution in D82129 was similarly to avoid trimming the location ranges for function-scoped variables.

[djtodoro]

@Orlando Thanks for looking into this!

I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue.

I think this is the main reason of the improvement. The motivation for this fix was to improve MIR and DBG_VALUE, so it becomes as close as possible to its COPY.

[jmorse]

This makes sense to me: the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong. Instruction spans with the "wrong" scope in the middle of a block will prematurely terminate ranges -- or in the case of the prologue, it won't cover early instructions. I think there's also potential for an argument that's moved more than once in the prologue to be dropped, not completely certain.

Either way: it's my understanding that the range trimming is about avoiding excessive compile time costs. There's no functional reason why we shouldn't extend all ranges over the whole program, it just happens to create pathological behaviour in certain circumstances, particularly when there's heavy inlining. IMO, making this conditional on a variable not being inlined is a good trade-off, we produce as much information as possible for the small number of "top level" variables, and less for everything that's inlined.

(One comment inline).

llvm/lib/CodeGen/LiveDebugVariables.cpp

1172

Does this range-check need to be guarded by getInlinedAt() too?

[djtodoro]

@jmorse Thanks for your comment!

the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong.

Yes, exactly. We are trying to fix some things in LiveDebugValues by doing some kind of analysis based on assumption that DBG_VALUE (up to LiveDebugValues) maps to a source-level assignment (that may indicate a new value), but it turns that it isn't the truth in every situation (e.g. there are a lot of duplicated DBG_VALUEs in one basic block that doesn't represent new value/assignment -- it is not only from SelectionDAG's DBG_VALUEs following COPY -- it is rather that after virtregrewrite virtual registers gets rewrited into the same physical register as some existing DBG_VALUE and some COPYs gets deleted, so after returning back DBG_VALUEs we are ending up with duplicated/redundant DBG_VALUEs for the same var. I am about to post some fixes for this in LiveDebugVariables)...

Either way: it's my understanding that the range trimming is about avoiding excessive compile time costs. There's no functional reason why we shouldn't extend all ranges over the whole program, it just happens to create pathological behaviour in certain circumstances, particularly when there's heavy inlining. IMO, making this conditional on a variable not being inlined is a good trade-off, we produce as much information as possible for the small number of "top level" variables, and less for everything that's inlined.

I agree.

llvm/lib/CodeGen/LiveDebugVariables.cpp

1172

Oh yes, nice catch. Thanks.

I think we don't need anything from D35953 for non-inlined instances, so I'll update the patch.

[djtodoro]

• improve the condition

[Orlando]

In D102917#2776626, @djtodoro wrote:

@Orlando Thanks for looking into this!

I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue.

I think this is the main reason of the improvement. The motivation for this fix was to improve MIR and DBG_VALUE, so it becomes as close as possible to its COPY.

Sounds reasonable to me.

In D102917#2776763, @jmorse wrote:

[...] or in the case of the prologue, it won't cover early instructions.

I was wondering whether LexicalScope::getRanges not including the prologue was a bug. There's no mention of this in the getRanges doxygen comment. Ah, I see that in LexicalScopes::extractLexicalScopes that instructions with an empty DebugLoc are skipped when trying to determine start/end of ranges. This means that all the instructions at the start of the function up to (but not including) the first one with a DebugLoc will not be included in any range. I think it might make sense to use the DISubprogram scope for the first instructions encountered if they have no DebugLoc? IIUC that would mean we don't need this change in LiveDebugVariables because all instructions in a function would be included in a range from getRanges. OTOH other parts of llvm also use getRanges so changing the behaviour may be undesirable or have unintended side effects, and also my speculations could just be wrong too. With that in mind, perhaps this (your patch) is the best way to approach the problem.

In D102917#2778963, @djtodoro wrote:

In D102917#2776763, @jmorse wrote:
Either way: it's my understanding that the range trimming is about avoiding excessive compile time costs. There's no functional reason why we shouldn't extend all ranges over the whole program, it just happens to create pathological behaviour in certain circumstances, particularly when there's heavy inlining. IMO, making this conditional on a variable not being inlined is a good trade-off, we produce as much information as possible for the small number of "top level" variables, and less for everything that's inlined.

I agree.

+1. FWIW I'm happy with this change either way that it is implemented. I'm going to be "out of office" for a week starting tomorrow so please don't wait for me.

EDIT: Slightly reworded to improve clarity.

[djtodoro]

In D102917#2779103, @Orlando wrote:

In D102917#2776626, @djtodoro wrote:

@Orlando Thanks for looking into this!

I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue.

I think this is the main reason of the improvement. The motivation for this fix was to improve MIR and DBG_VALUE, so it becomes as close as possible to its COPY.

Sounds reasonable to me.

In D102917#2776763, @jmorse wrote:

[...] or in the case of the prologue, it won't cover early instructions.

I was wondering whether LexicalScope::getRanges not including the prologue was a bug. There's no mention of this in the getRanges doxygen comment. Ah, I see that in LexicalScopes::extractLexicalScopes that instructions with an empty DebugLoc are skipped when trying to determine start/end of ranges. This means that all the instructions at the start of the function up to (but not including) the first one with a DebugLoc will not be included in any range. I think it might make sense to use the DISubprogram scope for the first instructions encountered if they have no DebugLoc? IIUC that would mean we don't need this change in LiveDebugVariables because all instructions in a function would be included in a range from getRanges. OTOH other parts of llvm also use getRanges so changing the behaviour may be undesirable or have unintended side effects, and also my speculations could just be wrong too. With that in mind, perhaps this (your patch) is the best way to approach the problem.

Good catch, I think this might be interesting to play with, but I'd still go with this patch first.

In D102917#2778963, @djtodoro wrote:

In D102917#2776763, @jmorse wrote:
Either way: it's my understanding that the range trimming is about avoiding excessive compile time costs. There's no functional reason why we shouldn't extend all ranges over the whole program, it just happens to create pathological behaviour in certain circumstances, particularly when there's heavy inlining. IMO, making this conditional on a variable not being inlined is a good trade-off, we produce as much information as possible for the small number of "top level" variables, and less for everything that's inlined.

I agree.

+1. FWIW I'm happy with this change either way that it is implemented. I'm going to be "out of office" for a week starting tomorrow so please don't wait for me.

Thanks!

EDIT: Slightly reworded to improve clarity.

@jmorse Is this ok now? :)

[jmorse]

LGTM plus nit.

llvm/lib/CodeGen/LiveDebugVariables.cpp

1139

Nit -- un-necessary newline

llvm/lib/CodeGen/LiveDebugVariables.cpp

void UserValue::computeIntervals(MachineRegisterInfo &MRI,
 
   // The computed intervals may extend beyond the range of the debug
   // location's lexical scope. In this case, splitting of an interval
   // can result in an interval outside of the scope being created,
   // causing extra unnecessary DBG_VALUEs to be emitted. To prevent
   // this, trim the intervals to the lexical scope.
 
   LexicalScope *Scope = LS.findLexicalScope(dl);
   if (!Scope)
     return;
 
   SlotIndex PrevEnd;
   LocMap::iterator I = locInts.begin();
 
   // Iterate over the lexical scope ranges. Each time round the loop
   // we check the intervals for overlap with the end of the previous
   // range and the start of the next. The first range is handled as
if (PrevEnd && I.start() < PrevEnd) {
       // Stop overlaps previous end - trim the end of the interval to
       // the scope range.
       I.setStopUnchecked(PrevEnd);
       ++I;
 
       // If the interval also overlaps the start of the "next" (i.e.
       // current) range create a new interval for the remainder (which
       // may be further trimmed).
       if (RStart < IStop)
         I.insert(RStart, IStop, DbgValue);
     }
 
     // Advance I so that I.stop() >= RStart, and check for overlap.
     I.advanceTo(RStart);
     if (!I.valid())
       return;
 
-    if (I.start() < RStart) {
+    if (dl.getInlinedAt() && I.start() < RStart) {
       // Interval start overlaps range - trim to the scope range.
       I.setStartUnchecked(RStart);
       // Remember that this interval was trimmed.
       trimmedDefs.insert(RStart);
     }
 
     // The end of a lexical scope range is the last instruction in the
     // range. To convert to an interval we need the index of the

llvm/test/CodeGen/AMDGPU/ptr-arg-dbg-value.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 < %s | FileCheck %s
 
 %struct.A = type { [100 x i32] }
 
 ; The 64-bit pointer argument %arg1 will be split into two registers
 ; and for its llvm.dbg.declare, DAG should emit two DBG_VALUE instructions
 ; with the fragment expressions.
 define hidden void @ptr_arg_split_subregs(%struct.A* %arg1) #0 !dbg !9 {
 ; CHECK-LABEL: ptr_arg_split_subregs:
 ; CHECK:       .Lfunc_begin0:
 ; CHECK:       .loc 1 5 0 ; example.cpp:5:0
 ; CHECK-NEXT:    .cfi_sections .debug_frame
 ; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
 ; CHECK-NEXT:    ;DEBUG_VALUE: ptr_arg_split_subregs:a <- [DW_OP_LLVM_fragment 32 32] [$vgpr1+0]
+; CHECK-NEXT:    ;DEBUG_VALUE: ptr_arg_split_subregs:a <- [DW_OP_LLVM_fragment 0 32] [$vgpr0+0]
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    v_mov_b32_e32 v2, 1
 ; CHECK-NEXT:  .Ltmp0:
-; CHECK-NEXT:    ;DEBUG_VALUE: ptr_arg_split_subregs:a <- [DW_OP_LLVM_fragment 0 32] [$vgpr0+0]
 ; CHECK-NEXT:    .loc 1 7 13 prologue_end ; example.cpp:7:13
 ; CHECK-NEXT:    flat_store_dword v[0:1], v2 offset:396
 ; CHECK-NEXT:    .loc 1 8 5 ; example.cpp:8:5
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_setpc_b64 s[30:31]
 ; CHECK-NEXT:  .Ltmp1:
 ; CHECK:         .cfi_endproc
   call void @llvm.dbg.declare(metadata %struct.A* %arg1, metadata !20, metadata !DIExpression()), !dbg !21

llvm/test/CodeGen/PowerPC/non-debug-mi-search-frspxsrsp.ll

 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
 
 ; Function Attrs: nounwind
 define dso_local void @test(float* nocapture readonly %Fptr, <4 x float>* nocapture %Vptr) local_unnamed_addr #0 !dbg !10 {
 ; CHECK-LABEL: test:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:          #DEBUG_VALUE: test:Fptr <- $x3
+; CHECK-NEXT:          #DEBUG_VALUE: test:Fptr <- $x3
 ; CHECK-NEXT:          #DEBUG_VALUE: test:Vptr <- $x4
 ; CHECK-NEXT:          addis 5, 2, .LCPI0_0@toc@ha
 ; CHECK-NEXT:  .Ltmp0:
 ; CHECK-NEXT:          .loc    1 2 38 prologue_end
 ; CHECK-NEXT:          lfsx 0, 0, 3
 ; CHECK-NEXT:          addis 3, 2, .LCPI0_1@toc@ha
 ; CHECK-NEXT:  .Ltmp1:
-; CHECK-NEXT:          #DEBUG_VALUE: test:Fptr <- $x3
 ; CHECK-NEXT:          .loc    1 0 38 is_stmt 0
 ; CHECK-NEXT:          lfs 1, .LCPI0_0@toc@l(5)
 ; CHECK-NEXT:          lfd 2, .LCPI0_1@toc@l(3)
 ; CHECK-NEXT:          .loc    1 2 27
 ; CHECK-NEXT:          xssubdp 1, 1, 0
 ; CHECK-NEXT:          .loc    1 2 45
 ; CHECK-NEXT:          xsadddp 1, 1, 2
 ; CHECK-NEXT:  .Ltmp2:

llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir

+# RUN: llc -run-pass=greedy -run-pass=virtregrewriter %s -o - | FileCheck %s
+
+## This tests that LiveDebugVariables does not trim non-inlined variable
+## location.
+
+# CHECK: ![[VARB:.*]] = !DILocalVariable(name: "b"
+# CHECK: ![[VARC:.*]] = !DILocalVariable(name: "c"
+# CHECK: $at = COPY $a2
+# CHECK-NEXT: DBG_VALUE $at, $noreg, ![[VARC]], !DIExpression(), debug-location
+# CHECK: $s0 = COPY $a1
+# CHECK-NEXT: DBG_VALUE $s0, $noreg, ![[VARB]], !DIExpression(), debug-location
+
+--- |
+  ; ModuleID = 'test.c'
+  source_filename = "test.c"
+  target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
+  target triple = "mips-unknown-linux-gnu"
+  
+  ; Function Attrs: nounwind
+  define dso_local i32 @fn2(i32 signext %a, i32 signext %b, i32 signext %c) local_unnamed_addr !dbg !7 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %a, metadata !12, metadata !DIExpression()), !dbg !16
+    call void @llvm.dbg.value(metadata i32 %b, metadata !13, metadata !DIExpression()), !dbg !16
+    call void @llvm.dbg.value(metadata i32 %c, metadata !14, metadata !DIExpression()), !dbg !16
+    %add = add nsw i32 %b, %a, !dbg !17
+    %add1 = add nsw i32 %b, 10, !dbg !18
+    %call = tail call i32 @fn1(i32 signext %add, i32 signext %c, i32 signext %add1), !dbg !19
+    call void @llvm.dbg.value(metadata i32 %call, metadata !15, metadata !DIExpression()), !dbg !16
+    %cmp = icmp sgt i32 %call, 10, !dbg !20
+    %add2 = add nsw i32 %call, 10, !dbg !22
+    %retval.0 = select i1 %cmp, i32 %add2, i32 %b, !dbg !22
+    ret i32 %retval.0, !dbg !23
+  }
+  
+  declare !dbg !24 dso_local i32 @fn1(i32 signext, i32 signext, i32 signext) local_unnamed_addr
+  
+  ; Function Attrs: nofree nosync nounwind readnone speculatable willreturn mustprogress
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 13.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/dir")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 13.0.0"}
+  !7 = distinct !DISubprogram(name: "fn2", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !10, !10, !10}
+  !10 = !DIBasicType(name: "long int", size: 32, encoding: DW_ATE_signed)
+  !11 = !{!12, !13, !14, !15}
+  !12 = !DILocalVariable(name: "a", arg: 1, scope: !7, file: !1, line: 2, type: !10)
+  !13 = !DILocalVariable(name: "b", arg: 2, scope: !7, file: !1, line: 2, type: !10)
+  !14 = !DILocalVariable(name: "c", arg: 3, scope: !7, file: !1, line: 2, type: !10)
+  !15 = !DILocalVariable(name: "local", scope: !7, file: !1, line: 3, type: !10)
+  !16 = !DILocation(line: 0, scope: !7)
+  !17 = !DILocation(line: 3, column: 22, scope: !7)
+  !18 = !DILocation(line: 3, column: 30, scope: !7)
+  !19 = !DILocation(line: 3, column: 17, scope: !7)
+  !20 = !DILocation(line: 4, column: 14, scope: !21)
+  !21 = distinct !DILexicalBlock(scope: !7, file: !1, line: 4, column: 8)
+  !22 = !DILocation(line: 4, column: 8, scope: !7)
+  !23 = !DILocation(line: 7, column: 2, scope: !7)
+  !24 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 1, type: !8, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+
+...
+---
+name:            fn2
+alignment:       4
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr32, preferred-register: '' }
+  - { id: 1, class: gpr32, preferred-register: '' }
+  - { id: 2, class: gpr32, preferred-register: '' }
+  - { id: 3, class: gpr32, preferred-register: '' }
+  - { id: 4, class: gpr32, preferred-register: '' }
+  - { id: 5, class: gpr32, preferred-register: '' }
+  - { id: 6, class: gpr32, preferred-register: '' }
+  - { id: 7, class: gpr32, preferred-register: '' }
+  - { id: 8, class: gpr32, preferred-register: '' }
+liveins:
+  - { reg: '$a0', virtual-reg: '%0' }
+  - { reg: '$a1', virtual-reg: '%1' }
+  - { reg: '$a2', virtual-reg: '%2' }
+body:             |
+  bb.0.entry:
+    liveins: $a0, $a1, $a2
+  
+    DBG_VALUE $a0, $noreg, !12, !DIExpression(), debug-location !16
+    DBG_VALUE $a1, $noreg, !13, !DIExpression(), debug-location !16
+    DBG_VALUE $a2, $noreg, !14, !DIExpression(), debug-location !16
+    %2:gpr32 = COPY $a2
+    DBG_VALUE %2, $noreg, !14, !DIExpression(), debug-location !16
+    %8:gpr32 = COPY $a1
+    DBG_VALUE %8, $noreg, !13, !DIExpression(), debug-location !16
+    %0:gpr32 = COPY $a0
+    DBG_VALUE %0, $noreg, !12, !DIExpression(), debug-location !16
+    %3:gpr32 = nsw ADDu %8, %0, debug-location !17
+    ADJCALLSTACKDOWN 16, 0, implicit-def dead $sp, implicit $sp, debug-location !19
+    %4:gpr32 = nsw ADDiu %8, 10, debug-location !18
+    $a0 = COPY %3, debug-location !19
+    $a1 = COPY %2, debug-location !19
+    DBG_VALUE $a1, $noreg, !14, !DIExpression(), debug-location !16
+    $a2 = COPY %4, debug-location !19
+    JAL @fn1, csr_o32, implicit-def dead $ra, implicit $a0, implicit $a1, implicit $a2, implicit-def $sp, implicit-def $v0, debug-location !19
+    ADJCALLSTACKUP 16, 0, implicit-def dead $sp, implicit $sp, debug-location !19
+    %5:gpr32 = COPY killed $v0, debug-location !19
+    DBG_VALUE %5, $noreg, !15, !DIExpression(), debug-location !16
+    %6:gpr32 = SLTi %5, 11, debug-location !22
+    %7:gpr32 = nsw ADDiu %5, 10, debug-location !22
+    %8:gpr32 = MOVZ_I_I %7, %6, %8, debug-location !22
+    $v0 = COPY %8, debug-location !23
+    RetRA implicit killed $v0, debug-location !23
+
+...

llvm/test/DebugInfo/X86/dbg-addr-dse.ll

entry:
   store i32 1, i32* @global, align 4, !dbg !22
   call void @llvm.dbg.addr(metadata i32* %x.addr, metadata !13, metadata !DIExpression()), !dbg !23
   store i32 2, i32* %x.addr, align 4, !dbg !23
   call void @escape(i32* %x.addr), !dbg !24
   ret void, !dbg !25
 }
 
 ; ASM-LABEL: f: # @f
-; ASM: movl    %ecx, [[OFF_X:[0-9]+]](%rsp)
-; ASM: #DEBUG_VALUE: f:x <- [DW_OP_plus_uconst [[OFF_X]]] [$rsp+0]
+; ASM: #DEBUG_VALUE: f:x <- [DW_OP_plus_uconst [[OFF_X:[0-9]+]]] [$rsp+0]
+; ASM: movl    %ecx, [[OFF_X]](%rsp)
 ; ASM: callq   escape
 ; ASM: #DEBUG_VALUE: f:x <- 1
 ; ASM: movl    $1, global(%rip)
 ; ASM: #DEBUG_VALUE: f:x <- [DW_OP_plus_uconst [[OFF_X]]] [$rsp+0]
 ; ASM: movl    $2, [[OFF_X]](%rsp)
 ; ASM: callq   escape
 ; ASM: retq