[DebugInfo][InstrRef][3/4] Produce DBG_INSTR_REFs for all* variable locations

[jmorse]

Summary

This patch emits DBG_INSTR_REFs for the final two flavours of variable locations that couldn't be handled before: copies, and inter-block VRegs. There are still some variable locations that are represented by DBG_VALUE instructions, which are:

• Constants,
• Frame-index references,
• Physical registers.

The first two are independent of any optimisations that occur, and don't really need to change. DBG_VALUEs of physregs are still emitted for function parameters, and I haven't been brave enough to go anywhere near that situation. Again, such DBG_VALUEs aren't actually affected by optimisations due to their placement and their physreg operands never being optimised.

For variable locations that refer to values defined in different blocks, it's slightly tricky: such values are given cross-block virtual registers before instruction selection begins, but there isn't necessarily an instruction defining them available: I don't believe there's a guarantee on the order in which blocks are instruction-selected and emitted. To get around this: if we can't immediately identify the defining instruction of a VReg, emit a "half done" DBG_INSTR_REF, where the first operand identifies the VReg it's associated with. Then at a later point in ISel, after all blocks are emitted to MIR, find the defining instruction and mutate the DBG_INSTR_REF to refer to it. This is a hack; but either an ordering guarantee has to be put on block emission (which I imagine could involve every CodeGen test needing updating) or some kind of intermediate form is needed until all the information is available.

Copies: this is one of the things in the original RFC that I complained about: having the same value describable in multiple locations is unpleasent. I would like this instruction referencing work to put us in a position where we never refer to copy instructions at all. They wouldn't work well with the InstrRefBased LiveDebugValues implementation anyway, as copies don't define any values, they only move them.

The solution to this is the salvageCopySSA method added in MachineFunction. This interprets:

• COPYs
• SUBREG_TO_REG
• Anything identifiable by the targets isCopyInstr method

