[DebugInfo] Fix updateDbgUsersToReg to support DBG_VALUE_LIST

[StephenTozer]

Summary

This patch resolves an issue reported in review https://reviews.llvm.org/D91722 (post-commit). The issue is relatively simple; the function updateDbgUsersToReg was not updated to support DBG_VALUE_LIST instructions in the patch that introduced that instruction. The fix is also simple, simply replacing hardcoded operand indices (getOperand(0)) with the new debug operand getter. The patch has also modified the assertion MI->isDebugInstr() with MI->isDebugValue(), as the former is overly general for this function. A test will be added to the review shortly, but the functional changes of the patch should be easy to review regardless.

[jmorse]

Looks mechanically correct, test wanted seeing how this was missed before.

[StephenTozer]

Added test case.

[StephenTozer]

Remove git hash and directory path from test case metadata.

[jmorse]

llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir

5

13

Hardcoded metadata numbers will break in the future, this needs to be captured and compared.

15

IMO: better to check the entire line.

[StephenTozer]

Fixed an issue with the previous implementation of this fix: the CopyDbgUsers vectors would sometimes contain duplicate elements, causing errors when we attempt to replace the same debug operand multiple times. This has been fixed by making it into a SmallSet instead of a SmallVector, although we still convert to SmallVector before updating to avoid changing the MRI interface.

[jmorse]

This LGTM modulo one inline comment, plus: the test is in the "Generic" directory, but it is not fully generic. It depends on the armv6 triple being supported, so things like the powerpc build bots will likely trip on it. You should add an appropriate REQUIRES line.

llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir

5

"then" -> "when" :)

[jmorse]

llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir

5

Nope, actually today I can't read, ignore this.

[StephenTozer]

Added a "requires" to the test, and simplified the test target to just arm (instead of armv6k, which is not needed for the test to pass).

[dblaikie]

In D101523#2738995, @StephenTozer wrote:

Added a "requires" to the test, and simplified the test target to just arm (instead of armv6k, which is not needed for the test to pass).

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

[StephenTozer]

In D101523#2739537, @dblaikie wrote:

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

This is something I considered before updating; I believed that it would be better in the generic folder, because the test isn't about anything actually specific to ARM, we only need the ARM target for the current reproducer. Making it require ARM is the easiest solution, and if that means it should be in the ARM directory then I'm fine to move it there - although the ideal solution in that case would probably be to find a reproducer that doesn't depend on using ARM as the target.

[dblaikie]

In D101523#2740813, @StephenTozer wrote:

In D101523#2739537, @dblaikie wrote:

Might it be more suitable to move the test to the DebugInfo/ARM directory - which I think implicitly has such a requirement?

This is something I considered before updating; I believed that it would be better in the generic folder, because the test isn't about anything actually specific to ARM, we only need the ARM target for the current reproducer. Making it require ARM is the easiest solution, and if that means it should be in the ARM directory then I'm fine to move it there - although the ideal solution in that case would probably be to find a reproducer that doesn't depend on using ARM as the target.

Yeah - I don't think the target-specific directories are about issues being innately target specific, they only express the raw idea that the test requires ARM to run, mechanically speaking.

Yeah, generic tests are generally nicer - but I'll admit I don't try /especially/ hard to get generic reproducers - I'll checkin x86 tests pretty regularly, for instance.

[StephenTozer]

Move test to DebugInfo/ARM, remove explicit ARM target requirement.

[aeubanks]

I believe this is causing a crash: https://crbug.com/1206764. There is a non-reduced repro in the bug, currently creducing to come up with a reduced repro. Reverting...

[lebedev.ri]

@aeubanks reverted this the moment i was going to do so.

[StephenTozer]

I've identified the issue with this patch, I'll submit an updated version for review shortly.

[StephenTozer]

The previous version of the patch was handling subregisters incorrectly; the updateDbgUsersToReg function assumed that each operand MO that corresponded to OldRegwould satisfy MO.getReg() == OldReg - this is correct for virtual registers, which use the subReg field, but not for physical registers.

