[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 ↗

(On Diff #219533)

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

287 ↗

(On Diff #219533)

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

320 ↗

(On Diff #219533)

///

360 ↗

(On Diff #219533)

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 ↗

(On Diff #219533)

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

780 ↗

(On Diff #219533)

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 ↗

(On Diff #219533)

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

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

205 ↗

(On Diff #219533)

nit: Please make this in order.

[jmorse]

Address feedback, twiddle various things.

lib/CodeGen/LiveDebugValues.cpp

262 ↗

(On Diff #219533)

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 ↗

(On Diff #219533)

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

360 ↗

(On Diff #219533)

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 ↗

(On Diff #219533)

Sounds good

[djtodoro]

lib/CodeGen/LiveDebugValues.cpp

304 ↗

(On Diff #219738)

clang-format please

262 ↗

(On Diff #219533)

Thanks.

[jmorse]

clang-format patch

[vsk]

Really nice!

lib/CodeGen/LiveDebugValues.cpp

333 ↗

(On Diff #219899)

Is the LLVM_ENABLE_DUMP guard still needed?

[jmorse]

lib/CodeGen/LiveDebugValues.cpp

333 ↗

(On Diff #219899)

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.

llvm/trunk/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;
         Loc.RegNo = RegNo;
       } else if (MI.getOperand(0).isImm()) {
         Kind = ImmediateKind;
         Loc.Immediate = MI.getOperand(0).getImm();
       } else if (MI.getOperand(0).isFPImm()) {
         Kind = ImmediateKind;
         Loc.FPImm = MI.getOperand(0).getFPImm();
       } else if (MI.getOperand(0).isCImm()) {
         Kind = ImmediateKind;
         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),
-          UVS(MI.getDebugLoc(), LS) {
-      assert(MI.isDebugValue() && "not a DBG_VALUE");
-      assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
-      Kind = SpillLocKind;
-      Loc.SpillLocation = {SpillBase, SpillOffset};
+    /// Take the variable and machine-location in DBG_VALUE MI, and build an
+    /// entry location using the given expression.
+    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;
     }
 
-    // Is the Loc field a constant or constant object?
+    /// Take the variable described by DBG_VALUE MI, and create a VarLoc
+    /// locating it in the specified spill location.
+    static VarLoc CreateSpillLoc(const MachineInstr &MI, unsigned SpillBase,
+                                 int SpillOffset, LexicalScopes &LS) {
+      VarLoc VL(MI, LS);
+      assert(VL.Kind == RegisterKind);
+      VL.Kind = SpillLocKind;
+      VL.Loc.SpillLocation = {SpillBase, SpillOffset};
+      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();
+      const DILocalVariable *Var = MI.getDebugVariable();
+      const DIExpression *DIExpr = MI.getDebugExpression();
+
+      switch (Kind) {
+      case EntryValueKind:
+        // An entry value is a register location -- but with an updated
+        // expression.
+        return BuildMI(MF, DbgLoc, IID, Indirect, Loc.RegNo, Var, Expr);
+      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, raw_ostream &Out = dbgs()) 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);
     }
   }
   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 = VarLoc::CreateSpillLoc(*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()));

llvm/trunk/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 !402 {
+  entry:
+    br label %foo
+
+  foo:
+    call void @llvm.dbg.value(metadata i32* %p, metadata !404, metadata !DIExpression()), !dbg !405
+    %tobool = icmp eq i32* %p, null, !dbg !406
+    br i1 %tobool, label %if.end, label %if.then, !dbg !408
+  
+  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 !409, !srcloc !411
+    br label %if.end, !dbg !412
+  
+  if.end:                                           ; preds = %entry, %if.then
+    %add.ptr = getelementptr inbounds i32, i32* %p, i64 1, !dbg !413
+    %0 = load i32, i32* %add.ptr, align 4, !dbg !414, !tbaa !24
+    ret i32 %0, !dbg !415
+  }
+
   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)
+  !402 = distinct !DISubprogram(name: "j", scope: !0, file: !1, line: 105, type: !8, scopeLine: 105, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !403)
+  !403 = !{!404}
+  !404 = !DILocalVariable(name: "t", arg: 1, scope: !402, file: !1, line: 105, type: !401)
+  !405 = !DILocation(line: 105, column: 12, scope: !402)
+  !406 = !DILocation(line: 106, column: 7, scope: !407)
+  !407 = distinct !DILexicalBlock(scope: !402, file: !1, line: 106, column: 7)
+  !408 = !DILocation(line: 106, column: 7, scope: !402)
+  !409 = !DILocation(line: 107, column: 5, scope: !410)
+  !410 = distinct !DILexicalBlock(scope: !407, file: !1, line: 106, column: 10)
+  !411 = !{i32 -2147471544}
+  !412 = !DILocation(line: 108, column: 3, scope: !410)
+  !413 = !DILocation(line: 109, column: 14, scope: !402)
+  !414 = !DILocation(line: 109, column: 10, scope: !402)
+  !415 = !DILocation(line: 109, column: 3, scope: !402)
 ...
 ---
 name:            f
 alignment:       16
 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, !404, !DIExpression(), debug-location !405
+    DBG_VALUE $rdi, 0, !404, !DIExpression(), debug-location !405
+    TEST64rr renamable $rdi, renamable $rdi, implicit-def $eflags, debug-location !406
+    JCC_1 %bb.2, 4, implicit $eflags, debug-location !408
+  
+  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 !409
+    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 !414 :: (load 4 from %ir.add.ptr, !tbaa !24)
+    RETQ $eax, debug-location !415
+
+...