[DebugInstrRef][5/9] Substitute debug value numbers to handle optimisations

[jmorse]

Summary

This patch handles two optimisations, TwoAddressInstruction and X86's FixupLEAs pass, both of which optimise by re-creating instructions. For LEAs, various bits of arithmetic are better represented as LEAs on X86, while TwoAddressInstruction sometimes converts instrs into three address instructions if it's profitable. Both of these require substitutions to be created -- test that an entry is made in the MIR output. (Handling of this in LiveDebugValues comes later).

[aprantl]

Are there other targets that also need to be updated?

[jmorse]

In D85756#2211044, @aprantl wrote:

Are there other targets that also need to be updated?

Probably all of them; I'm aware of at least one other x86 pass that requires a similar action (x86-fixup-bw-insts) and MachineCSE will need it too. I haven't yet looked at non-x86 targets.

I don't think I wrote this down anywhere else, but I'd like this patch to be seen as a "proof of concept" that debug instruction-numbers can / should be transferred in this way. Comprehensively dealing with all the places this action is required is a large-ish problem, which I'd prefer to address in a different patch series if possible. That means that, were this series to land today, there would be some variable locations dropped; but the work is after all in an experimental state hidden behind a flag.

[jmorse]

I wrote:

were this series to land today, there would be some variable locations dropped

To avoid this sounding like there's a huge loss: my current testing with this series and the llvm-locstats script on stage2 clang shows no reduction in the "total [variable] availability" number, and a 2% fall in "PC ranges covered", which I would characterise as "most locations preserved with some corner cases still to address".

[jmorse]

Add another call to substituteDebugValuesForInst that I missed (line 681).

[jmorse]

Rebase for rG60433c63acb7 touching something near this patch, no real change.

[aprantl]

@vsk We really need a richer API for MBB::erase that takes extra arguments and forces you to think about this.

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

762

MF->makeDebugValueSubstitution({OldInstrNum, OldIdx}, {NewInstrNum, NewIdx}); ?

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp

if (LIS)
     LIS->ReplaceMachineInstrInMaps(*mi, *NewMI);
 
   if (NewMI->findRegisterUseOperand(RegB, false, TRI))
     // FIXME: Temporary workaround. If the new instruction doesn't
     // uses RegB, convertToThreeAddress must have created more
     // then one instruction.
     Sunk = sink3AddrInstruction(NewMI, RegB, mi);
 
+  // If the old instruction is debug value tracked, an update is required.
+  if (auto OldInstrNum = mi->peekDebugInstrNum()) {
+    // Sanity check.
+    assert(mi->getNumExplicitDefs() == 1);
+    assert(NewMI->getNumExplicitDefs() == 1);
+
+    // Find the old and new def location.
+    auto OldIt = mi->defs().begin();
+    auto NewIt = NewMI->defs().begin();
+    unsigned OldIdx = mi->getOperandNo(OldIt);
+    unsigned NewIdx = NewMI->getOperandNo(NewIt);
+
+    // Record that one def has been replaced by the other.
+    unsigned NewInstrNum = NewMI->getDebugInstrNum();
+    MF->makeDebugValueSubstitution(std::make_pair(OldInstrNum, OldIdx),
+                                   std::make_pair(NewInstrNum, NewIdx));
+  }
+
   MBB->erase(mi); // Nuke the old inst.
 
   if (!Sunk) {
     DistanceMap.insert(std::make_pair(NewMI, Dist));
     mi = NewMI;
     nmi = std::next(mi);
   }
   else

llvm/lib/Target/X86/X86FixupLEAs.cpp

if (OptIncDec && (Disp.getImm() == 1 || Disp.getImm() == -1)) {
       } else {
         NewMI = BuildMI(MBB, I, MI.getDebugLoc(), TII->get(NewOpcode), DestReg)
           .addReg(BaseReg).addImm(Disp.getImm());
       }
     }
   } else
     return false;
 