To fix the issue, updatedbgUsersToReg has been modified to take MCRegisters as arguments, and checks each debug operand for the target DBG_VALUE* instruction to see if it has any RegUnits in common with OldReg; if it does, then we replace it with NewReg.

[jmorse]

Hmmmmm. What if we have:

$rax = COPY $rcx
DBG_VALUE $ax

And the copy to $rax is copy-prop'd? As I understand it, the DBG_VALUE will appear on the MaybeDeadDbgUsers list, and its operand $ax will be detected as being affected by the copy propagation. But because we use:

Op.setReg(NewReg);

In updateDbgUsersToReg, doesn't this then produce:

DBG_VALUE $rcx

i.e., widens what was a 16 bit register to a 64 bit one?

Another question is what the old implementation of this did. Was it similarly borken?

[StephenTozer]

In D101523#2750052, @jmorse wrote:

Another question is what the old implementation of this did. Was it similarly borken?

You're exactly correct, this implementation has that flaw and the old one did as well. I think in most cases where we aren't using a full register, this doesn't matter because we're simply using the variable value directly, in which case the truncation to the variable size fixes this for us. There are some cases where this won't work however - such as if we have DW_OP_minus in the DWARF expression, in which case including the higher bits may result in an incorrect output. I'll open a bugzilla ticket about the issue, but since it exists before this patch I think the fix can come later.

[jmorse]

In D101523#2750300, @StephenTozer wrote:

I'll open a bugzilla ticket about the issue, but since it exists before this patch I think the fix can come later.

Misery. But, now tracked misery at the very least. To me, this LGTM as the cause of the assertion failure is now understood and addressed.

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

public:
 
   /// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
   /// specified register as undefined which causes the DBG_VALUE to be
   /// deleted during LiveDebugVariables analysis.
   void markUsesInDebugValueAsUndef(Register Reg) const;
 
   /// updateDbgUsersToReg - Update a collection of DBG_VALUE instructions
   /// to refer to the designated register.
-  void updateDbgUsersToReg(Register Reg,
-                           ArrayRef<MachineInstr*> Users) const {
+  void updateDbgUsersToReg(Register OldReg, Register NewReg,
+                           ArrayRef<MachineInstr *> Users) const {
     for (MachineInstr *MI : Users) {
-      assert(MI->isDebugInstr());
-      assert(MI->getOperand(0).isReg());
-      MI->getOperand(0).setReg(Reg);
+      assert(MI->isDebugValue());
+      assert(MI->hasDebugOperandForReg(OldReg));
+      for (auto &Op : MI->getDebugOperandsForReg(OldReg))
+        Op.setReg(NewReg);
     }
   }
 
   /// Return true if the specified register is modified in this function.
   /// This checks that no defining machine operands exist for the register or
   /// any of its aliases. Definitions found on functions marked noreturn are
   /// ignored, to consider them pass 'true' for optional parameter
   /// SkipNoReturnDef. The register is also considered modified when it is set

llvm/lib/CodeGen/MachineCopyPropagation.cpp

private:
   bool hasImplicitOverlap(const MachineInstr &MI, const MachineOperand &Use);
   bool hasOverlappingMultipleDef(const MachineInstr &MI,
                                  const MachineOperand &MODef, Register Def);
 
   /// Candidates for deletion.
   SmallSetVector<MachineInstr *, 8> MaybeDeadCopies;
 
   /// Multimap tracking debug users in current BB
-  DenseMap<MachineInstr*, SmallVector<MachineInstr*, 2>> CopyDbgUsers;
+  DenseMap<MachineInstr *, SmallSet<MachineInstr *, 2>> CopyDbgUsers;
 
   CopyTracker Tracker;
 
   bool Changed;
 };
 
 } // end anonymous namespace
 
