[1/2][LiveDebugVariables] Avoid reduntant DBG_VALUEs after virtregrewrite

[djtodoro]

Summary

After the register allocator is done, more precisely, after the Virtual Register Rewriter, we end up having duplicated DBG_VALUEs, since some virtual registers are being rewritten into the same physical register as some of existing DBG_VALUEs. Each DBG_VALUE should indicate (at least before the LiveDebugValues) variables assignment, but it is being clobbered for function parameters during the SelectionDAG since it generates new DBG_VALUEs after COPY instructions, even though the parameter has no assignment. For example, if we had a DBG_VALUE $regX as an entry debug value representing the parameter, and a COPY and after the COPY, DBG_VALUE $virt_reg, and after the virtregrewrite the $virt_reg gets rewritten into $regX, we'd end up having redundant DBG_VALUE.

This breaks the definition of the DBG_VALUE since some analysis passes might be built on top of that premise..., and this patch tries to fix the MIR with the respect to that.

[jmorse]

This sounds great to me -- suppressing redundant DBG_VALUEs is a good aim. I'm slightly worried about performance however: in D94981 a patch landed to cover a scenario where findInsertLocation was repeatedly stepping back over every debug instruction at the start of a block, effectively becoming quadratic-complexity. Wouldn't looking back over every DBG_VALUE already inserted, when inserting a new one, become quadratic too?

Possibly the solution is to act like D94981, and build a temporary cache of "DBG_VALUEs already inserted at this SlotIdx".

llvm/test/DebugInfo/X86/livedebugvars-avoid-duplicates.ll

4

This is running quite a lot of llc, to then hit a fair specific scenario (DBG_VALUE then frame-setup), which makes me worry that it'll be fragile. Would it be sufficient to just check that there are two DBG_VALUEs in the output MIR, and that's it?

(Also, hard-coded metadata numbers)

[djtodoro]

In D105025#2844255, @jmorse wrote:

This sounds great to me -- suppressing redundant DBG_VALUEs is a good aim. I'm slightly worried about performance however: in D94981 a patch landed to cover a scenario where findInsertLocation was repeatedly stepping back over every debug instruction at the start of a block, effectively becoming quadratic-complexity. Wouldn't looking back over every DBG_VALUE already inserted, when inserting a new one, become quadratic too?

Possibly the solution is to act like D94981, and build a temporary cache of "DBG_VALUEs already inserted at this SlotIdx".

Yes... I wanted to get a consensus this is needed, and definitely, I'll need to measure performance impact and optimize it eventually. I think some caching will be appreciated here in order to avoid (potential) performance regression.

llvm/test/DebugInfo/X86/livedebugvars-avoid-duplicates.ll

4

yep, I'll need to fix it

[Orlando]

llvm/test/DebugInfo/X86/livedebugvars-avoid-duplicates.ll

4

You could add --implicit-check-not=DBG_VALUE to FileCheck here too to ensure there are no extra DBG_VALUEs?

[djtodoro]

Alternative implemented as D105279.

llvm/lib/CodeGen/LiveDebugVariables.cpp

if (any_of(Regs, [&I, &TRI](Register &Reg) {
         }))
       // The insert location is directly after the instruction/bundle.
       return std::next(I);
     ++I;
   }
   return MBB->end();
 }
 