+  MBB.getParent()->substituteDebugValuesForInst(*I, *NewMI);
   MBB.erase(I);
   I = NewMI;
   return true;
 }
 
 void FixupLEAPass::processInstruction(MachineBasicBlock::iterator &I,
                                       MachineBasicBlock &MBB) {
   // Process a load, store, or LEA instruction.
void FixupLEAPass::seekLEAFixup(MachineOperand &p,
   MachineBasicBlock::iterator MBI = searchBackwards(p, I, MBB);
   if (MBI != MachineBasicBlock::iterator()) {
     MachineInstr *NewMI = postRAConvertToLEA(MBB, MBI);
     if (NewMI) {
       ++NumLEAs;
       LLVM_DEBUG(dbgs() << "FixLEA: Candidate to replace:"; MBI->dump(););
       // now to replace with an equivalent LEA...
       LLVM_DEBUG(dbgs() << "FixLEA: Replaced by: "; NewMI->dump(););
+      MBB.getParent()->substituteDebugValuesForInst(*MBI, *NewMI);
       MBB.erase(MBI);
       MachineBasicBlock::iterator J =
           static_cast<MachineBasicBlock::iterator>(NewMI);
       processInstruction(J, MBB);
     }
   }
 }
 
const MCInstrDesc &ADDri =
         TII->get(getADDriFromLEA(Opcode, Offset));
     const MachineOperand &SrcR = SrcR1 == DstR ? Base : Index;
     NewMI = BuildMI(MBB, I, MI.getDebugLoc(), ADDri, DstR)
                 .add(SrcR)
                 .addImm(Offset.getImm());
     LLVM_DEBUG(NewMI->dump(););
   }
   if (NewMI) {
+    MBB.getParent()->substituteDebugValuesForInst(*I, *NewMI);
     MBB.erase(I);
     I = NewMI;
   }
 }
 
 void FixupLEAPass::processInstrForSlow3OpLEA(MachineBasicBlock::iterator &I,
                                              MachineBasicBlock &MBB,
                                              bool OptIncDec) {
if (hasLEAOffset(Offset)) {
         unsigned NewOpc = getADDriFromLEA(MI.getOpcode(), Offset);
         NewMI = BuildMI(MBB, I, MI.getDebugLoc(), TII->get(NewOpc), DestReg)
                     .addReg(DestReg)
                     .add(Offset);
         LLVM_DEBUG(NewMI->dump(););
       }
     }
 
+    MBB.getParent()->substituteDebugValuesForInst(*I, *NewMI);
     MBB.erase(I);
     I = NewMI;
     return;
   }
 
   // Handle the rest of the cases with inefficient base register:
   assert(DestReg != BaseReg && "DestReg == BaseReg should be handled already!");
   assert(IsInefficientBase && "efficient base should be handled already!");
if (IsScale1 && !hasLEAOffset(Offset)) {
     LLVM_DEBUG(MI.getPrevNode()->dump(););
 
     unsigned NewOpc = getADDrrFromLEA(MI.getOpcode());
     NewMI = BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(NewOpc), DestReg)
                 .addReg(DestReg)
                 .add(Index);
     LLVM_DEBUG(NewMI->dump(););
 
+    MBB.getParent()->substituteDebugValuesForInst(*I, *NewMI);
     MBB.erase(I);
     I = NewMI;
     return;
   }
 
   // lea offset(%base,%index,scale), %dst =>
   // lea offset( ,%index,scale), %dst; add %base,%dst
   NewMI = BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(LEAOpcode))

llvm/test/DebugInfo/MIR/InstrRef/twoaddr-to-threeaddr-sub.mir

+# RUN: llc -run-pass=twoaddressinstruction -mtriple=x86_64-- -o - %s -experimental-debug-variable-locations | FileCheck %s
+# 
+# Test that a new instruction (LEA) is created when the two-addr add below is
+# converted to three address; and that an appropriate substitution is created.
+# Maybe at some point we'll normalise DBG_INSTR_REFs on output, but until then,
+# lets not.
+#
+# CHECK:      debugValueSubstitutions:
+# CHECK-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0 }
+#
+# CHECK:      LEA64_32r
+# CHECK-SAME: debug-instr-number 2
+#
+# CHECK:      DBG_INSTR_REF 1, 0
+---
+name:            test1
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr32 }
+  - { id: 1, class: gr32 }
+  - { id: 2, class: gr32 }
+liveins:
+  - { reg: '$edi', virtual-reg: '%0' }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $edi
+
+    %0:gr32 = COPY killed $edi
+    %1:gr32 = SHL32ri killed %0, 5, implicit-def dead $eflags
+    %2:gr32 = ADD32ri8_DB killed %1, 3, implicit-def dead $eflags, debug-instr-number 1
+    DBG_INSTR_REF 1, 0
+    $eax = COPY killed %2
+    RET 0, killed $eax
+
+...
+