And follows chains of copies back to the defining instruction that they read from. This relies on an assumption: that any copies from physical registers originate from a preceeding instruction in the same block, or is a parameter. Take a look at the tests added in instr-ref-selectiondag.ll: due to the way SelectionDAG operates, whenever an instruction defs a physical register we copy it to a VReg; salvageCopySSA walks backwards from the COPY-from-rax to find the defining instruction. The exception is arguments: those never have a defining instruction, so we have to drop a DBG_PHI at the entry block. (Right now we drop one DBG_PHI for every variable that reads an Argument; I'll revise this with a cache in a future revision of this patch).

We might end up having to have a similar "Salvage copy" operation for after regalloc, but we'll cross that bridge later.

[jmorse]

(Rebase due to me fiddling with the signature of substituteDebugValuesForInst on master).

[Orlando]

I've taken a stab at reviewing this. I especially appreciate and enjoy the detailed comments and patch description. I've left some nits and questions inline but otherwise it looks good IMO.

llvm/include/llvm/CodeGen/MachineFunction.h

510

Does this need to return an Optional? As far as I can tell salvageCopySSA never returns None, and the only callsite doesn't check whether the returned result has a value or not.

llvm/lib/CodeGen/MachineFunction.cpp

1039–1040

I'm struggling to follow this part. When you say "only remember truncations or offsets" am I right in thinking that setting SubReg = 0 causes some kind of mismatch that is picked up and handled by LiveDebugValues/InstrRefBasedImpl in D88894?

1133–1140

Is it worth putting the create-a-dbg-phi code into a function that is shared between these patches? No strong opinion on this.

1139

nit: I think you can do Builder.addReg(State.first, RegState::Debug) on line 1135 instead.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

776

SGTM. I make a comment later about moving the SelectionDAGISel half-done-dbg-instr-fixup code into its own function. If you do that you could change and patch it up later in SelectionDAG to and patch it up later in <function-name>, which might make it easier to quickly find the relevant code in the future.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.h

121–128

It might be useful to split the factoring-out of this DBG_VALUE building code from EmitDbgValue into a separate NFC patch IMO, but it's a probably not too important.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

646–700

I think it would be good to move this fixup-half-done-instr-refs code into its own function. It would reduce clutter in runOnMachineFunction and you could then also add a comment to the EmitHalfDoneInstrRef lambda mentioning it so that it's easy to find from there. wdyt?

llvm/test/DebugInfo/X86/instr-ref-selectiondag.ll

83

Sorry if this is mentioned elsewhere - why are you explicitly not testing the location (position?) of the isntr-refs here?

[jmorse]

llvm/include/llvm/CodeGen/MachineFunction.h

510

/me scratches head. I guess I prototyped this with it returning an optional, then revised it to assert if any value wasn't available.

I'll update this to not return an optional.

llvm/lib/CodeGen/MachineFunction.cpp

1039–1040

Probably because it's a cop-out -- if I compile this:

void ext(int b);
int foo(char a)
{
    ext(a);
    return 0;
}

I get DBG_VALUE $edi for b rather than DBG_VALUE $al, which has encouraged me to be fairly lax about accurate widening for parameters. Off the top of my head, I can't think of any reason to omit the subregister information, I'll run some experiments to see what effect it has.

[jmorse]

Reshuffle this patch -- the "fixup half done instruction references" code moves to MachineFunction::finalizeDebugInstrRefs, and I've removed the ignore-widening-subreg-substitutions code. It didn't have any significant effect on variable locations, I think it was just dropping information.

Paging @StephenTozer , I've reshuffled debug instruction emission a little after the variadic-locations work landed -- I've moved single operand emission to InstrEmitter::EmitDbgValueFromSingleOp, and no-location emission to InstrEmitter::EmitDbgNoLocation. Now, empty variadic variable locations cannonicalise to DBG_VALUE $noreg which casuses a few test changes. arg-dbg-value-list.ll caught my eye -- can variadic locations not recover unused argument values right now? (I imagine this is tied up in the nest of special casing for arguments in SelectionDAG).

[jmorse]

(Re upload with the SubReg = 0; bit actually taken out, earlier diff was a case of mistaken identity)

[jmorse]

...and again with context this time...

[Orlando]

In D88896#2858425, @jmorse wrote:

Reshuffle this patch -- the "fixup half done instruction references" code moves to MachineFunction::finalizeDebugInstrRefs, and I've removed the ignore-widening-subreg-substitutions code. It didn't have any significant effect on variable locations, I think it was just dropping information.

SGTM

Paging @StephenTozer , I've reshuffled debug instruction emission a little after the variadic-locations work landed -- I've moved single operand emission to InstrEmitter::EmitDbgValueFromSingleOp, and no-location emission to InstrEmitter::EmitDbgNoLocation. Now, empty variadic variable locations cannonicalise to DBG_VALUE $noreg which casuses a few test changes. arg-dbg-value-list.ll caught my eye -- can variadic locations not recover unused argument values right now? (I imagine this is tied up in the nest of special casing for arguments in SelectionDAG).

I've left an inline comment about the empty variable location canonicalization (why it was the way it was before, plus a nit).

LGTM - though there are some clang-format nits plus my inline nit that need addressing before this lands.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

685–686

You can remove this bit of code now that you've hoisted the if (SD->isInvalidated()) outside of the if (SD->isVariadic()).

FWIW invalidated DBG_VALUE_LIST were emitted in this way ($noreg all the operands rather than emit a DBG_VALUE $noreg) with the aim of helping track down debug-info issues - the thinking being that the number of operands might hint at the identity of the DBG_VALUE when looking at print-after-all traces. I don't know whether that's a compelling enough reason to keep that way so I thought I'd just mention it in case you didn't' know, and let you decide.

llvm/include/llvm/CodeGen/MachineFunction.h

public:
   /// a modified instruction type, flags, or otherwise. An example: X86 moves
   /// are sometimes transformed into equivalent LEAs.
   /// If the two instructions are not the same opcode, limit which operands to
   /// examine for substitutions to the first N operands by setting
   /// \p MaxOperand.
   void substituteDebugValuesForInst(const MachineInstr &Old, MachineInstr &New,
                                     unsigned MaxOperand = UINT_MAX);
 
+  /// Find the underlying  defining instruction / operand for a COPY instruction
+  /// while in SSA form. Copies do not actually define values -- they move them
+  /// between registers. Labelling a COPY-like instruction with an instruction
+  /// number is to be avoided as it makes value numbers non-unique later in
+  /// compilation. This method follows the definition chain for any sequence of
+  /// COPY-like instructions to find whatever non-COPY-like instruction defines
+  /// the copied value; or for parameters, creates a DBG_PHI on entry.
+  /// May insert instructions into the entry block!
+  /// \p MI The copy-like instruction to salvage.
+  /// \returns Either an instruction/operand pair identifying the defining
+  ///          value, or None if nothing could be recovered.
+  Optional<DebugInstrOperandPair> salvageCopySSA(MachineInstr &MI);
+
   MachineFunction(Function &F, const LLVMTargetMachine &Target,
                   const TargetSubtargetInfo &STI, unsigned FunctionNum,
                   MachineModuleInfo &MMI);
   MachineFunction(const MachineFunction &) = delete;
   MachineFunction &operator=(const MachineFunction &) = delete;
   ~MachineFunction();
 
   /// Reset the instance as if it was just created.

llvm/lib/CodeGen/MachineFunction.cpp

for (unsigned int I = 0; I < Old.getNumOperands(); ++I) {
     assert(NewMO.isDef());
 
     unsigned NewInstrNum = New.getDebugInstrNum();
     makeDebugValueSubstitution(std::make_pair(OldInstrNum, I),
                                std::make_pair(NewInstrNum, I));
   }
 }
 
