[DebugInfo] LiveDebugValues: Move DBG_VALUE creation into VarLoc class

[jmorse]

Summary

In D67393 I've described a problem with LiveDebugValues having a feedback effect, which necessitates being able to delete location transfers that are later discovered to be invalid. This patch is an almost-entirely-NFC cleanup (see below for the functional difference) that aims to ease the deletion of transfers in LiveDebugValues.

The main theme here is moving the creation of DBG_VALUE instructions from two different functions into one method in the VarLoc class. A VarLoc and a DBG_VALUE are then effectively equivalent (the DBG_VALUE just describes the contents of a VarLoc), and in a later patch we can delete transfers by VarLoc number, without having to manipulate any DBG_VALUE insts. Reducing duplication has its own virtues too.

In slightly more detail: I've added a couple of factory functions (CreateCopyLoc and CreateEntryLoc) to encapsulate some slightly fiddly operations ("Use this variable location, but put it in this machine register"). BuildDbgValue implements creating a DBG_VALUE from whatever location a VarLoc describes, and condenses the code in insertTransferDebugPair and flushPendingLocs. A TransferDebugPair struct changes from being a {insert_point, DBG_VALUE} pair to being a {insert_point, VarLocID} pair. Creating a transfer now just means creating a VarLoc and recording the number. Transfer DBG_VALUEs are all created at the end of location propagation via calling BuildDbgValue.

There are a couple of subtle artefacts: previously VarLocs could effectively build long chains where the MI field referred to the previous transfer DBG_VALUE, eventually rooted in an original source-level assignment DBG_VALUE. Now, however, VarLocs can only ever refer to the originating source-level assignment DBG_VALUE, which I think is slightly neater. Plus, previously a VarLoc was effectively a proxy for a DBG_VALUE inst, to make it easier to identify, but because we use VarLoc as the source for creating DBG_VALUE insts, it now holds real meaningful state (TM). For example, the Expr field is the only place an entry-value expression is stored until a DBG_VALUE is created from it.

I'm not familiar with how entry-value information is recorded; as they appear to be register locations with additional expression information, I've recorded them as such.

This patch was going to be 100% NFC, but as it happens it fixes one small problem. Previously if an indirect DBG_VALUE was spilt, when the spill was restored it would be made a direct register reference, losing information along the way. Now, because VarLocs always point their MI field at the original source-level assignment DBG_VALUE, this information doesn't get lost. I've added a test function that records this. (Curiously, we can easily get indirect DBG_VALUEs before regalloc, from dbg.declare's of arguments).

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

286

nit: here we have enough horizontal space to actually write DILocalVariable and DIExpression, which may make this slightly easier to read.

287

this seems to be used only in one switch case, perhaps sink it?

320

///

360

It would be nice if this also printed the kind that we constructed it as (CopyLoc, ...) maybe using an asserts-only kind field? Maybe this also isn't necessary :-)

650

we should probably pass Out into dump and give a dbgs() default argument?

780

should we have a self-documenting static constructor function for this case as well?

[djtodoro]

I think that the problem described within the D67393 imposes a solution with introducing something like DBG_LOC, as @aprantl already suggested.

lib/CodeGen/LiveDebugValues.cpp

262

Is this redundant, since we already set the Kind within the VarLoc's constructor?

test/DebugInfo/MIR/X86/live-debug-values-restore.mir

205

nit: Please make this in order.

[jmorse]

Address feedback, twiddle various things.

lib/CodeGen/LiveDebugValues.cpp

262

This is deliberately building a VarLoc from the register location in MI, then modifying it to be an entry value.

This could be done differently, but I thought a function that "turns this existing DBG_VALUE into an entry-value VarLoc" would be simpler than building a whole VarLoc from scratch. I'll update the comment to indicate that's what I intended.

287

It's passed to BuildMI in each switch case (I've moved EntryValueKind to not fall-through with the latest revision).

360

Ah well, determining where a VarLoc originates from is the topic of a future patch, best leave it til then when it'll be easier to implement.

780

Sounds good

[djtodoro]

lib/CodeGen/LiveDebugValues.cpp

262

Thanks.

358

clang-format please

[jmorse]

clang-format patch

[vsk]

Really nice!

lib/CodeGen/LiveDebugValues.cpp

337

Is the LLVM_ENABLE_DUMP guard still needed?

[jmorse]

lib/CodeGen/LiveDebugValues.cpp

337

My understanding is that it exists to build-in dump methods when specified on a release build -- it's not necessary, but helpful if one is debugging a release, I think.

lib/CodeGen/LiveDebugValues.cpp

union {
       uint64_t RegNo;
       SpillLoc SpillLocation;
       uint64_t Hash;
       int64_t Immediate;
       const ConstantFP *FPImm;
       const ConstantInt *CImm;
     } Loc;
 
