[DebugInfo][InstrRef] Correctly update DBG_PHIs during instruction scheduling

[jmorse]

Summary

When the instruction scheduler runs, it saves debug-instructions, and when writing the instruction referencing stuff I figured that the new debug instructions should be treated the same as DBG_VALUEs. However, it turns out there's a nuance:

• DBG_VALUEs can have their operands updated by the instruction scheduler, if it determines that the movement of an instruction changes what the DBG_VALUE refers to,
• DBG_LABELs and other unimportant meta instructions are just ignored.

This is a feature I was unaware of, and it's great; unfortunately it causes a crash with instruction referencing, because:

• DBG_PHIs should be treated like DBG_VALUEs, and have their operands updated,
• DBG_INSTR_REFs should be treated like DBG_LABELs, just ignored.

This patch rectifies that: DBG_PHIs are placed in the collection of instructions that need to have their operands updated, and UpdateDbgValue now deals with DBG_PHIs too. In ScheduleDAGInstrs::buildSchedGraph, DBG_INSTR_REFs are now treated like DBG_LABELs and are ignored for instruction scheduling purposes. The modified test checks the same behaviour for DBG_VALUEs and DBG_PHIs, that when a register is changed in scheduling (rcx -> r8), both DBG_VALUEs and DBG_PHIs are updated.

