[WIP][DebugInfo][MachineSink] Preserve DBG_VALUE subregisters when sinking

[jmorse]

Summary

When sinking DBG_VALUEs, we were creating the new sunk DBG_VALUE by copying the old one. Unfortunately, it turns out that in the meantime copy propagation can significantly alter the old one, for example by changing the subregister it refers to.

Avoid any risk of sunk DBG_VALUEs picking up later subregister information, by storing the original (TM) subregister in the sink record.

This is WIP because the added test case _used_ to be failing, but now isn't for some reason. I'd like to share it with @uabelho who's reporting this problem happening downstream, to see whether I'm barking up the right tree before analysing further.

[uabelho]

I think you're on the right track. My mir-reproducer passes with this patch, so now I get the wanted

%6:an32_rn_pairs = COPY %5.hiAcc
%8:an32_0_7 = COPY %6
DBG_VALUE %8, [...]

instead of the previous

%6:an32_rn_pairs = COPY %5.hiAcc
%8:an32_0_7 = COPY %6
DBG_VALUE %8.hiAcc:an32_0_7, [...]

llvm/lib/CodeGen/MachineSink.cpp

class MachineSinking : public MachineFunctionPass {
     /// Record of DBG_VALUE uses of vregs in a block, so that we can identify
     /// debug instructions to sink.
     SmallDenseMap<unsigned, TinyPtrVector<SeenDbgUser>> SeenDbgUsers;
 
     /// Record of debug variables that have had their locations set in the
     /// current block.
     DenseSet<DebugVariable> SeenDbgVars;
 
+    // A single DBG_VALUE that is to be sunk, and its original subregister.
+    using SinkingVar = std::pair<MachineInstr *, unsigned>;
+
     /// A set of DBG_VALUEs, indexed by their DebugVariable. Refers to the
     /// original / non-sunk DBG_VALUE, which should be cloned to the final
-    /// sunk location.
-    using SinkingVarSet = DenseMap<DebugVariable, MachineInstr *>;
+    /// sunk location. The subregister the DBG_VALUE originally held is
+    /// stored too, as other salvage operations may change it.
+    using SinkingVarSet = DenseMap<DebugVariable, SinkingVar>;
 
     /// Record of variable locations to re-create after the sinking of a
     /// vreg definition completes.
     struct SunkDebugDef {
       SinkingVarSet InstsToSink; /// Set of DBG_VALUEs to recreate.
       MachineInstr *MI;          /// Location to place DBG_VALUEs.
     };
 
for (auto &Iterator : SunkDebugDefs) {
 
     MachineBasicBlock::iterator DestLoc = Def.MI->getIterator();
     MachineBasicBlock *MBB = Def.MI->getParent();
 
     // Place DBG_VALUEs immediately after the sunk instruction.
     ++DestLoc;
 
     // Collect the DBG_VALUEs being sunk and put them in a deterministic order.
-    using VarInstPair = std::pair<DebugVariable, MachineInstr *>;
+    using VarInstPair = std::pair<DebugVariable, SinkingVar>;
     SmallVector<VarInstPair, 16> InstsToSink;
     for (auto &SunkLoc : Def.InstsToSink)
       InstsToSink.push_back(SunkLoc);
     llvm::sort(InstsToSink);
 
     for (auto &SunkLoc : InstsToSink) {
-      MachineInstr *NewDbgMI = MF.CloneMachineInstr(SunkLoc.second);
+      SinkingVar ToSink = SunkLoc.second;
+      MachineInstr *NewDbgMI = MF.CloneMachineInstr(ToSink.first);
       NewDbgMI->getOperand(0).setReg(VReg);
+      NewDbgMI->getOperand(0).setSubReg(ToSink.second);
       MBB->insert(DestLoc, NewDbgMI);
     }
   }
 }
 
 bool MachineSinking::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(MF.getFunction()))
     return false;
for (auto &User : Users) {
         continue;
 
       DebugVariable Var(DbgMI->getDebugVariable(), DbgMI->getDebugExpression(),
                         DbgMI->getDebugLoc()->getInlinedAt());
 
       // Debug users that don't pass any other DBG_VALUEs for this variable
       // can be sunk.
       if (!User.getInt())
-        SunkDef.InstsToSink.insert({Var, DbgMI});
+        SunkDef.InstsToSink.insert(
+            {Var, {DbgMI, DbgMI->getOperand(0).getSubReg()}});
     }
   }
 
   // After sinking, some debug users may not be dominated any more. If possible,
   // copy-propagate their operands. As it's expensive, don't do this if there's
   // no debuginfo in the program.
   if (MI.getMF()->getFunction().getSubprogram() && MI.isCopy())
     SalvageUnsunkDebugUsersOfCopy(MI, SuccToSinkTo);

llvm/test/DebugInfo/MIR/X86/machinesink.mir

--- |
   ; Stripped
   entry:
     br label %nou
   nou:
     br label %exit
   exit:
     ret void
   }
-  
+  define void @test7(i32* nocapture readonly %p) local_unnamed_addr !dbg !601 {
+  ; Stripped
+  entry:
+    br label %nou
+  nou:
+    br label %exit
+  exit:
+    ret void
+  }
+
   declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
 
   !llvm.dbg.cu = !{!1}
   !llvm.module.flags = !{!6, !7}
   !llvm.ident = !{!8}
 
   !0 = !DIGlobalVariableExpression(var: !DIGlobalVariable(name: "x", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true), expr: !DIExpression())
   !1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