-    VarLoc(const MachineInstr &MI, LexicalScopes &LS,
-           VarLocKind K = InvalidKind)
+    VarLoc(const MachineInstr &MI, LexicalScopes &LS)
         : Var(MI), Expr(MI.getDebugExpression()), MI(MI),
           UVS(MI.getDebugLoc(), LS) {
       static_assert((sizeof(Loc) == sizeof(uint64_t)),
                     "hash does not cover all members of Loc");
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       if (int RegNo = isDbgValueDescribedByReg(MI)) {
         Kind = MI.isDebugEntryValue() ? EntryValueKind : RegisterKind;
VarLoc(const MachineInstr &MI, LexicalScopes &LS)
         Loc.CImm = MI.getOperand(0).getCImm();
       }
       assert((Kind != ImmediateKind || !MI.isDebugEntryValue()) &&
              "entry values must be register locations");
     }
 
     /// The constructor for spill locations.
     VarLoc(const MachineInstr &MI, unsigned SpillBase, int SpillOffset,
-           LexicalScopes &LS, const MachineInstr &OrigMI)
-        : Var(MI), Expr(MI.getDebugExpression()), MI(OrigMI),
+           LexicalScopes &LS)
+        : Var(MI), Expr(MI.getDebugExpression()), MI(MI),
           UVS(MI.getDebugLoc(), LS) {
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       Kind = SpillLocKind;
       Loc.SpillLocation = {SpillBase, SpillOffset};
     }
 
+    /// Create an entry location with the given expression, copying the
+    /// machine location from DBG_VALUE MI.
+    static VarLoc CreateEntryLoc(const MachineInstr &MI, LexicalScopes &LS,
+           const DIExpression *EntryExpr) {
+      VarLoc VL(MI, LS);
+      VL.Kind = EntryValueKind;
+      VL.Expr = EntryExpr;
+      return VL;
+    }
+
+    /// Copy the register location in DBG_VALUE MI, updating the register to
+    /// be NewReg.
+    static VarLoc CreateCopyLoc(const MachineInstr &MI, LexicalScopes &LS,
+           unsigned NewReg) {
+      VarLoc VL(MI, LS);
+      assert(VL.Kind == RegisterKind);
+      VL.Loc.RegNo = NewReg;
+      return VL;
+    }
+
+    /// Create a DBG_VALUE representing this VarLoc in the given function.
+    /// Copies variable-specific information such as DILocalVariable and
+    /// inlining information from the original DBG_VALUE instruction, which may
+    /// have been several transfers ago.
+    MachineInstr *
+    BuildDbgValue(MachineFunction &MF) const {
+      const DebugLoc &DbgLoc = MI.getDebugLoc();
+      bool Indirect = MI.isIndirectDebugValue();
+      const auto &IID = MI.getDesc();
+      auto *Var = MI.getDebugVariable();
+      auto *DIExpr = MI.getDebugExpression();
+
+      switch (Kind) {
+      case EntryValueKind:
+        // An entry value is a register location -- but with an updated
+        // expression.
+        DIExpr = Expr;
+        LLVM_FALLTHROUGH;
+      case RegisterKind:
+        // Register locations are like the source DBG_VALUE, but with the
+        // register number from this VarLoc.
+        return BuildMI(MF, DbgLoc, IID, Indirect, Loc.RegNo, Var, DIExpr);
+      case SpillLocKind:
+      {
+        // Spills are indirect DBG_VALUEs, with a base register and offset.
+        // Use the original DBG_VALUEs expression to build the spilt location
+        // on top of. FIXME: spill locations created before this pass runs
+        // are not recognized, and not handled here.
+        auto *SpillExpr = DIExpression::prepend(DIExpr, DIExpression::ApplyOffset,
+                                                Loc.SpillLocation.SpillOffset);
+        unsigned Base = Loc.SpillLocation.SpillBase;
+        return BuildMI(MF, DbgLoc, IID, true, Base, Var, SpillExpr);
+      }
+      case ImmediateKind:
+      {
+        MachineOperand MO = MI.getOperand(0);
+        return BuildMI(MF, DbgLoc, IID, Indirect, MO, Var, DIExpr);
+      }
+      case InvalidKind:
+        llvm_unreachable("Tried to produce DBG_VALUE for invalid VarLoc");
+      }
+    }
+
     // Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
 
     /// If this variable is described by a register, return it,
     /// otherwise return 0.
     unsigned isDescribedByReg() const {
       if (Kind == RegisterKind)
         return Loc.RegNo;
       return 0;
     }
 
     /// Determine whether the lexical scope of this value's debug location
     /// dominates MBB.
     bool dominates(MachineBasicBlock &MBB) const { return UVS.dominates(&MBB); }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-    LLVM_DUMP_METHOD void dump() const { MI.dump(); }