(Someday I think we'll need to have the instruction scheduler become more debug-info aware, to avoid re-ordering assignments, but it is not that day).

[StephenTozer]

Fix looks relatively trivial, test looks good, LGTM.

llvm/include/llvm/CodeGen/AntiDepBreaker.h

public:
   /// Update liveness information to account for the current
   /// instruction, which will not be scheduled.
   virtual void Observe(MachineInstr &MI, unsigned Count,
                        unsigned InsertPosIndex) = 0;
 
   /// Finish anti-dep breaking for a basic block.
   virtual void FinishBlock() = 0;
 
-  /// Update DBG_VALUE if dependency breaker is updating
+  /// Update DBG_VALUE or DBG_PHI if dependency breaker is updating
   /// other machine instruction to use NewReg.
   void UpdateDbgValue(MachineInstr &MI, unsigned OldReg, unsigned NewReg) {
-    assert(MI.isDebugValue() && "MI is not DBG_VALUE!");
+    if (MI.isDebugValue()) {
       if (MI.getDebugOperand(0).isReg() &&
           MI.getDebugOperand(0).getReg() == OldReg)
         MI.getDebugOperand(0).setReg(NewReg);
+    } else if (MI.isDebugPHI()) {
+      if (MI.getOperand(0).isReg() &&
+          MI.getOperand(0).getReg() == OldReg)
+        MI.getOperand(0).setReg(NewReg);
+    } else {
+      llvm_unreachable("MI is not DBG_VALUE / DBG_PHI!");
+    }
   }
 
   /// Update all DBG_VALUE instructions that may be affected by the dependency
   /// breaker's update of ParentMI to use NewReg.
   void UpdateDbgValues(const DbgValueVector &DbgValues, MachineInstr *ParentMI,
                        unsigned OldReg, unsigned NewReg) {
     // The following code is dependent on the order in which the DbgValues are
     // constructed in ScheduleDAGInstrs::buildSchedGraph.

llvm/lib/CodeGen/ScheduleDAGInstrs.cpp

void ScheduleDAGInstrs::buildSchedGraph(AAResults *AA,
   for (MachineBasicBlock::iterator MII = RegionEnd, MIE = RegionBegin;
        MII != MIE; --MII) {
     MachineInstr &MI = *std::prev(MII);
     if (DbgMI) {
       DbgValues.push_back(std::make_pair(DbgMI, &MI));
       DbgMI = nullptr;
     }
 
-    if (MI.isDebugValue() || MI.isDebugRef() || MI.isDebugPHI()) {
+    if (MI.isDebugValue() || MI.isDebugPHI()) {
       DbgMI = &MI;
       continue;
     }
-    if (MI.isDebugLabel())
-      continue;
 
-    if (MI.isPseudoProbe())
+    if (MI.isDebugLabel() || MI.isDebugRef() || MI.isPseudoProbe())
       continue;
 
     SUnit *SU = MISUnitMap[&MI];
     assert(SU && "No SUnit mapped to this MI");
 
     if (RPTracker) {
       RegisterOperands RegOpers;
       RegOpers.collect(MI, *TRI, MRI, TrackLaneMasks, false);

llvm/test/CodeGen/X86/post-ra-sched-with-debug.mir

 # RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=btver2 -run-pass=post-RA-sched -o - %s | FileCheck %s
+# RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=btver2 -run-pass=post-RA-sched -o - %s -experimental-debug-variable-locations| FileCheck %s
 
-# Test that multiple DBG_VALUE's following an instruction whose register needs
-# to be changed during the post-RA scheduler pass are updated correctly.
+# Test that multiple DBG_VALUE's and DBG_PHIs following an instruction whose
+# register needs # to be changed during the post-RA scheduler pass are updated
+# correctly.
 
 # Test case was derived from the output from the following command and
-# the source code below:
+# the source code below. DBG_PHIs added manually later:
 #
 #   clang -S -emit-llvm -target x86_64 -march=btver2 -O2 -g -o - <srcfile> |
 #   llc -stop-before=post-RA-sched -o -
 #
 # Source code reduced from the original 8MB source file:
 #
 # struct a;
 # class b {

 body:             |
   bb.0:
     successors: %bb.3, %bb.2
     liveins: $esi, $rdi, $r14, $rbx, $rbp
 
     ; CHECK:      [[REGISTER:\$r[a-z0-9]+]] = LEA64r {{\$r[a-z0-9]+}}, 1, $noreg, -20, $noreg
     ; CHECK-NEXT: DBG_VALUE [[REGISTER]], $noreg, ![[J_VAR]], !DIExpression(), debug-location ![[J_LOC]]
     ; CHECK-NEXT: DBG_VALUE [[REGISTER]], $noreg, ![[I_VAR]], !DIExpression(), debug-location ![[I_LOC]]
+    ; CHECK-NEXT: DBG_PHI [[REGISTER]], 0
+    ; CHECK-NEXT: DBG_PHI [[REGISTER]], 1
 
     frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
     CFI_INSTRUCTION def_cfa_offset 16
     CFI_INSTRUCTION offset $rbp, -16
     $rbp = frame-setup MOV64rr $rsp
     CFI_INSTRUCTION def_cfa_register $rbp
     frame-setup PUSH64r killed $r14, implicit-def $rsp, implicit $rsp
     frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
bb.0:
     $edx = XOR32rr undef $edx, undef $edx, implicit-def dead $eflags, implicit-def $rdx
     TEST64rr $rcx, $rcx, implicit-def $eflags
     $esi = MOV32ri @o, implicit-def $rsi
     $rsi = CMOV64rr killed $rsi, $rdx, 5, implicit killed $eflags
     $rsi = OR64rr killed $rsi, killed $rcx, implicit-def $eflags
     $rcx = LEA64r $rbp, 1, $noreg, -20, $noreg
     DBG_VALUE $rcx, $noreg, !46, !17, debug-location !48
     DBG_VALUE $rcx, $noreg, !39, !17, debug-location !44
+    DBG_PHI $rcx, 0
+    DBG_PHI $rcx, 1
     DBG_VALUE $rbp, -20, !29, !17, debug-location !36
     $rcx = CMOV64rr killed $rcx, killed $rdx, 5, implicit killed $eflags
     $rcx = OR64rr killed $rcx, killed $rsi, implicit-def dead $eflags
     $rdx = MOVSX64rm32 $rbx, 1, $noreg, 0, $noreg :: (load (s32), align 8)
+    DBG_INSTR_REF 1, 0, !46, !17, debug-location !48
+    DBG_INSTR_REF 2, 0, !39, !17, debug-location !44
     TEST32mr killed $rcx, 4, killed $rdx, 0, $noreg, killed $eax, implicit-def $eflags :: (load (s32))
     JCC_1 %bb.2, 5, implicit $eflags
     JMP_1 %bb.3
 
   bb.1:
     successors: %bb.2
     liveins: $rbx, $rbp