+auto MachineFunction::salvageCopySSA(MachineInstr &MI)
+    -> Optional<DebugInstrOperandPair> {
+  MachineRegisterInfo &MRI = getRegInfo();
+  const TargetRegisterInfo &TRI = *MRI.getTargetRegisterInfo();
+  const TargetInstrInfo &TII = *getSubtarget().getInstrInfo();
+
+  // Chase the value read by a copy-like instruction back to the instruction
+  // that ultimately _defines_ that value. This may pass:
+  //  * Through multiple intermediate copies, including subregister moves /
+  //    copies,
+  //  * Copies from physical registers that must then be traced back to the
+  //    defining instruction,
+  //  * Or, physical registers may be live-in to (only) the entry block, which
+  //    requires a DBG_PHI to be created.
+  // We can pursue this problem in that order: trace back through copies,
+  // optionally through a physical register, to a defining instruction. We
+  // should never move from physreg to vreg. As we're still in SSA form, no need
+  // to worry about partial definitions of registers.
+
+  // Helper lambda to interpret a copy-like instruction. Takes instruction,
+  // returns the register read and any subregister identifying which part is
+  // read.
+  auto GetRegAndSubreg =
+      [&](const MachineInstr &Cpy) -> std::pair<Register, unsigned> {
+    Register NewReg, OldReg;
+    unsigned SubReg;
+    if (Cpy.isCopy()) {
+      OldReg = Cpy.getOperand(0).getReg();
+      NewReg = Cpy.getOperand(1).getReg();
+      SubReg = Cpy.getOperand(1).getSubReg();
+    } else if (Cpy.isSubregToReg()) {
+      OldReg = Cpy.getOperand(0).getReg();
+      NewReg = Cpy.getOperand(2).getReg();
+      SubReg = Cpy.getOperand(3).getImm();
+    } else {
+      auto CopyDetails = *TII.isCopyInstr(Cpy);
+      const MachineOperand &Src = *CopyDetails.Source;
+      const MachineOperand &Dest = *CopyDetails.Destination;
+      OldReg = Dest.getReg();
+      NewReg = Src.getReg();
+      SubReg = Src.getSubReg();
+    }
+
+    // Don't bother to record the subregister if the number of bits is the same
+    // or widens; only remember truncations or offsets.
+    unsigned NewRegBits = TRI.getRegSizeInBits(NewReg, MRI);
+    unsigned OldRegBits = TRI.getRegSizeInBits(OldReg, MRI);
+    unsigned Offset = 0;
+    if (SubReg)
+      Offset = TRI.getSubRegIdxOffset(SubReg);
+    if (OldRegBits >= NewRegBits || Offset)
+      SubReg = 0;
+
+    return {NewReg, SubReg};
+  };
+
+  // First seek either the defining instruction, or a copy from a physreg.
+  // During search, the current state is the current copy instruction, and which
+  // register we've read. Accumulate qualifying subregisters into SubregsSeen;
+  // deal with those later.
+  auto State = GetRegAndSubreg(MI);
+  auto CurInst = MI.getIterator();
+  SmallVector<unsigned, 4> SubregsSeen;
+  while (true) {
+    // If we've found a copy from a physreg, first portion of search is over.
+    if (!State.first.isVirtual())
+      break;
+
+    // Record any subregister qualifier.
+    if (State.second)
+      SubregsSeen.push_back(State.second);
+
+    assert(MRI.hasOneDef(State.first));
+    MachineInstr &Inst = *MRI.def_begin(State.first)->getParent();
+    CurInst = Inst.getIterator();
+
+    // Any non-copy instruction is the defining instruction we're seeking.
+    if (!Inst.isCopyLike() && !TII.isCopyInstr(Inst))
+      break;
+    State = GetRegAndSubreg(Inst);
+  };
+
+  // Helper lambda to apply additional subregister substitutions to a known
+  // instruction/operand pair. Adds new (fake) substitutions so that we can
+  // record the subregister. FIXME: this isn't very space efficient if multiple
+  // values are tracked back through the same copies; cache something later.
+  auto ApplySubregisters =
+      [&](DebugInstrOperandPair P) -> DebugInstrOperandPair {
+    for (unsigned Subreg : reverse(SubregsSeen)) {
+      // Fetch a new instruction number, not attached to an actual instruction.
+      unsigned NewInstrNumber = getNewDebugInstrNum();
+      // Add a substitution from the "new" number to the known one, with a
+      // qualifying subreg.
+      makeDebugValueSubstitution({NewInstrNumber, 0}, P, Subreg);
+      // Return the new number; to find the underlying value, consumers need to
+      // deal with the qualifying subreg.
+      P = {NewInstrNumber, 0};
+    }
+    return P;
+  };
+
+  // If we managed to find the defining instruction after COPYs, return an
+  // instruction / operand pair after adding subregister qualifiers.
+  if (State.first.isVirtual()) {
+    // Virtual register def -- we can just look up where this happens.
+    MachineInstr *Inst = MRI.def_begin(State.first)->getParent();
+    for (auto &MO : Inst->operands()) {
+      if (!MO.isReg() || !MO.isDef() || MO.getReg() != State.first)
+        continue;
+      return ApplySubregisters(
+          {Inst->getDebugInstrNum(), Inst->getOperandNo(&MO)});
+    }
+
+    llvm_unreachable("Vreg def with no corresponding operand?");
+  }
+
+  // Our search ended in a copy from a physreg: walk back up the function
+  // looking for whatever defines the physreg.
+  assert(CurInst->isCopyLike() || TII.isCopyInstr(*CurInst));
+  State = GetRegAndSubreg(*CurInst);
+  Register RegToSeek = State.first;
+
+  auto RMII = CurInst->getReverseIterator();
+  auto PrevInstrs = make_range(RMII, CurInst->getParent()->instr_rend());
+  for (auto &ToExamine : PrevInstrs) {
+    for (auto &MO : ToExamine.operands()) {
+      // Test for operand that defines something aliasing RegToSeek.
+      if (!MO.isReg() || !MO.isDef() ||
+          !TRI.regsOverlap(RegToSeek, MO.getReg()))
+        continue;
+
+      return ApplySubregisters(
+          {ToExamine.getDebugInstrNum(), ToExamine.getOperandNo(&MO)});
+    }
+  }
+
+  // We reached the start of the block before finding a defining instruction.
+  // It must be an argument: assert that this is the entry block, and produce
+  // a DBG_PHI.
+  assert(!State.first.isVirtual());
+  MachineBasicBlock &TargetBB = *CurInst->getParent();
+  assert(&*TargetBB.getParent()->begin() == &TargetBB);
+
+  // Create DBG_PHI for specified physreg.
+  auto Builder = BuildMI(TargetBB, TargetBB.getFirstNonPHI(), DebugLoc(),
+                         TII.get(TargetOpcode::DBG_PHI));
+  Builder.addReg(State.first);
+  unsigned NewNum = getNewDebugInstrNum();
+  Builder.addImm(NewNum);
+  MachineInstr *NewInst = Builder;
+  NewInst->getOperand(0).setIsDebug(true);
+  return ApplySubregisters({NewNum, 0u});
+}
+
 /// \}
 
 //===----------------------------------------------------------------------===//
 //  MachineJumpTableInfo implementation
 //===----------------------------------------------------------------------===//
 
 /// Return the size of each entry in the jump table.
 unsigned MachineJumpTableInfo::getEntrySize(const DataLayout &TD) const {

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.h

public:
   /// (which do not go into the machine instrs.)
   static unsigned CountResults(SDNode *Node);
 
   /// EmitDbgValue - Generate machine instruction for a dbg_value node.
   ///
   MachineInstr *EmitDbgValue(SDDbgValue *SD,
                              DenseMap<SDValue, Register> &VRBaseMap);
 
-  /// Attempt to emit a dbg_value as a DBG_INSTR_REF. May fail and return
-  /// nullptr, in which case we fall back to plain EmitDbgValue.
+  /// Emit a dbg_value as a DBG_INSTR_REF. May produce DBG_VALUE $noreg instead
+  /// if there is no variable location; alternately a half-formed DBG_INSTR_REF
+  /// that refers to a virtual register and is corrected later in isel.
   MachineInstr *EmitDbgInstrRef(SDDbgValue *SD,
                                 DenseMap<SDValue, Register> &VRBaseMap);
 
+  /// Emit a DBG_VALUE $noreg, indicating a variable has no location.
+  MachineInstr *EmitDbgNoLocation(SDDbgValue *SD);
+
+  /// Emit a DBG_VALUE %stack, referring to a frame index.
+  MachineInstr *EmitDbgFrameIndex(SDDbgValue *SD);
+
+  /// Emit a DBG_VALUE of some constant value.
+  MachineInstr *EmitDbgConst(SDDbgValue *SD);
+
   /// Generate machine instruction for a dbg_label node.
   MachineInstr *EmitDbgLabel(SDDbgLabel *SD);
 
   /// EmitNode - Generate machine code for a node and needed dependencies.
   ///
   void EmitNode(SDNode *Node, bool IsClone, bool IsCloned,
                 DenseMap<SDValue, Register> &VRBaseMap) {
     if (Node->isMachineOpcode())

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

InstrEmitter::EmitDbgValue(SDDbgValue *SD,
                            DenseMap<SDValue, Register> &VRBaseMap) {
   MDNode *Var = SD->getVariable();
   MDNode *Expr = SD->getExpression();
   DebugLoc DL = SD->getDebugLoc();
   assert(cast<DILocalVariable>(Var)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");
 
   SD->setIsEmitted();
 
-  if (SD->isInvalidated()) {
-    // An invalidated SDNode must generate an undef DBG_VALUE: although the
-    // original value is no longer computed, earlier DBG_VALUEs live ranges
-    // must not leak into later code.
-    auto MIB = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE));
-    MIB.addReg(0U);
-    MIB.addReg(0U, RegState::Debug);
-    MIB.addMetadata(Var);
-    MIB.addMetadata(Expr);
-    return &*MIB;
-  }
+  if (SD->isInvalidated())
+    return EmitDbgNoLocation(SD);
 
   // Attempt to produce a DBG_INSTR_REF if we've been asked to.
   if (EmitDebugInstrRefs)
     if (auto *InstrRef = EmitDbgInstrRef(SD, VRBaseMap))
       return InstrRef;
 
-  if (SD->getKind() == SDDbgValue::FRAMEIX) {
-    // Stack address; this needs to be lowered in target-dependent fashion.
-    // EmitTargetCodeForFrameDebugValue is responsible for allocation.
-    auto FrameMI = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
-                       .addFrameIndex(SD->getFrameIx());
-    if (SD->isIndirect())
-      // Push [fi + 0] onto the DIExpression stack.
-      FrameMI.addImm(0);
-    else
-      // Push fi onto the DIExpression stack.
-      FrameMI.addReg(0);
-    return FrameMI.addMetadata(Var).addMetadata(Expr);
-  }
+  // Frame indexes don't need to use the register map.
+  if (SD->getKind() == SDDbgValue::FRAMEIX)
+    return EmitDbgFrameIndex(SD);
+
   // Otherwise, we're going to create an instruction here.
   const MCInstrDesc &II = TII->get(TargetOpcode::DBG_VALUE);
   MachineInstrBuilder MIB = BuildMI(*MF, DL, II);
   if (SD->getKind() == SDDbgValue::SDNODE) {
     SDNode *Node = SD->getSDNode();
     SDValue Op = SDValue(Node, SD->getResNo());
     // It's possible we replaced this SDNode with other(s) and therefore
     // didn't generate code for it.  It's better to catch these cases where
InstrEmitter::EmitDbgValue(SDDbgValue *SD,
   MIB.addMetadata(Expr);
 
   return &*MIB;
 }
 
 MachineInstr *
 InstrEmitter::EmitDbgInstrRef(SDDbgValue *SD,
                               DenseMap<SDValue, Register> &VRBaseMap) {
-  // Instruction referencing is still in a prototype state: for now we're only
-  // going to support SDNodes within a block. Copies are not supported, they
-  // don't actually define a value.
-  if (SD->getKind() != SDDbgValue::SDNODE)
-    return nullptr;
+  MDNode *Var = SD->getVariable();
+  MDNode *Expr = SD->getExpression();
+  DebugLoc DL = SD->getDebugLoc();
+  const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_INSTR_REF);
+
+  // Handle variable locations that don't actually depend on the instructions
+  // in the program: constants and stack locations.
+  if (SD->getKind() == SDDbgValue::FRAMEIX)
+    return EmitDbgFrameIndex(SD);
+
+  if (SD->getKind() == SDDbgValue::CONST)
+    return EmitDbgConst(SD);
+
+  // It may not be immediately possible to identify the MachineInstr that
+  // defines a VReg, it can depend for example on the order blocks are
+  // emitted in. When this happens, or when further analysis is needed later,
+  // produce an instruction like this:
+  //
+  //    DBG_INSTR_REF %0:gr64, 0, !123, !456
+  //
+  // i.e., point the instruction at the vreg, and patch it up later in
+  // SelectionDAG when more information is available.
+  auto EmitHalfDoneInstrRef = [&](unsigned VReg) -> MachineInstr * {
+    auto MIB = BuildMI(*MF, DL, RefII);
+    MIB.addReg(VReg);
+    MIB.addImm(0);
+    MIB.addMetadata(Var);
+    MIB.addMetadata(Expr);
+    return MIB;
+  };
+
+  // Try to find both the defined register and the instruction defining it.
+  MachineInstr *DefMI = nullptr;
+  unsigned VReg;
+
+  if (SD->getKind() == SDDbgValue::VREG) {
+    VReg = SD->getVReg();
 
+    // No definition means that block hasn't been emitted yet. Leave a vreg
+    // reference to be fixed later.
+    if (!MRI->hasOneDef(VReg))
+      return EmitHalfDoneInstrRef(VReg);
+
+    DefMI = &*MRI->def_instr_begin(VReg);
+  } else {
+    assert(SD->getKind() == SDDbgValue::SDNODE);
+    // Look up the corresponding VReg for the given SDNode, if any.
     SDNode *Node = SD->getSDNode();
     SDValue Op = SDValue(Node, SD->getResNo());
     DenseMap<SDValue, Register>::iterator I = VRBaseMap.find(Op);
+    // No VReg -> produce a DBG_VALUE $noreg instead.
     if (I==VRBaseMap.end())
-    return nullptr; // undef value: let EmitDbgValue produce a DBG_VALUE $noreg.
-
-  MDNode *Var = SD->getVariable();
-  MDNode *Expr = SD->getExpression();
-  DebugLoc DL = SD->getDebugLoc();
+      return EmitDbgNoLocation(SD);
 
     // Try to pick out a defining instruction at this point.
-  unsigned VReg = getVR(Op, VRBaseMap);
-  MachineInstr *ResultInstr = nullptr;
+    VReg = getVR(Op, VRBaseMap);
 
-  // No definition corresponds to scenarios where a vreg is live-in to a block,
-  // and doesn't have a defining instruction (yet). This can be patched up
-  // later; at this early stage of implementation, fall back to using DBG_VALUE.
+    // Again, if there's no instruction defining the VReg right now, fix it up
+    // later.
     if (!MRI->hasOneDef(VReg))
-    return nullptr;
+      return EmitHalfDoneInstrRef(VReg);
 
-  MachineInstr &DefMI = *MRI->def_instr_begin(VReg);
-  // Some target specific opcodes can become copies. As stated above, we're
-  // ignoring those for now.
-  if (DefMI.isCopy() || DefMI.getOpcode() == TargetOpcode::SUBREG_TO_REG)
-    return nullptr;
+    DefMI = &*MRI->def_instr_begin(VReg);
+  }
+
+  // Avoid copy like instructions: they don't define values, only move them.
+  // Leave a virtual-register reference until it can be fixed up later, to find
+  // the underlying value definition.
+  if (DefMI->isCopyLike() || TII->isCopyInstr(*DefMI))
+    return EmitHalfDoneInstrRef(VReg);
 
-  const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_INSTR_REF);
   auto MIB = BuildMI(*MF, DL, RefII);
 
-  // Find the operand which defines the specified VReg.
+  // Find the operand number which defines the specified VReg.
   unsigned OperandIdx = 0;
-  for (const auto &MO : DefMI.operands()) {
+  for (const auto &MO : DefMI->operands()) {
     if (MO.isReg() && MO.isDef() && MO.getReg() == VReg)
       break;
     ++OperandIdx;
   }
-  assert(OperandIdx < DefMI.getNumOperands());
+  assert(OperandIdx < DefMI->getNumOperands());
 
   // Make the DBG_INSTR_REF refer to that instruction, and that operand.
-  unsigned InstrNum = DefMI.getDebugInstrNum();
+  unsigned InstrNum = DefMI->getDebugInstrNum();
   MIB.addImm(InstrNum);
   MIB.addImm(OperandIdx);
   MIB.addMetadata(Var);
   MIB.addMetadata(Expr);
-  ResultInstr = &*MIB;
-  return ResultInstr;
+  return &*MIB;
+}
+
+MachineInstr *
+InstrEmitter::EmitDbgNoLocation(SDDbgValue *SD) {
+  // An invalidated SDNode must generate an undef DBG_VALUE: although the
+  // original value is no longer computed, earlier DBG_VALUEs live ranges
+  // must not leak into later code.
+  MDNode *Var = SD->getVariable();
+  MDNode *Expr = SD->getExpression();
+  DebugLoc DL = SD->getDebugLoc();
+  auto MIB = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE));
+  MIB.addReg(0U);
+  MIB.addReg(0U, RegState::Debug);
+  MIB.addMetadata(Var);
+  MIB.addMetadata(Expr);
+  return &*MIB;
+}
+
+MachineInstr *
+InstrEmitter::EmitDbgFrameIndex(SDDbgValue *SD) {
+  assert (SD->getKind() == SDDbgValue::FRAMEIX);
+  MDNode *Var = SD->getVariable();
+  MDNode *Expr = SD->getExpression();
+  DebugLoc DL = SD->getDebugLoc();
+
+  // Emit a DBG_VALUE with a FrameIndex operand.
+  auto FrameMI = BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE))
+                     .addFrameIndex(SD->getFrameIx());
+
+  if (SD->isIndirect())
+    // Push [fi + 0] onto the DIExpression stack.
+    FrameMI.addImm(0);
+  else
+    // Push fi onto the DIExpression stack.
+    FrameMI.addReg(0);
+
+  return FrameMI.addMetadata(Var).addMetadata(Expr);
+}
+
+MachineInstr *
+InstrEmitter::EmitDbgConst(SDDbgValue *SD) {
+  assert(SD->getKind() == SDDbgValue::CONST);
+  const MCInstrDesc &II = TII->get(TargetOpcode::DBG_VALUE);
+  MDNode *Var = SD->getVariable();
+  MDNode *Expr = SD->getExpression();
+  DebugLoc DL = SD->getDebugLoc();
+
+  // Build a DBG_VALUE instruction...
+  MachineInstrBuilder MIB = BuildMI(*MF, DL, II);
+
+  // ... and now find out what kind of constant operand to attach to it.
+  const Value *V = SD->getConst();
+  if (const ConstantInt *CI = dyn_cast<ConstantInt>(V)) {
+    if (CI->getBitWidth() > 64)
+      MIB.addCImm(CI);
+    else
+      MIB.addImm(CI->getSExtValue());
+  } else if (const ConstantFP *CF = dyn_cast<ConstantFP>(V)) {
+    MIB.addFPImm(CF);
+  } else if (isa<ConstantPointerNull>(V)) {
+    // Note: This assumes that all nullptr constants are zero-valued.
+    MIB.addImm(0);
+  } else {
+    // Could be an Undef.  In any case insert an Undef so we can see what we
+    // dropped.
+    MIB.addReg(0U, RegState::Debug);
+  }
+
+  MIB.addReg(0U, RegState::Debug);
+  MIB.addMetadata(Var);
+  MIB.addMetadata(Expr);
+  return MIB;
 }
 
 MachineInstr *
 InstrEmitter::EmitDbgLabel(SDDbgLabel *SD) {
   MDNode *Label = SD->getLabel();
   DebugLoc DL = SD->getDebugLoc();
   assert(cast<DILabel>(Label)->isValidLocationForIntrinsic(DL) &&
          "Expected inlined-at fields to agree");

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

bool SelectionDAGISel::runOnMachineFunction(MachineFunction &mf) {
 
   DenseMap<unsigned, unsigned> LiveInMap;
   if (!FuncInfo->ArgDbgValues.empty())
     for (std::pair<unsigned, unsigned> LI : RegInfo->liveins())
       if (LI.second)
         LiveInMap.insert(LI);
 
   // Insert DBG_VALUE instructions for function arguments to the entry block.
+  bool InstrRef = TM.Options.ValueTrackingVariableLocations;
   for (unsigned i = 0, e = FuncInfo->ArgDbgValues.size(); i != e; ++i) {
     MachineInstr *MI = FuncInfo->ArgDbgValues[e-i-1];
     bool hasFI = MI->getOperand(0).isFI();
     Register Reg =
         hasFI ? TRI.getFrameRegister(*MF) : MI->getOperand(0).getReg();
     if (Register::isPhysicalRegister(Reg))
       EntryMBB->insert(EntryMBB->begin(), MI);
     else {
       MachineInstr *Def = RegInfo->getVRegDef(Reg);
       if (Def) {
         MachineBasicBlock::iterator InsertPos = Def;
         // FIXME: VR def may not be in entry block.
         Def->getParent()->insert(std::next(InsertPos), MI);
       } else
         LLVM_DEBUG(dbgs() << "Dropping debug info for dead vreg"
                           << Register::virtReg2Index(Reg) << "\n");
     }
 
+    // Don't try and extend through copies in instruction referencing mode.
+    if (InstrRef)
+      continue;
+
     // If Reg is live-in then update debug info to track its copy in a vreg.
     DenseMap<unsigned, unsigned>::iterator LDI = LiveInMap.find(Reg);
     if (LDI != LiveInMap.end()) {
       assert(!hasFI && "There's no handling of frame pointer updating here yet "
                        "- add if needed");
       MachineInstr *Def = RegInfo->getVRegDef(LDI->second);
       MachineBasicBlock::iterator InsertPos = Def;
       const MDNode *Variable = MI->getDebugVariable();
if (LDI != LiveInMap.end()) {
             BuildMI(*MF, DL, TII->get(TargetOpcode::DBG_VALUE), IsIndirect,
                     CopyUseMI->getOperand(0).getReg(), Variable, Expr);
         MachineBasicBlock::iterator Pos = CopyUseMI;
         EntryMBB->insertAfter(Pos, NewMI);
       }
     }
   }
 
+  auto MakeDbgValue = [&](MachineInstr &MI) {
+    const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE);
+    MI.setDesc(RefII);
+    MI.getOperand(1).ChangeToRegister(0, false);
+    MI.getOperand(0).setIsDebug();
+  };
+
+  // Instruction referencing: update any half-done DBG_INSTR_REF instructions
+  // that point at virtual registers, to instead identify an instruction and
+  // operand number.
+  if (InstrRef) {
+    for (auto &MBB : *MF) {
+      for (auto &MI : MBB) {
+        if (!MI.isDebugRef() || !MI.getOperand(0).isReg())
+          continue;
+
+        Register Reg = MI.getOperand(0).getReg();
+
+        // Some vregs can be deleted as redundant in the meantime. Mark those
+        // as DBG_VALUE $noreg.
+        if (Reg == 0) {
+          MakeDbgValue(MI);
+          continue;
+        }
+
+        assert(Reg.isVirtual());
+        MachineInstr &DefMI = *MRI.def_instr_begin(Reg);
+        assert(MRI.hasOneDef(Reg));
+
+        // If we've found a copy-like instruction, follow it back to the
+        // instruction that defines the source value, see salvageCopySSA docs
+        // for why this is important.
+        if (DefMI.isCopyLike() || TII->isCopyInstr(DefMI)) {
+          auto Result = MF->salvageCopySSA(DefMI);
+          MI.getOperand(0).ChangeToImmediate(Result->first);
+          MI.getOperand(1).setImm(Result->second);
+        } else {
+          // Otherwise, identify the operand number that the VReg refers to.
+          unsigned OperandIdx = 0;
+          for (const auto &MO : DefMI.operands()) {
+            if (MO.isReg() && MO.isDef() && MO.getReg() == Reg)
+              break;
+            ++OperandIdx;
+          }
+          assert(OperandIdx < DefMI.getNumOperands());
+
+          // Morph this instr ref to point at the given instruction and operand.
+          unsigned ID = DefMI.getDebugInstrNum();
+          MI.getOperand(0).ChangeToImmediate(ID);
+          MI.getOperand(1).setImm(OperandIdx);
+       }
+      }
+    }
+  }
+
   // Determine if there are any calls in this machine function.
   MachineFrameInfo &MFI = MF->getFrameInfo();
   for (const auto &MBB : *MF) {
     if (MFI.hasCalls() && MF->hasInlineAsm())
       break;
 
     for (const auto &MI : MBB) {
       const MCInstrDesc &MCID = TII->get(MI.getOpcode());

llvm/test/DebugInfo/X86/instr-ref-selectiondag.ll

 ; RUN: llc %s -mtriple=x86_64-unknown-unknown -o - -stop-before=finalize-isel \
 ; RUN:   | FileCheck %s --check-prefix=NORMAL \
 ; RUN:     --implicit-check-not=debug-instr-number \
 ; RUN:     --implicit-check-not=DBG_INSTR_REF
 ; RUN: llc %s -mtriple=x86_64-unknown-unknown -o - -stop-before=finalize-isel \
 ; RUN:     -experimental-debug-variable-locations -verify-machineinstrs \
 ; RUN:   | FileCheck %s --check-prefix=INSTRREF \
 ; RUN:     --implicit-check-not=DBG_VALUE
 
 ; Test that SelectionDAG produces DBG_VALUEs normally, but DBG_INSTR_REFs when
 ; asked.
 
+; NORMAL-LABEL: name: foo
+
 ; NORMAL:      %[[REG0:[0-9]+]]:gr32 = ADD32rr
 ; NORMAL-NEXT: DBG_VALUE %[[REG0]]
 ; NORMAL-NEXT: %[[REG1:[0-9]+]]:gr32 = ADD32rr
 ; NORMAL-NEXT: DBG_VALUE %[[REG1]]
 
 ; Note that I'm baking in an assumption of one-based ordering here. We could
 ; capture and check for the instruction numbers, we'd rely on machine verifier
 ; ensuring there were no duplicates.
 
+; INSTRREF-LABEL: name: foo
+
 ; INSTRREF:      ADD32rr
 ; INSTRREF-SAME: debug-instr-number 1
 ; INSTRREF-NEXT: DBG_INSTR_REF 1, 0
 ; INSTRREF-NEXT: ADD32rr
 ; INSTRREF-SAME: debug-instr-number 2
 ; INSTRREF-NEXT: DBG_INSTR_REF 2, 0
 
+@glob32 = global i32 0
+@glob16 = global i16 0
+@glob8 = global i8 0
+
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 define i32 @foo(i32 %bar, i32 %baz, i32 %qux) !dbg !7 {
 entry:
   %0 = add i32 %bar, %baz, !dbg !14
   call void @llvm.dbg.value(metadata i32 %0, metadata !13, metadata !DIExpression()), !dbg !14
   %1 = add i32 %0, %qux
   call void @llvm.dbg.value(metadata i32 %1, metadata !13, metadata !DIExpression()), !dbg !14
   ret i32 %1, !dbg !14
 }
 
+; In the code below, isel produces a large number of copies between subregisters
+; to represent the gradually decreasing width of the argument. This gets
+; optimized away into three stores, but it's an objective of the instruction
+; referencing design that COPYs are not numbered: they move values, not define
+; them. Test that nothing is numbered, and instead that appropriate
+; substitutions with subregister details are recorded.
+
+; NORMAL-LABEL: name: bar
+
+; NORMAL:      DBG_VALUE $rdi
+; NORMAL-NEXT: %0:gr64_with_sub_8bit = COPY $rdi
+; NORMAL-NEXT: DBG_VALUE %0,
+; NORMAL-NEXT: %1:gr32 = COPY %0.sub_32bit,
+; NORMAL-NEXT: DBG_VALUE %1
+; NORMAL:      %2:gr16 = COPY %0.sub_16bit,
+; NORMAL-NEXT: DBG_VALUE %2
+; NORMAL:      %3:gr8 = COPY %0.sub_8bit,
+; NORMAL-NEXT: DBG_VALUE %3
+
+; INSTRREF-LABEL: name: bar
+
+;; 
+; INSTRREF:      debugValueSubstitutions:
+; INSTRREF-NEXT: - { srcinst: 2, srcop: 0, dstinst: 1, dstop: 0, subreg: 6 }
+; INSTRREF-NEXT: - { srcinst: 4, srcop: 0, dstinst: 3, dstop: 0, subreg: 4 }
+; INSTRREF-NEXT: - { srcinst: 6, srcop: 0, dstinst: 5, dstop: 0, subreg: 1 }
+
+;; As a slight inefficiency today, multiple DBG_PHIs are created.
+
+; INSTRREF:      DBG_PHI $rdi, 5
+; INSTRREF-NEXT: DBG_PHI $rdi, 3
+; INSTRREF-NEXT: DBG_PHI $rdi, 1
+;; Allow arguments to be specified by physreg DBG_VALUEs.
+; INSTRREF-NEXT: DBG_VALUE $rdi
+
+;; Don't test the location of these instr-refs, only that the three non-argument
+;; dbg.values become DBG_INSTR_REFs. We previously checked that these numbers
+;; get substituted, with appropriate subregister qualifiers.
+; INSTRREF:      DBG_INSTR_REF 2, 0
+; INSTRREF:      DBG_INSTR_REF 4, 0
+; INSTRREF:      DBG_INSTR_REF 6, 0
+
+define i32 @bar(i64 %bar) !dbg !20 {
+entry:
+  call void @llvm.dbg.value(metadata i64 %bar, metadata !21, metadata !DIExpression()), !dbg !22
+  %0 = trunc i64 %bar to i32, !dbg !22
+  call void @llvm.dbg.value(metadata i32 %0, metadata !21, metadata !DIExpression()), !dbg !22
+  store i32 %0, i32 *@glob32, !dbg !22
+  %1 = trunc i32 %0 to i16, !dbg !22
+  call void @llvm.dbg.value(metadata i16 %1, metadata !21, metadata !DIExpression()), !dbg !22
+  store i16 %1, i16 *@glob16, !dbg !22
+  %2 = trunc i16 %1 to i8, !dbg !22
+  call void @llvm.dbg.value(metadata i8 %2, metadata !21, metadata !DIExpression()), !dbg !22
+  store i8 %2, i8 *@glob8, !dbg !22
+  ret i32 0, !dbg !22
+}
+
+; Ensure that we can track copies back to physreg defs, and throw in a subreg
+; substitution for fun. The call to @xyzzy defines $rax, which gets copied to
+; a VReg, and then truncated by a subreg copy. We should be able to track
+; through the copies and walk back to the physreg def, labelling the CALL
+; instruction. We should also be able to do this even when the block layout is
+; crazily ordered.
+
+; NORMAL-LABEL: name: baz
+
+; NORMAL:      CALL64pcrel32 @xyzzy
+; NORMAL:      %2:gr64 = COPY $rax,
+; NORMAL:      %0:gr64 = COPY %2,
+; NORMAL-LABEL: bb.1.slippers:
+; NORMAL:      DBG_VALUE %1
+; NORMAL-LABEL: bb.2.shoes:
+; NORMAL:      %1:gr16 = COPY %0.sub_16bit
+
+; INSTRREF-LABEL: name: baz
+
+; INSTRREF:      debugValueSubstitutions:
+; INSTRREF-NEXT:  - { srcinst: 2, srcop: 0, dstinst: 1, dstop: 6, subreg: 4 }
+
+; INSTRREF:      CALL64pcrel32 @xyzzy, {{.*}} debug-instr-number 1
+; INSTRREF:      DBG_INSTR_REF 2, 0
+
+declare i64 @xyzzy()
+
+define i32 @baz() !dbg !30 {
+entry:
+  %foo = call i64 @xyzzy(), !dbg !32
+  br label %shoes
+
+slippers:
+  call void @llvm.dbg.value(metadata i16 %moo, metadata !31, metadata !DIExpression()), !dbg !32
+  store i16 %moo, i16 *@glob16, !dbg !32
+  ret i32 0, !dbg !32
+
+shoes:
+  %moo = trunc i64 %foo to i16
+  br label %slippers
+}
+
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
 !1 = !DIFile(filename: "exprconflict.c", directory: "/home/jmorse")
 !2 = !{}
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
 !8 = !DISubroutineType(types: !9)
 !9 = !{!10, !10}
 !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !11 = !{!13}
 !13 = !DILocalVariable(name: "baz", scope: !7, file: !1, line: 6, type: !10)
 !14 = !DILocation(line: 1, scope: !7)
+!20 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!21 = !DILocalVariable(name: "xyzzy", scope: !20, file: !1, line: 6, type: !10)
+!22 = !DILocation(line: 1, scope: !20)
+!30 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!31 = !DILocalVariable(name: "xyzzy", scope: !30, file: !1, line: 6, type: !10)
+!32 = !DILocation(line: 1, scope: !30)