+    // TRI can be null.
+    void dump(const TargetRegisterInfo *TRI) const {
+      dbgs() << "VarLoc(";
+      switch (Kind) {
+      case RegisterKind:
+      case EntryValueKind:
+        dbgs() << printReg(Loc.RegNo, TRI);
+        break;
+      case SpillLocKind:
+        dbgs() << printReg(Loc.SpillLocation.SpillBase, TRI);
+        dbgs() << "[" << Loc.SpillLocation.SpillOffset << "]";
+        break;
+      case ImmediateKind:
+        dbgs() << Loc.Immediate;
+        break;
+      case InvalidKind:
+        llvm_unreachable("Invalid VarLoc in dump method");
+      }
+
+      dbgs() << ", \"" << Var.getVar()->getName() << "\", " << *Expr << ", ";
+      if (Var.getInlinedAt())
+        dbgs() << "!" << Var.getInlinedAt()->getMetadataID() << ")\n";
+      else
+        dbgs() << "(null))\n";
+    }
 #endif
 
     bool operator==(const VarLoc &Other) const {
       return Kind == Other.Kind && Var == Other.Var &&
              Loc.Hash == Other.Loc.Hash && Expr == Other.Expr;
     }
 
     /// This operator guarantees that VarLocs are sorted by Variable first.
     bool operator<(const VarLoc &Other) const {
       return std::tie(Var, Kind, Loc.Hash, Expr) <
              std::tie(Other.Var, Other.Kind, Other.Loc.Hash, Other.Expr);
     }
   };
 
   using DebugParamMap = SmallDenseMap<const DILocalVariable *, MachineInstr *>;
   using VarLocMap = UniqueVector<VarLoc>;
   using VarLocSet = SparseBitVector<>;
   using VarLocInMBB = SmallDenseMap<const MachineBasicBlock *, VarLocSet>;
   struct TransferDebugPair {
-    MachineInstr *TransferInst;
-    MachineInstr *DebugInst;
+    MachineInstr *TransferInst; /// Instruction where this transfer occurs.
+    unsigned LocationID;        /// Location number for the transfer dest.
   };
   using TransferMap = SmallVector<TransferDebugPair, 4>;
 
   // Types for recording sets of variable fragments that overlap. For a given
   // local variable, we record all other fragments of that variable that could
   // overlap it, to reduce search time.
   using FragmentOfVar =
       std::pair<const DILocalVariable *, DIExpression::FragmentInfo>;