void MachineCopyPropagation::ReadRegister(MCRegister Reg, MachineInstr &Reader,
   // for elimination. If a copy is "read" by a debug user, record the user
   // for propagation.
   for (MCRegUnitIterator RUI(Reg, TRI); RUI.isValid(); ++RUI) {
     if (MachineInstr *Copy = Tracker.findCopyForUnit(*RUI, *TRI)) {
       if (DT == RegularUse) {
         LLVM_DEBUG(dbgs() << "MCP: Copy is used - not dead: "; Copy->dump());
         MaybeDeadCopies.remove(Copy);
       } else {
-        CopyDbgUsers[Copy].push_back(&Reader);
+        CopyDbgUsers[Copy].insert(&Reader);
       }
     }
   }
 }
 
 /// Return true if \p PreviousCopy did copy register \p Src to register \p Def.
 /// This fact may have been obscured by sub register usage or may not be true at
 /// all even though Src and Def are subregisters of the registers used in
if (MBB.succ_empty()) {
     for (MachineInstr *MaybeDead : MaybeDeadCopies) {
       LLVM_DEBUG(dbgs() << "MCP: Removing copy due to no live-out succ: ";
                  MaybeDead->dump());
       assert(!MRI->isReserved(MaybeDead->getOperand(0).getReg()));
 
       // Update matching debug values, if any.
       assert(MaybeDead->isCopy());
       Register SrcReg = MaybeDead->getOperand(1).getReg();
-      MRI->updateDbgUsersToReg(SrcReg, CopyDbgUsers[MaybeDead]);
+      Register DestReg = MaybeDead->getOperand(0).getReg();
+      SmallVector<MachineInstr *> MaybeDeadDbgUsers(
+          CopyDbgUsers[MaybeDead].begin(), CopyDbgUsers[MaybeDead].end());
+      MRI->updateDbgUsersToReg(DestReg, SrcReg, MaybeDeadDbgUsers);
 
       MaybeDead->eraseFromParent();
       Changed = true;
       ++NumDeletes;
     }
   }
 
   MaybeDeadCopies.clear();

llvm/test/DebugInfo/Generic/machine-cp-updates-dbg-reg.mir