+// Avoid duplicates by scanning the machine basic block backward, up to the
+// first non-debug instr or up to the MBBStart.
+static bool shouldSkipThisDbgValue(MachineBasicBlock *MBB, MachineInstr &MI,
+                                   const DILocalVariable *Variable,
+                                   const MachineOperand &LocOp,
+                                   const DIExpression *Expr, DebugLoc &dl) {
+  MachineBasicBlock::reverse_iterator it =
+      std::next(MachineBasicBlock::reverse_iterator(MI));
+  while (it != MBB->rend()) {
+    if (!it->isDebugValue())
+      return false;
+
+    MachineOperand &loc = it->getDebugOperand(0);
+    if (Variable == it->getDebugVariable() && loc.isReg() && LocOp.isReg() &&
+        loc.getReg() == LocOp.getReg() && Expr == it->getDebugExpression() &&
+        dl == it->getDebugLoc()) {
+      LLVM_DEBUG(llvm::dbgs() << "\navoiding duplicated "; it->dump(););
+      return true;
+    }
+
+    it = std::next(it);
+  }
+
+  return false;
+}
+
 void UserValue::insertDebugValue(MachineBasicBlock *MBB, SlotIndex StartIdx,
                                  SlotIndex StopIdx, DbgVariableValue DbgValue,
                                  ArrayRef<bool> LocSpills,
                                  ArrayRef<unsigned> SpillOffsets,
                                  LiveIntervals &LIS, const TargetInstrInfo &TII,
                                  const TargetRegisterInfo &TRI,
                                  BlockSkipInstsMap &BBSkipInstsMap) {
   SlotIndex MBBEndIdx = LIS.getMBBEndIdx(&*MBB);
   // Only search within the current MBB.
   StopIdx = (MBBEndIdx < StopIdx) ? MBBEndIdx : StopIdx;
   MachineBasicBlock::iterator I =
       findInsertLocation(MBB, StartIdx, LIS, BBSkipInstsMap);
+
+  // After virtual register rewriting, we can end up having multiple same
+  // contiguous DBG_VALUEs representing the same variable, so we are avoiding
+  // duplicates here.
+  if (locations.size() == 1 && I != MBB->end() &&
+      shouldSkipThisDbgValue(MBB, *I, Variable, locations[0],
+                             DbgValue.getExpression(), dl))
+    return;
+
   // Undef values don't exist in locations so create new "noreg" register MOs
   // for them. See getLocationNo().
   SmallVector<MachineOperand, 8> MOs;
   if (DbgValue.isUndef()) {
     MOs.assign(DbgValue.loc_nos().size(),
                MachineOperand::CreateReg(
                    /* Reg */ 0, /* isDef */ false, /* isImp */ false,
                    /* isKill */ false, /* isDead */ false,

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

 ; while the latter was emitted for the parameter copy to a virtual register inserted
 ; at the function entry by DAGBuilder.
 define hidden void @ptr_arg_split_reg_mem(<31 x i32>, %struct.A* %arg2) #0 !dbg !25 {
 ; CHECK-LABEL: ptr_arg_split_reg_mem:
 ; CHECK:       .Lfunc_begin1:
 ; CHECK-NEXT:    .loc 1 10 0 ; example.cpp:10:0
 ; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
-; CHECK-NEXT:    ;DEBUG_VALUE: ptr_arg_split_reg_mem:b <- [$vgpr31+0]
+;; NOTE: One dbg_value (DEBUG_VALUE: ptr_arg_split_reg_mem:b <- [$vgpr31+0]) will be considered as
+;; redundant after the virtregrewrite, so it will be removed.
 ; CHECK-NEXT:    ;DEBUG_VALUE: ptr_arg_split_reg_mem:b <- [$vgpr31+0]
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    buffer_load_dword v32, off, s[0:3], s32
 ; CHECK-NEXT:    v_mov_b32_e32 v0, 1
 ; CHECK-NEXT:  .Ltmp2:
 ; CHECK-NEXT:    .loc 1 12 13 prologue_end ; example.cpp:12:13
 ; CHECK-NEXT:    s_waitcnt vmcnt(0)
 ; CHECK-NEXT:    flat_store_dword v[31:32], v0 offset:396

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:          .loc    1 0 38 is_stmt 0

llvm/test/DebugInfo/MIR/Hexagon/live-debug-values-bundled-entry-values.mir

 liveins:
   - { reg: '$r0' }
 body:             |
   bb.0.entry:
     successors: %bb.1
     liveins: $r0
 
     DBG_VALUE $r0, $noreg, !22, !DIExpression(), debug-location !23
-    DBG_VALUE $r0, $noreg, !22, !DIExpression(), debug-location !23
     BUNDLE implicit-def dead $p0, implicit-def $pc, implicit killed $r0, implicit killed $r31, debug-location !24 {
       renamable $p0 = C2_cmpeqi killed renamable $r0, 0, debug-location !24
       PS_jmprettnew internal killed $p0, killed $r31, implicit-def $pc, implicit-def $pc, implicit killed undef $r0, debug-location !27
     }
 
   bb.1.land.rhs:
     CFI_INSTRUCTION def_cfa $r30, 8
     CFI_INSTRUCTION offset $r31, -4

llvm/test/DebugInfo/MIR/X86/multiple-param-dbg-value-entry.mir

 tracksRegLiveness: true
 liveins:
   - { reg: '$edi' }
 body:             |
   bb.0.entry:
     liveins: $edi
 
     DBG_VALUE $edi, $noreg, !15, !DIExpression(), debug-location !18
-    DBG_VALUE $edi, $noreg, !15, !DIExpression(), debug-location !18
     DBG_VALUE $esi, $noreg, !16, !DIExpression(), debug-location !18
     DBG_VALUE $edx, $noreg, !17, !DIExpression(), debug-location !18
     renamable $edi = nsw INC32r killed renamable $edi, implicit-def dead $eflags, debug-location !18
     MOV32mr $rip, 1, $noreg, @global, $noreg, killed renamable $edi, debug-location !18 :: (store 4 into @global)
     INLINEASM &"", 1, 12, implicit-def dead early-clobber $edi, 12, implicit-def dead early-clobber $esi, 12, implicit-def dead early-clobber $edx, 12, implicit-def dead early-clobber $df, 12, implicit-def dead early-clobber $fpsw, 12, implicit-def dead early-clobber $eflags, !19, debug-location !18
     $eax = MOV32ri 123, debug-location !18
     RETQ killed $eax, debug-location !18
 
 ...

llvm/test/DebugInfo/X86/livedebugvars-avoid-duplicates.ll

+;; Check that after virtregrewrite MIR does not contain
+;; redundant DBG_VALUEs.
+
+; RUN: llc -O2 %s -stop-before=livedebugvalues -o - | FileCheck %s
+
+; CHECK: DBG_VALUE $edi, $noreg, !13, !DIExpression(), debug-location !14
+; CHECK-NEXT: frame-setup
+; CHECK: DBG_VALUE $edi, $noreg, !13, !DIExpression(), debug-location !14
+
+; ModuleID = 'test.c'
+source_filename = "test.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@side_effect = external dso_local local_unnamed_addr global i32, align 4
+@value = external dso_local local_unnamed_addr global i32, align 4
+
+; Function Attrs: nounwind uwtable
+define dso_local i32 @foo(i32 %param) local_unnamed_addr !dbg !8 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %param, metadata !13, metadata !DIExpression()), !dbg !14
+  store i32 %param, i32* @side_effect, align 4, !dbg !15
+  %0 = load i32, i32* @value, align 4, !dbg !20
+  call void @llvm.dbg.value(metadata i32 %0, metadata !13, metadata !DIExpression()), !dbg !14
+  tail call void @bar(i32 %0), !dbg !21
+  ret i32 0, !dbg !22
+}
+
+declare !dbg !23 dso_local void @bar(i32) 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, !6}
+!llvm.ident = !{!7}
+
+!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 = !{i32 7, !"uwtable", i32 1}
+!7 = !{!"clang version 13.0.0"}
+!8 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, type: !9, scopeLine: 4, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13}
+!13 = !DILocalVariable(name: "param", arg: 1, scope: !8, file: !1, line: 4, type: !11)
+!14 = !DILocation(line: 0, scope: !8)
+!15 = !DILocation(line: 5, column: 17, scope: !8)
+!20 = !DILocation(line: 6, column: 13, scope: !8)
+!21 = !DILocation(line: 7, column: 5, scope: !8)
+!22 = !DILocation(line: 8, column: 5, scope: !8)
+!23 = !DISubprogram(name: "bar", scope: !1, file: !1, line: 1, type: !24, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!24 = !DISubroutineType(types: !25)
+!25 = !{null, !11}