for (const MachineBasicBlock &BB : MF) {
     const VarLocSet &L = V.lookup(&BB);
     if (L.empty())
       continue;
     Out << "MBB: " << BB.getNumber() << ":\n";
     for (unsigned VLL : L) {
       const VarLoc &VL = VarLocIDs[VLL];
       Out << " Var: " << VL.Var.getVar()->getName();
       Out << " MI: ";
-      VL.dump();
+      VL.dump(TRI);
     }
   }
   Out << "\n";
 }
 #endif
 
 LiveDebugValues::VarLoc::SpillLoc
 LiveDebugValues::extractSpillBaseRegAndOffset(const MachineInstr &MI) {

 }
 
 void LiveDebugValues::emitEntryValues(MachineInstr &MI,
                                       OpenRangesSet &OpenRanges,
                                       VarLocMap &VarLocIDs,
                                       TransferMap &Transfers,
                                       DebugParamMap &DebugEntryVals,
                                       SparseBitVector<> &KillSet) {
-  MachineFunction *MF = MI.getParent()->getParent();
   for (unsigned ID : KillSet) {
     if (!VarLocIDs[ID].Var.getVar()->isParameter())
       continue;
 
     const MachineInstr *CurrDebugInstr = &VarLocIDs[ID].MI;
 
     // If parameter's DBG_VALUE is not in the map that means we can't
     // generate parameter's entry value.
     if (!DebugEntryVals.count(CurrDebugInstr->getDebugVariable()))
       continue;
 
     auto ParamDebugInstr = DebugEntryVals[CurrDebugInstr->getDebugVariable()];
     DIExpression *NewExpr = DIExpression::prepend(
         ParamDebugInstr->getDebugExpression(), DIExpression::EntryValue);
-    MachineInstr *EntryValDbgMI =
-        BuildMI(*MF, ParamDebugInstr->getDebugLoc(), ParamDebugInstr->getDesc(),
-                ParamDebugInstr->isIndirectDebugValue(),
-                ParamDebugInstr->getOperand(0).getReg(),
-                ParamDebugInstr->getDebugVariable(), NewExpr);
-
-    if (ParamDebugInstr->isIndirectDebugValue())
-      EntryValDbgMI->getOperand(1).setImm(
-          ParamDebugInstr->getOperand(1).getImm());
-
-    Transfers.push_back({&MI, EntryValDbgMI});
-    VarLoc VL(*EntryValDbgMI, LS);
-    unsigned EntryValLocID = VarLocIDs.insert(VL);
-    OpenRanges.insert(EntryValLocID, VL.Var);
+
+    VarLoc EntryLoc = VarLoc::CreateEntryLoc(*ParamDebugInstr, LS, NewExpr);
+
+    unsigned EntryValLocID = VarLocIDs.insert(EntryLoc);
+    Transfers.push_back({&MI, EntryValLocID});
+    OpenRanges.insert(EntryValLocID, EntryLoc.Var);
   }
 }
 
 /// Create new TransferDebugPair and insert it in \p Transfers. The VarLoc
 /// with \p OldVarID should be deleted form \p OpenRanges and replaced with
 /// new VarLoc. If \p NewReg is different than default zero value then the
 /// new location will be register location created by the copy like instruction,
 /// otherwise it is variable's location on the stack.
 void LiveDebugValues::insertTransferDebugPair(
     MachineInstr &MI, OpenRangesSet &OpenRanges, TransferMap &Transfers,
     VarLocMap &VarLocIDs, unsigned OldVarID, TransferKind Kind,
     unsigned NewReg) {
   const MachineInstr *DebugInstr = &VarLocIDs[OldVarID].MI;
-  MachineFunction *MF = MI.getParent()->getParent();
-  MachineInstr *NewDebugInstr;
 
   auto ProcessVarLoc = [&MI, &OpenRanges, &Transfers, &DebugInstr,
-                        &VarLocIDs](VarLoc &VL, MachineInstr *NewDebugInstr) {
+                        &VarLocIDs](VarLoc &VL) {
     unsigned LocId = VarLocIDs.insert(VL);
 
     // Close this variable's previous location range.
     DebugVariable V(*DebugInstr);
     OpenRanges.erase(V);
 
+    // Record the new location as an open range, and a postponed transfer
+    // inserting a DBG_VALUE for this location.
     OpenRanges.insert(LocId, VL.Var);
-    // The newly created DBG_VALUE instruction NewDebugInstr must be inserted
-    // after MI. Keep track of the pairing.
-    TransferDebugPair MIP = {&MI, NewDebugInstr};
+    TransferDebugPair MIP = {&MI, LocId};
     Transfers.push_back(MIP);
   };
 
   // End all previous ranges of Var.
   OpenRanges.erase(VarLocIDs[OldVarID].Var);
   switch (Kind) {
   case TransferKind::TransferCopy: {
     assert(NewReg &&
            "No register supplied when handling a copy of a debug value");
     // Create a DBG_VALUE instruction to describe the Var in its new
     // register location.
-    NewDebugInstr = BuildMI(
-        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(),
-        DebugInstr->isIndirectDebugValue(), NewReg,
-        DebugInstr->getDebugVariable(), DebugInstr->getDebugExpression());
-    if (DebugInstr->isIndirectDebugValue())
-      NewDebugInstr->getOperand(1).setImm(DebugInstr->getOperand(1).getImm());
-    VarLoc VL(*NewDebugInstr, LS);
-    ProcessVarLoc(VL, NewDebugInstr);
-    LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for register copy: ";
-               NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
-                                    /*SkipOpers*/false, /*SkipDebugLoc*/false,
-                                    /*AddNewLine*/true, TII));
+    VarLoc VL = VarLoc::CreateCopyLoc(*DebugInstr, LS, NewReg);
+    ProcessVarLoc(VL);
+    LLVM_DEBUG({dbgs() << "Creating VarLoc for register copy:";
+                VL.dump(TRI);});
     return;
   }
   case TransferKind::TransferSpill: {
     // Create a DBG_VALUE instruction to describe the Var in its spilled
     // location.
     VarLoc::SpillLoc SpillLocation = extractSpillBaseRegAndOffset(MI);
-    auto *SpillExpr = DIExpression::prepend(DebugInstr->getDebugExpression(),
-                                            DIExpression::ApplyOffset,
-                                            SpillLocation.SpillOffset);
-    NewDebugInstr = BuildMI(
-        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), true,
-        SpillLocation.SpillBase, DebugInstr->getDebugVariable(), SpillExpr);
-    VarLoc VL(*NewDebugInstr, SpillLocation.SpillBase,
-              SpillLocation.SpillOffset, LS, *DebugInstr);
-    ProcessVarLoc(VL, NewDebugInstr);
-    LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for spill: ";
-               NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
-                                    /*SkipOpers*/false, /*SkipDebugLoc*/false,
-                                    /*AddNewLine*/true, TII));
+    VarLoc VL(*DebugInstr, SpillLocation.SpillBase,
+              SpillLocation.SpillOffset, LS);
+    ProcessVarLoc(VL);
+    LLVM_DEBUG({dbgs() << "Creating VarLoc for spill:";
+                VL.dump(TRI);});
     return;
   }
   case TransferKind::TransferRestore: {
     assert(NewReg &&
            "No register supplied when handling a restore of a debug value");
     MachineFunction *MF = MI.getMF();
     DIBuilder DIB(*const_cast<Function &>(MF->getFunction()).getParent());
     // DebugInstr refers to the pre-spill location, therefore we can reuse
     // its expression.
-    NewDebugInstr = BuildMI(
-        *MF, DebugInstr->getDebugLoc(), DebugInstr->getDesc(), false, NewReg,
-        DebugInstr->getDebugVariable(), DebugInstr->getDebugExpression());
-    VarLoc VL(*NewDebugInstr, LS);
-    ProcessVarLoc(VL, NewDebugInstr);
-    LLVM_DEBUG(dbgs() << "Creating DBG_VALUE inst for register restore: ";
-               NewDebugInstr->print(dbgs(), /*IsStandalone*/false,
-                                    /*SkipOpers*/false, /*SkipDebugLoc*/false,
-                                    /*AddNewLine*/true, TII));
+    VarLoc VL = VarLoc::CreateCopyLoc(*DebugInstr, LS, NewReg);
+    ProcessVarLoc(VL);
+    LLVM_DEBUG({dbgs() << "Creating VarLoc for restore:";
+                VL.dump(TRI);});
     return;
   }
   }
   llvm_unreachable("Invalid transfer kind");
 }
 
 /// A definition of a register may mark the end of a range.
 void LiveDebugValues::transferRegisterDef(
for (unsigned ID : OpenRanges.getVarLocs()) {
         // Doing this at a later stage would require re-interpreting all
         // DBG_VALUes and DIExpressions to identify whether they point at
         // memory, and then analysing all memory writes to see if they
         // overwrite that memory, which is expensive.
         //
         // At this stage, we already know which DBG_VALUEs are for spills and
         // where they are located; it's best to fix handle overwrites now.
         KillSet.set(ID);
-        MachineInstr *NewDebugInstr =
-            BuildMI(*MF, VL.MI.getDebugLoc(), VL.MI.getDesc(),
-                    VL.MI.isIndirectDebugValue(), 0, // $noreg
-                    VL.MI.getDebugVariable(), VL.MI.getDebugExpression());
-        Transfers.push_back({&MI, NewDebugInstr});
+        VarLoc UndefVL = VarLoc::CreateCopyLoc(VL.MI, LS, 0);
+        unsigned UndefLocID = VarLocIDs.insert(UndefVL);
+        Transfers.push_back({&MI, UndefLocID});
       }
     }
     OpenRanges.erase(KillSet, VarLocIDs);
   }
 
   // Try to recognise spill and restore instructions that may create a new
   // variable location.
   if (isLocationSpill(MI, MF, Reg)) {
bool LiveDebugValues::transferTerminator(MachineBasicBlock *CurMBB,
                                          VarLocInMBB &OutLocs,
                                          const VarLocMap &VarLocIDs) {
   bool Changed = false;
 
   LLVM_DEBUG(for (unsigned ID
                   : OpenRanges.getVarLocs()) {
     // Copy OpenRanges to OutLocs, if not already present.
     dbgs() << "Add to OutLocs in MBB #" << CurMBB->getNumber() << ":  ";
-    VarLocIDs[ID].dump();
+    VarLocIDs[ID].dump(TRI);
   });
   VarLocSet &VLS = OutLocs[CurMBB];
   Changed = VLS != OpenRanges.getVarLocs();
   // New OutLocs set may be different due to spill, restore or register
   // copy instruction processing.
   if (Changed)
     VLS = OpenRanges.getVarLocs();
   OpenRanges.clear();
for (auto &Iter : PendingInLocs) {
     // Map is keyed on a constant pointer, unwrap it so we can insert insts.
     auto &MBB = const_cast<MachineBasicBlock &>(*Iter.first);
     VarLocSet &Pending = Iter.second;
 
     for (unsigned ID : Pending) {
       // The ID location is live-in to MBB -- work out what kind of machine
       // location it is and create a DBG_VALUE.
       const VarLoc &DiffIt = VarLocIDs[ID];
-      const MachineInstr *DebugInstr = &DiffIt.MI;
-      MachineInstr *MI = nullptr;
+      MachineInstr *MI = DiffIt.BuildDbgValue(*MBB.getParent());
+      MBB.insert(MBB.instr_begin(), MI);
 
-      if (DiffIt.isConstant()) {
-        MachineOperand MO(DebugInstr->getOperand(0));
-        MI = BuildMI(MBB, MBB.instr_begin(), DebugInstr->getDebugLoc(),
-                     DebugInstr->getDesc(), false, MO,
-                     DebugInstr->getDebugVariable(),
-                     DebugInstr->getDebugExpression());
-      } else {
-        auto *DebugExpr = DebugInstr->getDebugExpression();
-        Register Reg = DebugInstr->getOperand(0).getReg();
-        bool IsIndirect = DebugInstr->isIndirectDebugValue();
-
-        if (DiffIt.Kind == VarLoc::SpillLocKind) {
-          // This is is a spilt location; DebugInstr refers to the unspilt
-          // location. We need to rebuild the spilt location expression and
-          // point the DBG_VALUE at the frame register.
-          DebugExpr = DIExpression::prepend(
-              DebugInstr->getDebugExpression(), DIExpression::ApplyOffset,
-              DiffIt.Loc.SpillLocation.SpillOffset);
-          Reg = TRI->getFrameRegister(*DebugInstr->getMF());
-          IsIndirect = true;
-        }
-
-        MI = BuildMI(MBB, MBB.instr_begin(), DebugInstr->getDebugLoc(),
-                     DebugInstr->getDesc(), IsIndirect, Reg,
-                     DebugInstr->getDebugVariable(), DebugExpr);
-      }
       (void)MI;
       LLVM_DEBUG(dbgs() << "Inserted: "; MI->dump(););
     }
   }
 }
 
 /// Calculate the liveness information for the given machine function and
 /// extend ranges across basic blocks.
while (!Worklist.empty() || !Pending.empty()) {
     Worklist.swap(Pending);
     // At this point, pending must be empty, since it was just the empty
     // worklist
     assert(Pending.empty() && "Pending should be empty");
   }
 
   // Add any DBG_VALUE instructions created by location transfers.
   for (auto &TR : Transfers) {
-    auto *MBB = TR.TransferInst->getParent();
-    MBB->insertAfterBundle(TR.TransferInst->getIterator(), TR.DebugInst);
+    MachineBasicBlock *MBB = TR.TransferInst->getParent();
+    const VarLoc &VL = VarLocIDs[TR.LocationID];
+    MachineInstr *MI = VL.BuildDbgValue(MF);
+    MBB->insertAfterBundle(TR.TransferInst->getIterator(), MI);
   }
   Transfers.clear();
 
   // Deferred inlocs will not have had any DBG_VALUE insts created; do
   // that now.
   flushPendingLocs(PendingInLocs, VarLocIDs);
 
   LLVM_DEBUG(printVarLocInMBB(MF, OutLocs, VarLocIDs, "Final OutLocs", dbgs()));

test/DebugInfo/MIR/X86/live-debug-values-restore.mir

 # RUN: llc -run-pass livedebugvalues -march=x86-64 -o - %s | FileCheck %s
 
 # Generated from the following source with:
 # clang -g -mllvm -stop-before=livedebugvalues -S -O2 test.c -o test.mir
 # Then more functions added to test for extra behaviours with complex
 # expressions:
 #   'g': test for a crash from PR42773
 #   'h': complex expressions should be restored
 #   'i': spills should be restored across block boundaries
+#   'j': indirect DBG_VALUEs should be indirect after restoration
 
 # #define FORCE_SPILL() \
 #   __asm volatile("" : : : \
 #                    "rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "r8", \
 #                    "r9", "r10", "r11", "r12", "r13", "r14", "r15")
 # int f(int *p) {
 #   if (p) {
 #     FORCE_SPILL();
 #   }
 #   return *(p + 1);
 # }
 
 # Pick out DILocalVariable numbers for "p", "q" and "r" etc
 # CHECK: ![[PVAR:[0-9]+]] = !DILocalVariable(name: "p",
 # CHECK: ![[QVAR:[0-9]+]] = !DILocalVariable(name: "q",
 # CHECK: ![[RVAR:[0-9]+]] = !DILocalVariable(name: "r",
 # CHECK: ![[SVAR:[0-9]+]] = !DILocalVariable(name: "s",
+# CHECK: ![[TVAR:[0-9]+]] = !DILocalVariable(name: "t",
 
 # Ascertain that the spill has been recognized and manifested in a DBG_VALUE.
 # CHECK: MOV64mr $rsp,{{.*-8.*}}killed{{.*}}$rdi :: (store 8 into %stack.0)
 # CHECK-NEXT: DBG_VALUE $rsp,{{.*}}![[PVAR]],{{.*}}!DIExpression(DW_OP_constu, 8, DW_OP_minus)
 
 # Check for the restore.
 # CHECK: $rdi = MOV64rm $rsp,{{.*-8.*}}:: (load 8 from %stack.0)
 # CHECK-NEXT: DBG_VALUE $rdi,{{.*}}![[PVAR]], !DIExpression()
if.then:                                          ; preds = %entry
     tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !dbg !218, !srcloc !220
     br label %if.end, !dbg !221
   
   if.end:                                           ; preds = %entry, %if.then
     %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !222
     %0 = load i32, i32* %add.ptr, align 4, !dbg !223, !tbaa !24
     ret i32 %0, !dbg !228
   }