+# REQUIRES: arm-registered-target
+# RUN: llc %s -run-pass=machine-cp -o - | FileCheck %s --implicit-check-not=DBG_VALUE
+
+## Ensure that when the destination register of a copy instruction is used by a
+## DBG_VALUE/DBG_VALUE_LIST, and then that instruction is deleted during copy
+## propagation, the debug use is updated to the source register.
+
+# CHECK: ![[VAR_V:[0-9]+]] = !DILocalVariable(name: "v"
+# CHECK-LABEL: body:
+
+# CHECK: DBG_VALUE $r0, $noreg, ![[VAR_V]], !DIExpression()
+# CHECK: DBG_VALUE_LIST ![[VAR_V]], !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 4, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), $r0, 0
+
+--- |
+  target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "arm-unknown-linux-gnueabi"
+
+  %struct.atomic_t.0.6.15.24.48.51.54.57 = type { i32 }
+  %struct.mm_struct.2.8.17.26.50.53.56.59 = type { %struct.anon.1.7.16.25.49.52.55.58 }
+  %struct.anon.1.7.16.25.49.52.55.58 = type { i32 }
+
+  @dup_mm_err = external dso_local local_unnamed_addr global i32, align 4, !dbg !0
+
+  declare dso_local i32 @atomic_long_read(%struct.atomic_t.0.6.15.24.48.51.54.57*) local_unnamed_addr #0
+
+  declare dso_local i32 @get_mm_counter(%struct.mm_struct.2.8.17.26.50.53.56.59*, i32) local_unnamed_addr #0
+
+  define dso_local i32 @dup_mm() local_unnamed_addr !dbg !16 {
+  entry:
+    %call = tail call i32 bitcast (i32 (...)* @kmem_cache_alloc to i32 (i32)*)(i32 0)
+    br i1 undef, label %cleanup, label %if.end
+
+  if.end:                                           ; preds = %entry
+    %tobool1.not = icmp eq i32 undef, 0
+    br i1 %tobool1.not, label %if.end3, label %free_pt
+
+  if.end3:                                          ; preds = %if.end
+    call void @llvm.dbg.value(metadata !DIArgList(i32 %call, i32 0), metadata !30, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 4, DW_OP_mul, DW_OP_plus, DW_OP_stack_value)), !dbg !40
+    %0 = load volatile i32, i32* undef, align 4
+    br label %cleanup
+
+  free_pt:                                          ; preds = %if.end
+    store i32 0, i32* undef, align 4
+    br label %cleanup
+
+  cleanup:                                          ; preds = %free_pt, %if.end3, %entry
+    %retval.0 = phi i32 [ %call, %if.end3 ], [ 0, %entry ], [ 0, %free_pt ]
+    ret i32 %retval.0
+  }
+
+  declare dso_local i32 @kmem_cache_alloc(...) local_unnamed_addr
+
+  declare void @llvm.dbg.label(metadata)
+
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!14, !15}
+
+  !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+  !1 = distinct !DIGlobalVariable(name: "dup_mm_err", scope: !2, file: !3, line: 5, type: !12, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 13.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, retainedTypes: !9, globals: !13, splitDebugInlining: false, nameTableKind: None)
+  !3 = !DIFile(filename: "machine-cp-updates-dbg-reg.ll", directory: "/")
+  !4 = !{!5}
+  !5 = !DICompositeType(tag: DW_TAG_enumeration_type, file: !3, line: 4, baseType: !6, size: 32, elements: !7)
+  !6 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+  !7 = !{!8}
+  !8 = !DIEnumerator(name: "MM_SHMEMPAGES", value: 0, isUnsigned: true)
+  !9 = !{!10}
+  !10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 32)
+  !11 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !12)
+  !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !13 = !{!0}
+  !14 = !{i32 2, !"Debug Info Version", i32 3}
+  !15 = !{i32 1, !"min_enum_size", i32 4}
+  !16 = distinct !DISubprogram(name: "dup_mm", scope: !3, file: !3, line: 14, type: !17, scopeLine: 14, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !19)
+  !17 = !DISubroutineType(types: !18)
+  !18 = !{!12}
+  !19 = !{!20, !28, !29}
+  !20 = !DILocalVariable(name: "mm", scope: !16, file: !3, line: 15, type: !21)
+  !21 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !22, size: 32)
+  !22 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "mm_struct", file: !3, line: 7, size: 32, elements: !23)
+  !23 = !{!24}
+  !24 = !DIDerivedType(tag: DW_TAG_member, scope: !22, file: !3, line: 8, baseType: !25, size: 32)
+  !25 = distinct !DICompositeType(tag: DW_TAG_structure_type, scope: !22, file: !3, line: 8, size: 32, elements: !26)
+  !26 = !{!27}
+  !27 = !DIDerivedType(tag: DW_TAG_member, name: "binfmt", scope: !25, file: !3, line: 9, baseType: !12, size: 32)
+  !28 = !DILabel(scope: !16, name: "free_pt", file: !3, line: 22)
+  !29 = !DILabel(scope: !16, name: "fail_nomem", file: !3, line: 24)
+  !30 = !DILocalVariable(name: "v", arg: 1, scope: !31, file: !3, line: 6, type: !34)
+  !31 = distinct !DISubprogram(name: "atomic_long_read", scope: !3, file: !3, line: 6, type: !32, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !39)
+  !32 = !DISubroutineType(types: !33)
+  !33 = !{!12, !34}
+  !34 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !35, size: 32)
+  !35 = !DIDerivedType(tag: DW_TAG_typedef, name: "atomic_t", file: !3, line: 3, baseType: !36)
+  !36 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !3, line: 1, size: 32, elements: !37)
+  !37 = !{!38}
+  !38 = !DIDerivedType(tag: DW_TAG_member, name: "counter", scope: !36, file: !3, line: 2, baseType: !12, size: 32)
+  !39 = !{!30}
+  !40 = !DILocation(line: 0, scope: !31, inlinedAt: !41)
+  !41 = distinct !DILocation(line: 12, column: 3, scope: !42, inlinedAt: !48)
+  !42 = distinct !DISubprogram(name: "get_mm_counter", scope: !3, file: !3, line: 11, type: !43, scopeLine: 11, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !45)
+  !43 = !DISubroutineType(types: !44)
+  !44 = !{!22, !21, !12}
+  !45 = !{!46, !47}
+  !46 = !DILocalVariable(name: "mm", arg: 1, scope: !42, file: !3, line: 11, type: !21)
+  !47 = !DILocalVariable(name: "member", arg: 2, scope: !42, file: !3, line: 11, type: !12)
+  !48 = distinct !DILocation(line: 20, column: 3, scope: !16)
+
+...
+---
+name:            dup_mm
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+registers:       []
+liveins:         []
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       8
+  offsetAdjustment: 0
+  maxAlignment:    4
+  adjustsStack:    true
+  hasCalls:        true
+  stackProtector:  ''
+  maxCallFrameSize: 0
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -4, size: 4, alignment: 4,
+      stack-id: default, callee-saved-register: '$lr', callee-saved-restored: false,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, name: '', type: spill-slot, offset: -8, size: 4, alignment: 4,
+      stack-id: default, callee-saved-register: '$r4', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    successors: %bb.4(0x40000000), %bb.1(0x40000000)
+    liveins: $r4, $lr
+
+    $sp = frame-setup STMDB_UPD $sp, 14 /* CC::al */, $noreg, killed $r4, killed $lr
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    frame-setup CFI_INSTRUCTION offset $lr, -4
+    frame-setup CFI_INSTRUCTION offset $r4, -8
+    renamable $r4 = MOVi 0, 14 /* CC::al */, $noreg, $noreg
+    $r0 = MOVi 0, 14 /* CC::al */, $noreg, $noreg
+    BL @kmem_cache_alloc, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit $r0, implicit-def $sp, implicit-def $r0
+    CMPri renamable $r4, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
+    Bcc %bb.4, 1 /* CC::ne */, killed $cpsr
+
+  bb.1.if.end:
+    successors: %bb.2(0x30000000), %bb.3(0x50000000)
+    liveins: $r0
+
+    renamable $r4 = MOVi 0, 14 /* CC::al */, $noreg, $noreg
+    CMPri renamable $r4, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
+    Bcc %bb.3, 1 /* CC::ne */, killed $cpsr
+
+  bb.2.if.end3:
+    liveins: $r0
+
+    dead renamable $r1 = LDRi12 undef renamable $r0, 0, 14 /* CC::al */, $noreg :: (volatile load 4 from `i32* undef`)
+    renamable $r4 = COPY killed renamable $r0
+    DBG_VALUE $r4, $noreg, !30, !DIExpression(), debug-location !40
+    DBG_VALUE_LIST !30, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 4, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), $r4, 0, debug-location !40
+    $r0 = COPY killed renamable $r4
+    $sp = frame-destroy LDMIA_RET $sp, 14 /* CC::al */, $noreg, def $r4, def $pc, implicit $r0
+
+  bb.3.free_pt:
+    successors: %bb.4(0x80000000)
+    liveins: $r4
+
+    STRi12 renamable $r4, undef renamable $r0, 0, 14 /* CC::al */, $noreg :: (store 4 into `i32* undef`)
+
+  bb.4.cleanup:
+    liveins: $r4
+
+    $r0 = COPY killed renamable $r4
+    $sp = frame-destroy LDMIA_RET $sp, 14 /* CC::al */, $noreg, def $r4, def $pc, implicit $r0
+
+...