--- |
   !401 = distinct !DISubprogram(name: "test5", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !402)
   !402 = !{!403}
   !403 = !DILocalVariable(name: "t", arg: 1, scope: !401, file: !2, line: 2, type: !12)
   !404 = !DILocation(line: 1, column: 1, scope: !401)
   !501 = distinct !DISubprogram(name: "test5", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !502)
   !502 = !{!503}
   !503 = !DILocalVariable(name: "u", arg: 1, scope: !501, file: !2, line: 2, type: !12)
   !504 = !DILocation(line: 1, column: 1, scope: !501)
+  !601 = distinct !DISubprogram(name: "test5", scope: !2, file: !2, line: 2, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !602)
+  !602 = !{!603}
+  !603 = !DILocalVariable(name: "v", arg: 1, scope: !601, file: !2, line: 2, type: !12)
+  !604 = !DILocation(line: 1, column: 1, scope: !601)
+
 
   ; CHECK: [[VARNUM:![0-9]+]] = !DILocalVariable(name: "p",
   ; CHECK: [[VAR2NUM:![0-9]+]] = !DILocalVariable(name: "q",
   ; CHECK: [[VAR3NUM:![0-9]+]] = !DILocalVariable(name: "r",
   ; CHECK: [[VAR4NUM:![0-9]+]] = !DILocalVariable(name: "s",
   ; CHECK: [[VAR5NUM:![0-9]+]] = !DILocalVariable(name: "t",
   ; CHECK: [[VAR6NUM:![0-9]+]] = !DILocalVariable(name: "u",
+  ; CHECK: [[VAR7NUM:![0-9]+]] = !DILocalVariable(name: "v",
 
 ...
 ---
 name:            Process
 tracksRegLiveness: true
 liveins:
   - { reg: '$rdi', virtual-reg: '%2' }
   - { reg: '$rsi', virtual-reg: '%2' }
bb.1.nou:
     ; CHECK-NEXT:  JMP_1
     %1:gr64 = ADD64ri8 %8, 4, implicit-def dead $eflags
     JMP_1 %bb.2.exit
 
   bb.2.exit:
     $rax = MOV64ri 1
     RET 0
 ...
+---
+name:            test7
+# CHECK-LABEL: name: test7
+tracksRegLiveness: true
+liveins:
+  - { reg: '$rdi', virtual-reg: '%2' }
+  - { reg: '$rsi', virtual-reg: '%2' }
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: bb.0.entry:
+    ; CHECK-NOT:   DBG_VALUE
+    successors: %bb.3
+    liveins: $rdi, $esi
+    JMP_1 %bb.3
+
+  bb.1:
+    successors: %bb.2, %bb.4
+    liveins: $rdi, $esi
+
+    ; Register %5 sinks through this block; the DBG_VALUE should become $noreg
+    ; CHECK-LABEL: bb.1:
+    ; CHECK:       DBG_VALUE $noreg, $noreg, [[VAR7NUM]], !DIExpression()
+
+    CMP32ri $esi, 1, implicit-def $eflags
+    DBG_VALUE %5, $noreg, !603, !17, debug-location !604
+    JCC_1 %bb.2, 4, implicit $eflags
+    JMP_1 %bb.4
+
+  bb.2:
+    successors: %bb.4
+
+    ; Both copies should sink into this block, and we should sink the DBG_VALUE
+    ; from %bb.1 into this one too. The DBG_VALUE should not pick up a
+    ; subregister along the way.
+    ; CHECK-LABEL: bb.2:
+    ; CHECK:       %[[COPYDEST1:[0-9]+]]:gr32 = COPY %[[COPYSRC1:[0-9]+]].sub_32bit
+    ; CHECK-NEXT: %[[COPYDEST2:[0-9]+]]:gr32 = COPY %[[COPYDEST1]]
+    ; CHECK-NEXT: DBG_VALUE %[[COPYDEST2]], $noreg, [[VAR7NUM]], !DIExpression()
+    ; CHECK-NEXT: ADD32ri
+
+    %1:gr32 = ADD32ri8 %5, 4, implicit-def dead $eflags
+    JMP_1 %bb.4
+
+  bb.3:
+    successors: %bb.1, %bb.4
+    liveins: $rdi, $esi
+
+    ; CHECK-LABEL: bb.3:
+    ; CHECK:       %[[COPYSRC1]]:gr64 = COPY $rdi
+    ; CHECK:       DBG_VALUE $noreg, $noreg, [[VAR7NUM]], !DIExpression()
+
+    %2:gr64 = COPY $rdi
+    %12:gr32 = COPY %2.sub_32bit
+    %5:gr32 = COPY %12
+    CMP32ri $esi, 0, implicit-def $eflags
+    DBG_VALUE %5, $noreg, !603, !17, debug-location !604
+    JCC_1 %bb.1, 4, implicit $eflags
+    JMP_1 %bb.4
+
+  bb.4:
+    liveins: $rdi, $esi
+    $eax = COPY killed $rdi
+    RET 0
+...