- 
   define dso_local i32 @i(i32* readonly %p) local_unnamed_addr !dbg !307 {
   entry:
     br label %foo
 
   foo:
     call void @llvm.dbg.value(metadata i32* %p, metadata !313, metadata !DIExpression()), !dbg !314
     %tobool = icmp eq i32* %p, null, !dbg !315
     br i1 %tobool, label %if.end, label %if.then, !dbg !317
   
   if.then:                                          ; preds = %entry
     tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !dbg !318, !srcloc !320
     br label %if.end, !dbg !321
   
   if.end:                                           ; preds = %entry, %if.then
     %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !322
     %0 = load i32, i32* %add.ptr, align 4, !dbg !323, !tbaa !24
     ret i32 %0, !dbg !328
   }
 
+  define dso_local i32 @j(i32* readonly %p) local_unnamed_addr !dbg !407 {
+  entry:
+    br label %foo
+
+  foo:
+    call void @llvm.dbg.value(metadata i32* %p, metadata !413, metadata !DIExpression()), !dbg !414
+    %tobool = icmp eq i32* %p, null, !dbg !415
+    br i1 %tobool, label %if.end, label %if.then, !dbg !417
+  
+  if.then:                                          ; preds = %entry
+    tail call void asm sideeffect "", "~{rax},~{rbx},~{rcx},~{rdx},~{rsi},~{rdi},~{rbp},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15},~{dirflag},~{fpsr},~{flags}"(), !dbg !418, !srcloc !420
+    br label %if.end, !dbg !421
+  
+  if.end:                                           ; preds = %entry, %if.then
+    %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !422
+    %0 = load i32, i32* %add.ptr, align 4, !dbg !423, !tbaa !24
+    ret i32 %0, !dbg !428
+  }
+
   declare void @llvm.dbg.value(metadata, metadata, metadata)
   
   !llvm.dbg.cu = !{!0}
   !llvm.module.flags = !{!3, !4, !5}
   !llvm.ident = !{!6}
   
   !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0 (https://git.llvm.org/git/clang.git/ 57a6ce7ac318de98e3e777e09cb9ed8282b5cc03) (https://git.llvm.org/git/llvm.git/ ff54a19e4912d7f15cb02798a7f2048441bff751)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
   !1 = !DIFile(filename: "test2.c", directory: "/home/test")
--- |
   !317 = !DILocation(line: 106, column: 7, scope: !307)
   !318 = !DILocation(line: 107, column: 5, scope: !319)
   !319 = distinct !DILexicalBlock(scope: !316, file: !1, line: 106, column: 10)
   !320 = !{i32 -2147471544}
   !321 = !DILocation(line: 108, column: 3, scope: !319)
   !322 = !DILocation(line: 109, column: 14, scope: !307)
   !323 = !DILocation(line: 109, column: 10, scope: !307)
   !328 = !DILocation(line: 109, column: 3, scope: !307)
+  !401 = !DIBasicType(name: "looong int", size: 64, encoding: DW_ATE_signed)
+  !407 = distinct !DISubprogram(name: "j", scope: !0, file: !1, line: 105, type: !8, scopeLine: 105, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !412)
+  !412 = !{!413}
+  !413 = !DILocalVariable(name: "t", arg: 1, scope: !407, file: !1, line: 105, type: !401)
+  !414 = !DILocation(line: 105, column: 12, scope: !407)
+  !415 = !DILocation(line: 106, column: 7, scope: !416)
+  !416 = distinct !DILexicalBlock(scope: !407, file: !1, line: 106, column: 7)
+  !417 = !DILocation(line: 106, column: 7, scope: !407)
+  !418 = !DILocation(line: 107, column: 5, scope: !419)
+  !419 = distinct !DILexicalBlock(scope: !416, file: !1, line: 106, column: 10)
+  !420 = !{i32 -2147471544}
+  !421 = !DILocation(line: 108, column: 3, scope: !419)
+  !422 = !DILocation(line: 109, column: 14, scope: !407)
+  !423 = !DILocation(line: 109, column: 10, scope: !407)
+  !428 = !DILocation(line: 109, column: 3, scope: !407)
 ...
 ---
 name:            f
 alignment:       4
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
 selected:        false
body:             |
   bb.3.if.end:
     liveins: $rbx, $r12, $r13, $r14, $r15, $rbp
   
     renamable $rdi = MOV64rm $rsp, 1, $noreg, -8, $noreg :: (load 8 from %stack.0)
     renamable $eax = MOV32rm killed renamable $rdi, 1, $noreg, 4, $noreg, debug-location !323 :: (load 4 from %ir.add.ptr, !tbaa !24)
     RETQ $eax, debug-location !328
 
 ...
+---
+# Test that if an unspilt DBG_VALUE starts as an indirect DBG_VALUE, then it
+# is restored as an indirect DBG_VALUE. FIXME: Note that for the intervening
+# period of being a spilt location there is still a missing layer of
+# indirection.
+
+# CHECK-LABEL: name: j
+# CHECK-LABEL: bb.0.entry:
+# CHECK:       DBG_VALUE $rdi, 0, ![[TVAR]], !DIExpression()
+# CHECK-LABEL: bb.1.if.then:
+# CHECK:       DBG_VALUE $rsp, 0, ![[TVAR]], !DIExpression(DW_OP_constu, 8, DW_OP_minus)
+# CHECK:       INLINEASM
+# CHECK:       DBG_VALUE ${{[a-zA-Z0-9]+}}, 0, ![[TVAR]], !DIExpression()
+# CHECK-LABEL: bb.2.if.end
+
+name:            j
+tracksRegLiveness: true
+liveins:         
+  - { reg: '$rdi', virtual-reg: '' }
+frameInfo:       
+  stackSize:       48
+  offsetAdjustment: -48
+  maxAlignment:    8
+  cvBytesOfCalleeSavedRegisters: 48
+fixedStack:      
+  - { id: 0, type: spill-slot, offset: -56, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$rbx', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 1, type: spill-slot, offset: -48, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$r12', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 2, type: spill-slot, offset: -40, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$r13', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 3, type: spill-slot, offset: -32, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$r14', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 4, type: spill-slot, offset: -24, size: 8, alignment: 8, stack-id: default,
+      callee-saved-register: '$r15', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+  - { id: 5, type: spill-slot, offset: -16, size: 8, alignment: 16, stack-id: default,
+      callee-saved-register: '$rbp', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+stack:           
+  - { id: 0, name: '', type: spill-slot, offset: -64, size: 8, alignment: 8, 
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+constants:       []
+body:             |
+  bb.0.entry:
+    successors: %bb.2, %bb.1
+    liveins: $rdi, $rbx, $r12, $r13, $r14, $r15, $rbp
+  
+    DBG_VALUE $rdi, 0, !413, !DIExpression(), debug-location !414
+    DBG_VALUE $rdi, 0, !413, !DIExpression(), debug-location !414
+    TEST64rr renamable $rdi, renamable $rdi, implicit-def $eflags, debug-location !415
+    JCC_1 %bb.2, 4, implicit $eflags, debug-location !417
+  
+  bb.1.if.then:
+    successors: %bb.2
+    liveins: $rdi, $rbp, $r15, $r14, $r13, $r12, $rbx
+  
+    frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    frame-setup PUSH64r killed $r15, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 24
+    frame-setup PUSH64r killed $r14, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 32
+    frame-setup PUSH64r killed $r13, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 40
+    frame-setup PUSH64r killed $r12, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 48
+    frame-setup PUSH64r killed $rbx, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 56
+    CFI_INSTRUCTION offset $rbx, -56
+    CFI_INSTRUCTION offset $r12, -48
+    CFI_INSTRUCTION offset $r13, -40
+    CFI_INSTRUCTION offset $r14, -32
+    CFI_INSTRUCTION offset $r15, -24
+    CFI_INSTRUCTION offset $rbp, -16
+    MOV64mr $rsp, 1, $noreg, -8, $noreg, killed renamable $rdi :: (store 8 into %stack.0)
+    INLINEASM &"", 1, 12, implicit-def dead early-clobber $rax, 12, implicit-def dead early-clobber $rbx, 12, implicit-def dead early-clobber $rcx, 12, implicit-def dead early-clobber $rdx, 12, implicit-def dead early-clobber $rsi, 12, implicit-def dead early-clobber $rdi, 12, implicit-def dead early-clobber $rbp, 12, implicit-def dead early-clobber $r8, 12, implicit-def dead early-clobber $r9, 12, implicit-def dead early-clobber $r10, 12, implicit-def dead early-clobber $r11, 12, implicit-def dead early-clobber $r12, 12, implicit-def dead early-clobber $r13, 12, implicit-def dead early-clobber $r14, 12, implicit-def dead early-clobber $r15, 12, implicit-def dead early-clobber $eflags, !20, debug-location !418
+    renamable $rdi = MOV64rm $rsp, 1, $noreg, -8, $noreg :: (load 8 from %stack.0)
+    $rbx = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 48
+    $r12 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 40
+    $r13 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 32
+    $r14 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 24
+    $r15 = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 8
+  
+  bb.2.if.end:
+    liveins: $rdi, $rbx, $r12, $r13, $r14, $r15, $rbp
+  
+    renamable $eax = MOV32rm killed renamable $rdi, 1, $noreg, 4, $noreg, debug-location !423 :: (load 4 from %ir.add.ptr, !tbaa !24)
+    RETQ $eax, debug-location !428
+
+...