[DebugInfo][Instr] Track subregisters across stack spills/restores

[jmorse]

Summary

(This patch superceeds D104519, but does it better)

Track machine values within sub-fields of stack slots. Sometimes we generate code that writes to a subregister, then spills / restores a super-register to the stack, for example:

$eax = MOV32ri 0 
MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax
$rcx = MOV64rm $rsp, 1, $noreg, 8, $noreg

Which happens a lot on x86, because stack spills tend to be widened to optimise instruction encoding. Right now, we can't identify that $ecx contains the value from $eax having a constant loaded into it.

Over in D104519 I took a shot at this, by tracking subregister indexes within stack slots. However this wasn't a good solution:

• The largest registers don't have subregister indexes,
• There can be different register hierarchies (eax vs xmm0), the largest of each not having a subregister index,
• A courtesy look at ARM targets suggest they have ~130 subregister indexes.

Which is a recipe for confusion and inefficiency.

This patch takes a different approach: it adds another index to MLocTracker that identifies a size/offset within a stack slot. A location on the stack is then a pair of {StackNum, SlotNum}, where the stack number is an identifier for the stacks FrameIndex, while the SlotNum tells us where within that FrameIndex we're referring to. I've added a diagram to the class docustring for MLocTracker to try and illustrate this. The benefit is that we don't have to consider relationships between registers when identifying something on the stack, only size and offset. It also coalesces locations that are the same size/offset, but have different subregister numbers.

Spilling and restoring is now a matter of identifying the src/dest register number, and the dest/src stack position, then copying a ValueIDNum between the two.

One limitation this exposes, is that if a PHI happens inside a stack slot, LLVM doesn't record how large the value is. If we have a DBG_PHI of %stack.0, is that referring to a 32 bit or 64 bit value within it? Possibly in the future we'll need to record a size in a DBG_PHI instruction, but until then, this affects a very small number of locations. Overall, this patch recovers an additional 1% of variable locations on a clang3.4 build, largely due to patterns like the one copied above.

I've also added unit tests to ensure that values are recorded correctly on the stack, and things like overwriting a spill of $rax with $xmm0 doesn't lead to unexpected values turning up, etc.

[Orlando]

I think this looks good (the patch sounds great), but I definitely need to go over it at least once more before approving to be sure I understand the changes. In amongst the inline nits there are a couple of questions.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

753

nit: location's

1203

std::array here could be preferable so the size can be used in the loop below?

1385

nit: Is FI used anywhere else after these isXXFromStackSlotPostFE calls? The comment above makes it sound like the answer is yes, but I can't see it being used anywhere.

1459

Is this an assumption that needs to be chceked in an assert, or is it that it's handled gracefully / won't cause a catastrophe if it is ever wrong?

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

299–323

nit: unnecessarily

361

nit: missing the 3rd / in ///

373

nit: missing the 3rd / in ///

384

Seems like it could be easy to mix up the fields in a pair of identical types. If this could be implemented as a struct without too much boilerplate I'd be slightly in favour of going down that route, but this isn't a strong opinion / request.

446

nit: apram -> param

469

I am a little confused with the large number of different indices flying around in here, so this might be a silly question. Should there be another number subtracted here, equivalent to the Idx added to the ID in the function above (getSpillIDWithIdx)? Or is this auto-magically handled due to integer division rounding or something?

[djtodoro]

This looks cool.

I was wondering, shell we be paying attention to subregisters in spills/restores in the case of VarLocBasedLDV?

[jmorse]

In D112133#3077284, @djtodoro wrote:

I was wondering, shell we be paying attention to subregisters in spills/restores in the case of VarLocBasedLDV?

This should be possible -- it'd involve enumerating all subregisters in `transferSpillOrRestoreInst, finding VarLocs for them, then moving them to a stack location. It would probably mean tracking the size of different spills, so that when a stack restore happens, the right variables are restored to the right subregisters.

It might come with a performance cost, as the number of locations tracked for each variable on the stack goes up a lot. InstrRefBasedLDV can get away with this because it only solves the problem once, it doesn't do it for each individual variable. On the other hand, spills are a small proportion of the locations tracked by by LiveDebugValues.

[djtodoro]

In D112133#3077383, @jmorse wrote:

In D112133#3077284, @djtodoro wrote:

I was wondering, shell we be paying attention to subregisters in spills/restores in the case of VarLocBasedLDV?

This should be possible -- it'd involve enumerating all subregisters in `transferSpillOrRestoreInst, finding VarLocs for them, then moving them to a stack location. It would probably mean tracking the size of different spills, so that when a stack restore happens, the right variables are restored to the right subregisters.

It might come with a performance cost, as the number of locations tracked for each variable on the stack goes up a lot. InstrRefBasedLDV can get away with this because it only solves the problem once, it doesn't do it for each individual variable. On the other hand, spills are a small proportion of the locations tracked by by LiveDebugValues.

I see... But I guess it can give us a decent location coverage improvement, especially for targets such as x86, right?

[jmorse]

In D112133#3077525, @djtodoro wrote:

I see... But I guess it can give us a decent location coverage improvement, especially for targets such as x86, right?

Probably yes -- it's hard to say in advance because InstrRef is better at handling spill locations. It can follow values across both the stack and in registers, while VarLocBasedLDV has to commit to either following a spill or staying in a register. Definitely worth prototyping though.

Note that I'm cooking up a plan to turn instr-ref on by default for X86 for LLVM14, although it's not a certain thing.

[djtodoro]

In D112133#3077552, @jmorse wrote:

In D112133#3077525, @djtodoro wrote:

I see... But I guess it can give us a decent location coverage improvement, especially for targets such as x86, right?

Probably yes -- it's hard to say in advance because InstrRef is better at handling spill locations. It can follow values across both the stack and in registers, while VarLocBasedLDV has to commit to either following a spill or staying in a register. Definitely worth prototyping though.

Thanks.

Note that I'm cooking up a plan to turn instr-ref on by default for X86 for LLVM14, although it's not a certain thing.

I am happy to hear that, good luck!

[jmorse]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

469

The latter; I'm relying on the fact that integer division truncates any remainder, the "index" / "idx" / whatever will be eliminated from "ID" in the process. To illustrate, here's an example LocIDToLocIdx table, assuming there are 7 "Idxes"

0 => AL
1 => AH
[... some ~288 X86 registers]
288 => Slot 0 Idx 0
289 => Slot 0 Idx 1
290 => Slot 0 Idx 2
291 => Slot 0 Idx 3
292 => Slot 0 Idx 4
293 => Slot 0 Idx 5
294 => Slot 0 Idx 6
295 => Slot 1 Idx 0
296 => Slot 1 Idx 1
297 => Slot 1 Idx 2
298 => Slot 1 Idx 3
299 => Slot 1 Idx 4
300 => Slot 1 Idx 5
301 => Slot 1 Idx 6

This function subtracts to find the range where the "slots" are recorded, then divides by 7 to remove the "Idx" part, leaving only the slot number.

[Orlando]

Ok, been over this again and LGTM (for real this time). I'm a big fan of all the testing.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

469

SGTM, ty for that. Might be worth just mentioning that in a comment briefly but I'll leave that up to you.

[jmorse]

Add a little more type safety to spill location numbers -- the number of the spill in the UniqueVector is now wrapped, so that it's a bit more obvious what's going on.

(All the number of numbers and indexes continues to be a weakness)

[jmorse]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1385

Looks like it isn't; it was in the past, I think, now it's un-necessary. I'll rename "DummyFI" and fix the comment.

1459

I think we'd have to decode the machine-instruction and work out where it was addressing, to assert that. IMO this is a design assumption: we can't assert that LLVM always does the right thing, only document what was originally expected by this code.

(Another design assumption, for example, is that all modifications to stack slots have a MachineMemoryOperand attached to the instruction, which we can't really assert for).

[jmorse]

(Address Orlandos review comments)

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

return Twine("Value{bb: ")
                             .concat(Twine(", loc: ").concat(Twine(mlocname)))
                             .concat(Twine("}")))))
         .str();
   }
 
   static ValueIDNum EmptyValue;
 };
 
+/// Thin wrapper around an integer -- designed to give more type safety to
+/// spill location numbers.
+class SpillLocationNo {
+public:
+  explicit SpillLocationNo(unsigned SpillNo) : SpillNo(SpillNo) {}
+  unsigned SpillNo;
+  unsigned id() const { return SpillNo; }
+
+  bool operator<(const SpillLocationNo &Other) const {
+    return SpillNo < Other.SpillNo;
+  }
+
+  bool operator==(const SpillLocationNo &Other) const {
+    return SpillNo == Other.SpillNo;
+  }
+  bool operator!=(const SpillLocationNo &Other) const {
+    return !(*this == Other);
+  }
+};
+
 /// Meta qualifiers for a value. Pair of whatever expression is used to qualify
 /// the the value, and Boolean of whether or not it's indirect.
 class DbgValueProperties {
 public:
   DbgValueProperties(const DIExpression *DIExpr, bool Indirect)
       : DIExpr(DIExpr), Indirect(Indirect) {}
 
   /// Extract properties from an existing DBG_VALUE instruction.

 /// Tracker for what values are in machine locations. Listens to the Things
 /// being Done by various instructions, and maintains a table of what machine
 /// locations have what values (as defined by a ValueIDNum).
 ///
 /// There are potentially a much larger number of machine locations on the
 /// target machine than the actual working-set size of the function. On x86 for
 /// example, we're extremely unlikely to want to track values through control
 /// or debug registers. To avoid doing so, MLocTracker has several layers of
-/// indirection going on, with two kinds of ``location'':
-///  * A LocID uniquely identifies a register or spill location, with a
-///    predictable value.
-///  * A LocIdx is a key (in the database sense) for a LocID and a ValueIDNum.
-/// Whenever a location is def'd or used by a MachineInstr, we automagically
-/// create a new LocIdx for a location, but not otherwise. This ensures we only
-/// account for locations that are actually used or defined. The cost is another
-/// vector lookup (of LocID -> LocIdx) over any other implementation. This is
-/// fairly cheap, and the compiler tries to reduce the working-set at any one
-/// time in the function anyway.
+/// indirection going on, described below, to avoid unnecessarily tracking
+/// any location.
+///
+/// Here's a sort of diagram of the indexes, read from the bottom up:
+///
+///           Size on stack   Offset on stack
+///                 \              /
+///          Stack Idx (Where in slot is this?)
+///                         /
+///                        /
+/// Slot Num (%stack.0)   /
+/// FrameIdx => SpillNum /
+///              \      /
+///           SpillID (int)              Register number (int)
+///                      \                  /
+///                      LocationID => LocIdx
+///                                |
+///                       LocIdx => ValueIDNum
 ///
-/// Register mask operands completely blow this out of the water; I've just
-/// piled hacks on top of hacks to get around that.
+/// The aim here is that the LocIdx => ValueIDNum vector is just an array of
+/// values in numbered locations, so that later analyses can ignore whether the
+/// location is a register or otherwise. To map a register / spill location to
+/// a LocIdx, you have to use the (sparse) LocationID => LocIdx map. And to
+/// build a LocationID for a stack slot, you need to combine identifiers for
+/// which stack slot it is and where within that slot is being described.
+///
+/// Register mask operands cause trouble by technically defining every register;
+/// various hacks are used to avoid tracking registers that are never read and
+/// only written by regmasks.
 class MLocTracker {
 public:
   MachineFunction &MF;
   const TargetInstrInfo &TII;
   const TargetRegisterInfo &TRI;
   const TargetLowering &TLI;
 
   /// IndexedMap type, mapping from LocIdx to ValueIDNum.
public:
   /// Inverse map of LocIDToLocIdx.
   IndexedMap<unsigned, LocIdxToIndexFunctor> LocIdxToLocID;
 
   /// When clobbering register masks, we chose to not believe the machine model
   /// and don't clobber SP. Do the same for SP aliases, and for efficiency,
   /// keep a set of them here.
   SmallSet<Register, 8> SPAliases;
 
-  /// Unique-ification of spill slots. Used to number them -- their LocID
-  /// number is the index in SpillLocs minus one plus NumRegs.
+  /// Unique-ification of spill. Used to number them -- their LocID number is
+  /// the index in SpillLocs minus one plus NumRegs.
   UniqueVector<SpillLoc> SpillLocs;
 
   // If we discover a new machine location, assign it an mphi with this
   // block number.
   unsigned CurBB;
 
   /// Cached local copy of the number of registers the target has.
   unsigned NumRegs;
 
+  /// Number of slot indexes the target has -- distinct segments of a stack
+  /// slot that can take on the value of a subregister, when a super-register
+  /// is written to the stack.
+  unsigned NumSlotIdxes;
+
   /// Collection of register mask operands that have been observed. Second part
   /// of pair indicates the instruction that they happened in. Used to
   /// reconstruct where defs happened if we start tracking a location later
   /// on.
   SmallVector<std::pair<const MachineOperand *, unsigned>, 32> Masks;
 
+  /// Pair for describing a position within a stack slot -- first the size in
+  /// bits, then the offset.
+  typedef std::pair<unsigned short, unsigned short> StackSlotPos;
+
+  /// Map from a size/offset pair describing a position in a stack slot, to a
+  /// numeric identifier for that position. Allows easier identification of
+  /// individual positions.
+  DenseMap<StackSlotPos, unsigned> StackSlotIdxes;
+
+  /// Inverse of StackSlotIdxes.
+  DenseMap<unsigned, StackSlotPos> StackIdxesToPos;
+
   /// Iterator for locations and the values they contain. Dereferencing
   /// produces a struct/pair containing the LocIdx key for this location,
   /// and a reference to the value currently stored. Simplifies the process
   /// of seeking a particular location.
   class MLocIterator {
     LocToValueType &ValueMap;
     LocIdx Idx;
 
public:
     void operator++() { Idx = LocIdx(Idx.asU64() + 1); }
 
     value_type operator*() { return value_type(Idx, ValueMap[LocIdx(Idx)]); }
   };
 
   MLocTracker(MachineFunction &MF, const TargetInstrInfo &TII,
               const TargetRegisterInfo &TRI, const TargetLowering &TLI);
 
-  /// Produce location ID number for indexing LocIDToLocIdx. Takes the register
-  /// or spill number, and flag for whether it's a spill or not.
-  unsigned getLocID(Register RegOrSpill, bool isSpill) {
-    return (isSpill) ? RegOrSpill.id() + NumRegs - 1 : RegOrSpill.id();
+  /// Produce location ID number for a Register. Provides some small amount of
+  /// type safety.
+  /// \param Reg The register we're looking up.
+  unsigned getLocID(Register Reg) { return Reg.id(); }
+
+  /// Produce location ID number for a spill position.
+  /// \param Spill The number of the spill we're fetching the location for.
+  /// \param SpillSubReg Subregister within the spill we're addressing.
+  unsigned getLocID(SpillLocationNo Spill, unsigned SpillSubReg) {
+    unsigned short Size = TRI.getSubRegIdxSize(SpillSubReg);
+    unsigned short Offs = TRI.getSubRegIdxOffset(SpillSubReg);
+    return getLocID(Spill, {Size, Offs});
+  }
+
+  /// Produce location ID number for a spill position.
+  /// \param Spill The number of the spill we're fetching the location for.
+  /// \apram SpillIdx size/offset within the spill slot to be addressed.
+  unsigned getLocID(SpillLocationNo Spill, StackSlotPos Idx) {
+    unsigned SlotNo = Spill.id() - 1;
+    SlotNo *= NumSlotIdxes;
+    assert(StackSlotIdxes.find(Idx) != StackSlotIdxes.end());
+    SlotNo += StackSlotIdxes[Idx];
+    SlotNo += NumRegs;
+    return SlotNo;
+  }
+
+  /// Given a spill number, and a slot within the spill, calculate the ID number
+  /// for that location.
+  unsigned getSpillIDWithIdx(SpillLocationNo Spill, unsigned Idx) {
+    unsigned SlotNo = Spill.id() - 1;
+    SlotNo *= NumSlotIdxes;
+    SlotNo += Idx;
+    SlotNo += NumRegs;
+    return SlotNo;
+  }
+
+  /// Return the spill number that a location ID corresponds to.
+  SpillLocationNo locIDToSpill(unsigned ID) const {
+    assert(ID >= NumRegs);
+    ID -= NumRegs;
+    // Truncate away the index part, leaving only the spill number.
+    ID /= NumSlotIdxes;
+    return SpillLocationNo(ID + 1); // The UniqueVector is one-based.
+  }
+
+  /// Returns the spill-slot size/offs that a location ID corresponds to.
+  StackSlotPos locIDToSpillIdx(unsigned ID) const {
+    assert(ID >= NumRegs);
+    ID -= NumRegs;
+    unsigned Idx = ID % NumSlotIdxes;
+    return StackIdxesToPos.find(Idx)->second;
   }
 
   unsigned getNumLocs(void) const { return LocIdxToIDNum.size(); }
 
   /// Reset all locations to contain a PHI value at the designated block. Used
   /// sometimes for actual PHI values, othertimes to indicate the block entry
   /// value (before any more information is known).
   void setMPhis(unsigned NewCurBB) {
public:
   void clear(void) {
     reset();
     LocIDToLocIdx.clear();
     LocIdxToLocID.clear();
     LocIdxToIDNum.clear();
     // SpillLocs.reset(); XXX UniqueVector::reset assumes a SpillLoc casts from
     // 0
     SpillLocs = decltype(SpillLocs)();
+    StackSlotIdxes.clear();
+    StackIdxesToPos.clear();
 
     LocIDToLocIdx.resize(NumRegs, LocIdx::MakeIllegalLoc());
   }
 
   /// Set a locaiton to a certain value.
   void setMLoc(LocIdx L, ValueIDNum Num) {
     assert(L.asU64() < LocIdxToIDNum.size());
     LocIdxToIDNum[L] = Num;
bool isRegisterTracked(Register R) {
     LocIdx &Index = LocIDToLocIdx[R];
     return !Index.isIllegal();
   }
 
   /// Record a definition of the specified register at the given block / inst.
   /// This doesn't take a ValueIDNum, because the definition and its location
   /// are synonymous.
   void defReg(Register R, unsigned BB, unsigned Inst) {
-    unsigned ID = getLocID(R, false);
+    unsigned ID = getLocID(R);
     LocIdx Idx = lookupOrTrackRegister(ID);
     ValueIDNum ValueID = {BB, Inst, Idx};
     LocIdxToIDNum[Idx] = ValueID;
   }
 
   /// Set a register to a value number. To be used if the value number is
   /// known in advance.
   void setReg(Register R, ValueIDNum ValueID) {
-    unsigned ID = getLocID(R, false);
+    unsigned ID = getLocID(R);
     LocIdx Idx = lookupOrTrackRegister(ID);
     LocIdxToIDNum[Idx] = ValueID;
   }
 
   ValueIDNum readReg(Register R) {
-    unsigned ID = getLocID(R, false);
+    unsigned ID = getLocID(R);
     LocIdx Idx = lookupOrTrackRegister(ID);
     return LocIdxToIDNum[Idx];
   }
 
   /// Reset a register value to zero / empty. Needed to replicate the
   /// VarLoc implementation where a copy to/from a register effectively
   /// clears the contents of the source register. (Values can only have one
   ///  machine location in VarLocBasedImpl).
   void wipeRegister(Register R) {
-    unsigned ID = getLocID(R, false);
+    unsigned ID = getLocID(R);
     LocIdx Idx = LocIDToLocIdx[ID];
     LocIdxToIDNum[Idx] = ValueIDNum::EmptyValue;
   }
 
   /// Determine the LocIdx of an existing register.
   LocIdx getRegMLoc(Register R) {
-    unsigned ID = getLocID(R, false);
+    unsigned ID = getLocID(R);
+    assert(ID < LocIDToLocIdx.size());
+    assert(LocIDToLocIdx[ID] != UINT_MAX); // Sentinal for IndexedMap.
     return LocIDToLocIdx[ID];
   }
 
   /// Record a RegMask operand being executed. Defs any register we currently
   /// track, stores a pointer to the mask in case we have to account for it
   /// later.
   void writeRegMask(const MachineOperand *MO, unsigned CurBB, unsigned InstID);
 
   /// Find LocIdx for SpillLoc \p L, creating a new one if it's not tracked.
-  LocIdx getOrTrackSpillLoc(SpillLoc L);
-
-  /// Set the value stored in a spill slot.
-  void setSpill(SpillLoc L, ValueIDNum ValueID) {
-    LocIdx Idx = getOrTrackSpillLoc(L);
-    LocIdxToIDNum[Idx] = ValueID;
-  }
-
-  /// Read whatever value is in a spill slot, or None if it isn't tracked.
-  Optional<ValueIDNum> readSpill(SpillLoc L) {
-    unsigned SpillID = SpillLocs.idFor(L);
-    if (SpillID == 0)
-      return None;
-
-    unsigned LocID = getLocID(SpillID, true);
-    LocIdx Idx = LocIDToLocIdx[LocID];
-    return LocIdxToIDNum[Idx];
-  }
+  SpillLocationNo getOrTrackSpillLoc(SpillLoc L);
 
-  /// Determine the LocIdx of a spill slot. Return None if it previously
-  /// hasn't had a value assigned.
-  Optional<LocIdx> getSpillMLoc(SpillLoc L) {
-    unsigned SpillID = SpillLocs.idFor(L);
-    if (SpillID == 0)
-      return None;
-    unsigned LocNo = getLocID(SpillID, true);
-    return LocIDToLocIdx[LocNo];
+  // Get LocIdx of a spill ID.
+  LocIdx getSpillMLoc(unsigned SpillID) {
+    assert(LocIDToLocIdx[SpillID] != UINT_MAX); // Sentinal for IndexedMap.
+    return LocIDToLocIdx[SpillID];
   }
 
   /// Return true if Idx is a spill machine location.
   bool isSpill(LocIdx Idx) const { return LocIdxToLocID[Idx] >= NumRegs; }
 
   MLocIterator begin() { return MLocIterator(LocIdxToIDNum, 0); }
 
   MLocIterator end() {
public:
 
   /// Vector (per block) of a collection (inner smallvector) of live-ins.
   /// Used as the result type for the variable value dataflow problem.
   using LiveInsT = SmallVector<SmallVector<VarAndLoc, 8>, 8>;
 
 private:
   MachineDominatorTree *DomTree;
   const TargetRegisterInfo *TRI;
+  const MachineRegisterInfo *MRI;
   const TargetInstrInfo *TII;
   const TargetFrameLowering *TFI;
   const MachineFrameInfo *MFI;
   BitVector CalleeSavedRegs;
   LexicalScopes LS;
   TargetPassConfig *TPC;
 
   // An empty DIExpression. Used default / placeholder DbgValueProperties
private:
   /// - Is there a register operand that is both used and killed?
   /// TODO: Store optimization can fold spills into other stores (including
   /// other spills). We do not handle this yet (more than one memory operand).
   bool isLocationSpill(const MachineInstr &MI, MachineFunction *MF,
                        unsigned &Reg);
 
   /// If a given instruction is identified as a spill, return the spill slot
   /// and set \p Reg to the spilled register.
-  Optional<SpillLoc> isRestoreInstruction(const MachineInstr &MI,
+  Optional<SpillLocationNo> isRestoreInstruction(const MachineInstr &MI,
                                           MachineFunction *MF, unsigned &Reg);
 
-  /// Given a spill instruction, extract the register and offset used to
-  /// address the spill slot in a target independent way.
-  SpillLoc extractSpillBaseRegAndOffset(const MachineInstr &MI);
+  /// Given a spill instruction, extract the spill slot information, ensure it's
+  /// tracked, and return the spill number.
+  SpillLocationNo extractSpillBaseRegAndOffset(const MachineInstr &MI);
 
   /// Observe a single instruction while stepping through a block.
   void process(MachineInstr &MI, ValueIDNum **MLiveOuts = nullptr,
                ValueIDNum **MLiveIns = nullptr);
 
   /// Examines whether \p MI is a DBG_VALUE and notifies trackers.
   /// \returns true if MI was recognized and processed.
   bool transferDebugValue(const MachineInstr &MI);

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

 #define DEBUG_TYPE "livedebugvalues"
 
 // Act more like the VarLoc implementation, by propagating some locations too
 // far and ignoring some transfers.
 static cl::opt<bool> EmulateOldLDV("emulate-old-livedebugvalues", cl::Hidden,
                                    cl::desc("Act like old LiveDebugValues did"),
                                    cl::init(false));
 
-/// Thin wrapper around an integer -- designed to give more type safety to
-/// spill location numbers.
-class SpillLocationNo {
-public:
-  explicit SpillLocationNo(unsigned SpillNo) : SpillNo(SpillNo) {}
-  unsigned SpillNo;
-  unsigned id() const { return SpillNo; }
-};
-
 /// Tracker for converting machine value locations and variable values into
 /// variable locations (the output of LiveDebugValues), recorded as DBG_VALUEs
 /// specifying block live-in locations and transfers within blocks.
 ///
 /// Operating on a per-block basis, this class takes a (pre-loaded) MLocTracker
 /// and must be initialized with the set of variable values that are live-in to
 /// the block. The caller then repeatedly calls process(). TransferTracker picks
 /// out variable locations for the live-in variable values (if there _is_ a
MLocTracker::MLocTracker(MachineFunction &MF, const TargetInstrInfo &TII,
   LocIDToLocIdx.resize(NumRegs, LocIdx::MakeIllegalLoc());
   assert(NumRegs < (1u << NUM_LOC_BITS)); // Detect bit packing failure
 
   // Always track SP. This avoids the implicit clobbering caused by regmasks
   // from affectings its values. (LiveDebugValues disbelieves calls and
   // regmasks that claim to clobber SP).
   Register SP = TLI.getStackPointerRegisterToSaveRestore();
   if (SP) {
-    unsigned ID = getLocID(SP, false);
+    unsigned ID = getLocID(SP);
     (void)lookupOrTrackRegister(ID);
 
     for (MCRegAliasIterator RAI(SP, &TRI, true); RAI.isValid(); ++RAI)
       SPAliases.insert(*RAI);
   }
+
+  // Build some common stack positions -- full registers being spilt to the
+  // stack.
+  StackSlotIdxes.insert({{8, 0}, 0});
+  StackSlotIdxes.insert({{16, 0}, 1});
+  StackSlotIdxes.insert({{32, 0}, 2});
+  StackSlotIdxes.insert({{64, 0}, 3});
+  StackSlotIdxes.insert({{128, 0}, 4});
+  StackSlotIdxes.insert({{256, 0}, 5});
+  StackSlotIdxes.insert({{512, 0}, 6});
+
+  // Traverse all the subregister idxes, and ensure there's an index for them.
+  // Duplicates are no problem: we're interested in their position in the
+  // stack slot, we don't want to type the slot.
+  for (unsigned int I = 1; I < TRI.getNumSubRegIndices(); ++I) {
+    unsigned Size = TRI.getSubRegIdxSize(I);
+    unsigned Offs = TRI.getSubRegIdxOffset(I);
+    unsigned Idx = StackSlotIdxes.size();
+
+    // Some subregs have -1, -2 and so forth fed into their fields, to mean
+    // special backend things. Ignore those.
+    if (Size > 60000 || Offs > 60000)
+      continue;
+
+    StackSlotIdxes.insert({{Size, Offs}, Idx});
+  }
+
+  for (auto &Idx : StackSlotIdxes)
+    StackIdxesToPos[Idx.second] = Idx.first;
+
+  NumSlotIdxes = StackSlotIdxes.size();
 }
 
 LocIdx MLocTracker::trackRegister(unsigned ID) {
   assert(ID != 0);
   LocIdx NewIdx = LocIdx(LocIdxToIDNum.size());
   LocIdxToIDNum.grow(NewIdx);
   LocIdxToLocID.grow(NewIdx);
 
for (auto Location : locations()) {
     unsigned ID = LocIdxToLocID[Location.Idx];
     // Don't clobber SP, even if the mask says it's clobbered.
     if (ID < NumRegs && !SPAliases.count(ID) && MO->clobbersPhysReg(ID))
       defReg(ID, CurBB, InstID);
   }
   Masks.push_back(std::make_pair(MO, InstID));
 }
 
-LocIdx MLocTracker::getOrTrackSpillLoc(SpillLoc L) {
-  unsigned SpillID = SpillLocs.idFor(L);
-  if (SpillID == 0) {
-    SpillID = SpillLocs.insert(L);
-    unsigned L = getLocID(SpillID, true);
+SpillLocationNo MLocTracker::getOrTrackSpillLoc(SpillLoc L) {
+  SpillLocationNo SpillID(SpillLocs.idFor(L));
+  if (SpillID.id() == 0) {
+    // Spill location is untracked: create record for this one, and all
+    // subregister slots too.
+    SpillID = SpillLocationNo(SpillLocs.insert(L));
+    for (unsigned StackIdx = 0; StackIdx < NumSlotIdxes; ++StackIdx) {
+      unsigned L = getSpillIDWithIdx(SpillID, StackIdx);
       LocIdx Idx = LocIdx(LocIdxToIDNum.size()); // New idx
       LocIdxToIDNum.grow(Idx);
       LocIdxToLocID.grow(Idx);
       LocIDToLocIdx.push_back(Idx);
       LocIdxToLocID[Idx] = L;
-    return Idx;
-  } else {
-    unsigned L = getLocID(SpillID, true);
-    LocIdx Idx = LocIDToLocIdx[L];
-    return Idx;
+      // Initialize to PHI value; corresponds to the location's live-in value
+      // during transfer function construction.
+      LocIdxToIDNum[Idx] = ValueIDNum(CurBB, 0, Idx);
     }
   }
+  return SpillID;
+}
 
 std::string MLocTracker::LocIdxToName(LocIdx Idx) const {
   unsigned ID = LocIdxToLocID[Idx];
-  if (ID >= NumRegs)
-    return Twine("slot ").concat(Twine(ID - NumRegs)).str();
-  else
+  if (ID >= NumRegs) {
+    StackSlotPos Pos = locIDToSpillIdx(ID);
+    ID -= NumRegs;
+    unsigned Slot = ID / NumSlotIdxes;
+    return Twine("slot ")
+        .concat(Twine(Slot).concat(Twine(" sz ").concat(Twine(Pos.first)
+        .concat(Twine(" offs ").concat(Twine(Pos.second))))))
+        .str();
+  } else {
     return TRI.getRegAsmName(ID).str();
   }
+}
 
 std::string MLocTracker::IDAsString(const ValueIDNum &Num) const {
   std::string DefName = LocIdxToName(Num.getLoc());
   return Num.asString(DefName);
 }
 
 #ifndef NDEBUG
 LLVM_DUMP_METHOD void MLocTracker::dump() {
MachineInstrBuilder MLocTracker::emitLoc(Optional<LocIdx> MLoc,
 
   const DIExpression *Expr = Properties.DIExpr;
   if (!MLoc) {
     // No location -> DBG_VALUE $noreg
     MIB.addReg(0);
     MIB.addReg(0);
   } else if (LocIdxToLocID[*MLoc] >= NumRegs) {
     unsigned LocID = LocIdxToLocID[*MLoc];
-    const SpillLoc &Spill = SpillLocs[LocID - NumRegs + 1];
-
-    auto *TRI = MF.getSubtarget().getRegisterInfo();
-    Expr = TRI->prependOffsetExpression(Expr, DIExpression::ApplyOffset,
+    SpillLocationNo SpillID = locIDToSpill(LocID);
+    StackSlotPos StackIdx = locIDToSpillIdx(LocID);
+    unsigned short Offset = StackIdx.second;
+
+    // TODO: support variables that are located in spill slots, with non-zero
+    // offsets from the start of the spill slot. It would require some more
+    // complex DIExpression calculations. This doesn't seem to be produced by
+    // LLVM right now, so don't try and support it.
+    // Accept no-subregister slots and subregisters where the offset is zero.
+    // The consumer should already have type information to work out how large
+    // the variable is.
+    if (Offset == 0) {
+      const SpillLoc &Spill = SpillLocs[SpillID.id()];
+      Expr = TRI.prependOffsetExpression(Expr, DIExpression::ApplyOffset,
                                          Spill.SpillOffset);
       unsigned Base = Spill.SpillBase;
       MIB.addReg(Base);
       MIB.addImm(0);
     } else {
+      // This is a stack location with a weird subregister offset: emit an undef
+      // DBG_VALUE instead.
+      MIB.addReg(0);
+      MIB.addReg(0);
+    }
+  } else {
+    // Non-empty, non-stack slot, must be a plain register.
     unsigned LocID = LocIdxToLocID[*MLoc];
     MIB.addReg(LocID);
     if (Properties.Indirect)
       MIB.addImm(0);
     else
       MIB.addReg(0);
   }
 

 //            Debug Range Extension Implementation
 //===----------------------------------------------------------------------===//
 
 #ifndef NDEBUG
 // Something to restore in the future.
 // void InstrRefBasedLDV::printVarLocInMBB(..)
 #endif
 
-SpillLoc
+SpillLocationNo
 InstrRefBasedLDV::extractSpillBaseRegAndOffset(const MachineInstr &MI) {
   assert(MI.hasOneMemOperand() &&
          "Spill instruction does not have exactly one memory operand?");
   auto MMOI = MI.memoperands_begin();
   const PseudoSourceValue *PVal = (*MMOI)->getPseudoValue();
   assert(PVal->kind() == PseudoSourceValue::FixedStack &&
          "Inconsistent memory operand in spill instruction");
   int FI = cast<FixedStackPseudoSourceValue>(PVal)->getFrameIndex();
   const MachineBasicBlock *MBB = MI.getParent();
   Register Reg;
   StackOffset Offset = TFI->getFrameIndexReference(*MBB->getParent(), FI, Reg);
-  return {Reg, Offset};
+  return MTracker->getOrTrackSpillLoc({Reg, Offset});
 }
 
 /// End all previous ranges related to @MI and start a new range from @MI
 /// if it is a DBG_VALUE instr.
 bool InstrRefBasedLDV::transferDebugValue(const MachineInstr &MI) {
   if (!MI.isDebugValue())
     return false;
 
if (InstrIt != DebugInstrNumToInstr.end()) {
 
     // Pick out the designated operand.
     assert(OpNo < TargetInstr.getNumOperands());
     const MachineOperand &MO = TargetInstr.getOperand(OpNo);
 
     // Today, this can only be a register.
     assert(MO.isReg() && MO.isDef());
 
-    unsigned LocID = MTracker->getLocID(MO.getReg(), false);
+    unsigned LocID = MTracker->getLocID(MO.getReg());
     LocIdx L = MTracker->LocIDToLocIdx[LocID];
     NewID = ValueIDNum(BlockNo, InstrIt->second.second, L);
   } else if (PHIIt != DebugPHINumToValue.end() && PHIIt->InstrNum == InstNo) {
     // It's actually a PHI value. Which value it is might not be obvious, use
     // the resolver helper to find out.
     NewID = resolveDbgPHIs(*MI.getParent()->getParent(), MLiveOuts, MLiveIns,
                            MI, InstNo);
   }
if (MO.isReg()) {
     // The value is whatever's currently in the register. Read and record it,
     // to be analysed later.
     Register Reg = MO.getReg();
     ValueIDNum Num = MTracker->readReg(Reg);
     auto PHIRec = DebugPHIRecord(
         {InstrNum, MI.getParent(), Num, MTracker->lookupOrTrackRegister(Reg)});
     DebugPHINumToValue.push_back(PHIRec);
 
-    // Subsequent register operations, or variable locations, might occur for
-    // any of the subregisters of this DBG_PHIs operand. Ensure that all
-    // registers aliasing this register are tracked.
+    // Ensure this register is tracked.
     for (MCRegAliasIterator RAI(MO.getReg(), TRI, true); RAI.isValid(); ++RAI)
       MTracker->lookupOrTrackRegister(*RAI);
   } else {
     // The value is whatever's in this stack slot.
     assert(MO.isFI());
     unsigned FI = MO.getIndex();
 
     // If the stack slot is dead, then this was optimized away.
     // FIXME: stack slot colouring should account for slots that get merged.
     if (MFI->isDeadObjectIndex(FI))
       return true;
 
-    // Identify this spill slot.
+    // Identify this spill slot, ensure it's tracked.
     Register Base;
     StackOffset Offs = TFI->getFrameIndexReference(*MI.getMF(), FI, Base);
     SpillLoc SL = {Base, Offs};
-    Optional<ValueIDNum> Num = MTracker->readSpill(SL);
+    SpillLocationNo SpillNo = MTracker->getOrTrackSpillLoc(SL);
 
-    if (!Num)
-      // Nothing ever writes to this slot. Curious, but nothing we can do.
-      return true;
+    // Problem: what value should we extract from the stack? LLVM does not
+    // record what size the last store to the slot was, and it would become
+    // sketchy after stack slot colouring anyway. Take a look at what values
+    // are stored on the stack, and pick the largest one that wasn't def'd
+    // by a spill (i.e., the value most likely to have been def'd in a register
+    // and then spilt.
+    std::array<unsigned, 4> CandidateSizes = {64, 32, 16, 8};
+    Optional<ValueIDNum> Result = None;
+    Optional<LocIdx> SpillLoc = None;
+    for (unsigned int I = 0; I < CandidateSizes.size(); ++I) {
+      unsigned SpillID = MTracker->getLocID(SpillNo, {CandidateSizes[I], 0});
+      SpillLoc = MTracker->getSpillMLoc(SpillID);
+      ValueIDNum Val = MTracker->readMLoc(*SpillLoc);
+      // If this value was defined in it's own position, then it was probably
+      // an aliasing index of a small value that was spilt.
+      if (Val.getLoc() != SpillLoc->asU64()) {
+        Result = Val;
+        break;
+      }
+    }
+
+    // If we didn't find anything, we're probably looking at a PHI, or a memory
+    // store folded into an instruction. FIXME: Take a guess that's it's 64
+    // bits. This isn't ideal, but tracking the size that the spill is
+    // "supposed" to be is more complex, and benefits a small number of
+    // locations.
+    if (!Result) {
+      unsigned SpillID = MTracker->getLocID(SpillNo, {64, 0});
+      SpillLoc = MTracker->getSpillMLoc(SpillID);
+      Result = MTracker->readMLoc(*SpillLoc);
+    }
 
     // Record this DBG_PHI for later analysis.
-    auto DbgPHI = DebugPHIRecord(
-        {InstrNum, MI.getParent(), *Num, *MTracker->getSpillMLoc(SL)});
+    auto DbgPHI = DebugPHIRecord({InstrNum, MI.getParent(), *Result, *SpillLoc});
     DebugPHINumToValue.push_back(DbgPHI);
   }
 
   return true;
 }
 
 void InstrRefBasedLDV::transferRegisterDef(MachineInstr &MI) {
   // Meta Instructions do not affect the debug liveness of any register they
bool InstrRefBasedLDV::isLocationSpill(const MachineInstr &MI,
   if (!isSpillInstruction(MI, MF))
     return false;
 
   int FI;
   Reg = TII->isStoreToStackSlotPostFE(MI, FI);
   return Reg != 0;
 }
 
-Optional<SpillLoc>
+Optional<SpillLocationNo>
 InstrRefBasedLDV::isRestoreInstruction(const MachineInstr &MI,
                                        MachineFunction *MF, unsigned &Reg) {
   if (!MI.hasOneMemOperand())
     return None;
 
   // FIXME: Handle folded restore instructions with more than one memory
   // operand.
   if (MI.getRestoreSize(TII)) {
     Reg = MI.getOperand(0).getReg();
     return extractSpillBaseRegAndOffset(MI);
   }
   return None;
 }
 
 bool InstrRefBasedLDV::transferSpillOrRestoreInst(MachineInstr &MI) {
   // XXX -- it's too difficult to implement VarLocBasedImpl's  stack location
   // limitations under the new model. Therefore, when comparing them, compare
   // versions that don't attempt spills or restores at all.
   if (EmulateOldLDV)
     return false;
 
+  // Strictly limit ourselves to plain loads and stores, not all instructions
+  // that can access the stack.
+  int DummyFI = -1;
+  if (!TII->isStoreToStackSlotPostFE(MI, DummyFI) &&
+      !TII->isLoadFromStackSlotPostFE(MI, DummyFI))
+    return false;
+
   MachineFunction *MF = MI.getMF();
   unsigned Reg;
-  Optional<SpillLoc> Loc;
 
   LLVM_DEBUG(dbgs() << "Examining instruction: "; MI.dump(););
 
   // First, if there are any DBG_VALUEs pointing at a spill slot that is
   // written to, terminate that variable location. The value in memory
   // will have changed. DbgEntityHistoryCalculator doesn't try to detect this.
   if (isSpillInstruction(MI, MF)) {
-    Loc = extractSpillBaseRegAndOffset(MI);
+    SpillLocationNo Loc = extractSpillBaseRegAndOffset(MI);
 
-    if (TTracker) {
-      Optional<LocIdx> MLoc = MTracker->getSpillMLoc(*Loc);
-      if (MLoc) {
-        // Un-set this location before clobbering, so that we don't salvage
-        // the variable location back to the same place.
-        MTracker->setMLoc(*MLoc, ValueIDNum::EmptyValue);
+    // Un-set this location and clobber, so that earlier locations don't
+    // continue past this store.
+    for (unsigned SlotIdx = 0; SlotIdx < MTracker->NumSlotIdxes; ++SlotIdx) {
+      unsigned SpillID = MTracker->getSpillIDWithIdx(Loc, SlotIdx);
+      Optional<LocIdx> MLoc = MTracker->getSpillMLoc(SpillID);
+      if (!MLoc)
+        continue;
+
+      // We need to over-write the stack slot with something (here, a def at
+      // this instruction) to ensure no values are preserved in this stack slot
+      // after the spill. It also prevents TTracker from trying to recover the
+      // location and re-installing it in the same place.
+      ValueIDNum Def(CurBB, CurInst, *MLoc);
+      MTracker->setMLoc(*MLoc, Def);
+      if (TTracker)
         TTracker->clobberMloc(*MLoc, MI.getIterator());
-      }
-    }
-  }
 
   // Try to recognise spill and restore instructions that may transfer a value.
   if (isLocationSpill(MI, MF, Reg)) {
-    Loc = extractSpillBaseRegAndOffset(MI);
-    auto ValueID = MTracker->readReg(Reg);
+    SpillLocationNo Loc = extractSpillBaseRegAndOffset(MI);
 
-    // If the location is empty, produce a phi, signify it's the live-in value.
-    if (ValueID.getLoc() == 0)
-      ValueID = {CurBB, 0, MTracker->getRegMLoc(Reg)};
-
-    MTracker->setSpill(*Loc, ValueID);
-    auto OptSpillLocIdx = MTracker->getSpillMLoc(*Loc);
-    assert(OptSpillLocIdx && "Spill slot set but has no LocIdx?");
-    LocIdx SpillLocIdx = *OptSpillLocIdx;
+    auto DoTransfer = [&](Register SrcReg, unsigned SpillID) {
+      auto ReadValue = MTracker->readReg(SrcReg);
+      LocIdx DstLoc = MTracker->getSpillMLoc(SpillID);
+      MTracker->setMLoc(DstLoc, ReadValue);
 
-    // Tell TransferTracker about this spill, produce DBG_VALUEs for it.
-    if (TTracker)
-      TTracker->transferMlocs(MTracker->getRegMLoc(Reg), SpillLocIdx,
-                              MI.getIterator());
+      if (TTracker) {
+        LocIdx SrcLoc = MTracker->getRegMLoc(SrcReg);
+        TTracker->transferMlocs(SrcLoc, DstLoc, MI.getIterator());
+      }
+    };
+
+    // Then, transfer subreg bits.
+    for (MCSubRegIterator SRI(Reg, TRI, false); SRI.isValid(); ++SRI) {
+      // Ensure this reg is tracked,
+      (void)MTracker->lookupOrTrackRegister(*SRI);
+      unsigned SubregIdx = TRI->getSubRegIndex(Reg, *SRI);
+      unsigned SpillID = MTracker->getLocID(Loc, SubregIdx);
+      DoTransfer(*SRI, SpillID);
+    }
+
+    // Directly lookup size of main source reg, and transfer.
+    unsigned Size = TRI->getRegSizeInBits(Reg, *MRI);
+    unsigned SpillID = MTracker->getLocID(Loc, {Size, 0});
+    DoTransfer(Reg, SpillID);
   } else {
-    if (!(Loc = isRestoreInstruction(MI, MF, Reg)))
+    Optional<SpillLocationNo> OptLoc = isRestoreInstruction(MI, MF, Reg);
+    if (!OptLoc)
       return false;
+    SpillLocationNo Loc = *OptLoc;
 
-    // Is there a value to be restored?
-    auto OptValueID = MTracker->readSpill(*Loc);
-    if (OptValueID) {
-      ValueIDNum ValueID = *OptValueID;
-      LocIdx SpillLocIdx = *MTracker->getSpillMLoc(*Loc);
-      // XXX -- can we recover sub-registers of this value? Until we can, first
-      // overwrite all defs of the register being restored to.
-      for (MCRegAliasIterator RAI(Reg, TRI, true); RAI.isValid(); ++RAI)
-        MTracker->defReg(*RAI, CurBB, CurInst);
-
-      // Now override the reg we're restoring to.
-      MTracker->setReg(Reg, ValueID);
+    // Assumption: we're reading from the base of the stack slot, not some
+    // offset into it. It seems very unlikely LLVM would ever generate
+    // restores where this wasn't true. This then becomes a question of what
+    // subregisters in the destination register line up with positions in the
+    // stack slot.
 
-      // Report this restore to the transfer tracker too.
-      if (TTracker)
-        TTracker->transferMlocs(SpillLocIdx, MTracker->getRegMLoc(Reg),
-                                MI.getIterator());
-    } else {
-      // There isn't anything in the location; not clear if this is a code path
-      // that still runs. Def this register anyway just in case.
-      for (MCRegAliasIterator RAI(Reg, TRI, true); RAI.isValid(); ++RAI)
-        MTracker->defReg(*RAI, CurBB, CurInst);
+    // Def all registers that alias the destination.
 
-      // Force the spill slot to be tracked.
-      LocIdx L = MTracker->getOrTrackSpillLoc(*Loc);
+    // Now find subregisters within the destination register, and load values
+    // from stack slot positions.
+    auto DoTransfer = [&](Register DestReg, unsigned SpillID) {
+      LocIdx SrcIdx = MTracker->getSpillMLoc(SpillID);
+      auto ReadValue = MTracker->readMLoc(SrcIdx);
+      MTracker->setReg(DestReg, ReadValue);
+
+      if (TTracker) {
+        LocIdx DstLoc = MTracker->getRegMLoc(DestReg);
+        TTracker->transferMlocs(SrcIdx, DstLoc, MI.getIterator());
+      }
+    };
 
-      // Set the restored value to be a machine phi number, signifying that it's
-      // whatever the spills live-in value is in this block. Definitely has
-      // a LocIdx due to the setSpill above.
-      ValueIDNum ValueID = {CurBB, 0, L};
-      MTracker->setReg(Reg, ValueID);
-      MTracker->setSpill(*Loc, ValueID);
+    for (MCSubRegIterator SRI(Reg, TRI, false); SRI.isValid(); ++SRI) {
+      unsigned Subreg = TRI->getSubRegIndex(Reg, *SRI);
+      unsigned SpillID = MTracker->getLocID(Loc, Subreg);
+      DoTransfer(*SRI, SpillID);
     }
+
+    // Directly look up this registers slot idx by size, and transfer.
+    unsigned Size = TRI->getRegSizeInBits(Reg, *MRI);
+    unsigned SpillID = MTracker->getLocID(Loc, {Size, 0});
+    DoTransfer(Reg, SpillID);
   }
   return true;
 }
 
 bool InstrRefBasedLDV::transferRegisterCopy(MachineInstr &MI) {
   auto DestSrc = TII->isCopyInstr(MI);
   if (!DestSrc)
     return false;
void InstrRefBasedLDV::produceMLocTransferFunction(
   for (unsigned int I = 0; I < MaxNumBlocks; ++I) {
     BitVector &BV = BlockMasks[I];
     BV.flip();
     BV &= UsedRegs;
     // This produces all the bits that we clobber, but also use. Check that
     // they're all clobbered or at least set in the designated transfer
     // elem.
     for (unsigned Bit : BV.set_bits()) {
-      unsigned ID = MTracker->getLocID(Bit, false);
+      unsigned ID = MTracker->getLocID(Bit);
       LocIdx Idx = MTracker->LocIDToLocIdx[ID];
       auto &TransferMap = MLocTransfer[I];
 
       // Install a value representing the fact that this location is effectively
       // written to in this block. As there's no reserved value, instead use
       // a value number that is never generated. Pick the value number for the
       // first instruction in the block, def'ing this location, which we know
       // this block never used anyway.
bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   if (!MF.getFunction().getSubprogram())
     return false;
 
   LLVM_DEBUG(dbgs() << "\nDebug Range Extension\n");
   this->TPC = TPC;
 
   this->DomTree = DomTree;
   TRI = MF.getSubtarget().getRegisterInfo();
+  MRI = &MF.getRegInfo();
   TII = MF.getSubtarget().getInstrInfo();
   TFI = MF.getSubtarget().getFrameLowering();
   TFI->getCalleeSaves(MF, CalleeSavedRegs);
   MFI = &MF.getFrameInfo();
   LS.initialize(MF);
 
   MTracker =
       new MLocTracker(MF, *TII, *TRI, *MF.getSubtarget().getTargetLowering());

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_stackslot_subregs.mir

+# RUN: llc %s -march=x86-64 -run-pass=livedebugvalues -experimental-debug-variable-locations -o - 2>&1 | FileCheck %s
+#
+# Test that we can spill and restore through subregisters too. In this test,
+# eax is def'd and then spilt, but as part of a larger register.
+--- |
+  define i8 @test(i32 %bar) local_unnamed_addr !dbg !7 {
+  entry:
+    ret i8 0, !dbg !12
+  }
+
+  declare dso_local void @ext(i64)
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5, !6}
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+  !1 = !DIFile(filename: "foo.cpp", directory: ".")
+  !2 = !DIBasicType(name: "int", size: 8, encoding: DW_ATE_signed)
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 2}
+  !6 = !{i32 7, !"PIC Level", i32 2}
+  !7 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 6, type: !8, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!2, !2}
+  !10 = !{!11}
+  !11 = !DILocalVariable(name: "baz", scope: !7, file: !1, line: 7, type: !2)
+  !12 = !DILocation(line: 10, scope: !7)
+...
+---
+name: test
+tracksRegLiveness: true
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+stack:
+  - { id: 0, name: '', type: spill-slot, offset: -16, size: 8, alignment: 8,
+      stack-id: default, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:  |
+  bb.0:
+  liveins: $rdi, $rax, $rbx
+    $eax = MOV32ri 0, debug-instr-number 1
+    $edi = COPY $eax
+    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rdi :: (store 8 into %stack.0)
+    $rsi = MOV64rm $rsp, 1, $noreg, 8, $noreg :: (load 8 from %stack.0)
+
+    ; Store a random value onto stack, forces value to be in one place only.
+    ; Clobber other registers that contain the value we're to track.
+    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rbx :: (store 8 into %stack.0)
+    $rax = MOV64ri 0
+    $rdi = MOV64ri 0
+
+    DBG_INSTR_REF 1, 0, !11, !DIExpression(), debug-location !12
+    ; CHECK:      DBG_INSTR_REF
+    ; CHECK-NEXT: DBG_VALUE $esi
+    RETQ $rsi, debug-location !12
+...

llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_subreg_substitutions.mir

liveins: $rdi, $rax
     ; CHECK-NEXT: DBG_INSTR_REF 8, 0
     ; CHECK-NEXT: DBG_VALUE $ah
     DBG_INSTR_REF 13, 0, !11, !DIExpression(), debug-location !12
     ; CHECK-NEXT: DBG_INSTR_REF 13, 0
     ; CHECK-NEXT: DBG_VALUE $noreg
     MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax :: (store 8 into %stack.0)
     $rax = MOV64ri 0, debug-location !12
     ; CHECK:      $rax = MOV64ri 0
-    ; The value is now located in a spill slot; currently InstrRefBasedLDV
-    ; can't express subregister locations inside spills. These should all
-    ; end up being $noreg, but could be improved in the future.
+    ; The value is now located in a spill slot, as a subregister within the
+    ; slot, which InstrRefBasedLDV should be able to find.
     DBG_INSTR_REF 1, 0, !11, !DIExpression(), debug-location !12
     ; CHECK-NEXT: DBG_INSTR_REF 1, 0
-    ; CHECK-NEXT: DBG_VALUE $noreg
+    ; CHECK-NEXT: DBG_VALUE $rsp, 0, !{{[0-9]*}}, !DIExpression(DW_OP_constu, 8, DW_OP_minus)
     DBG_INSTR_REF 5, 0, !11, !DIExpression(), debug-location !12
+    ; This and the next DBG_INSTR_REF refer to a value that is on the stack, but
+    ; is located at a non-zero offset from the start of the slot -- $ah within
+    ; $rax is 8 bits in. Today, InstrRefBasedLDV can't express this. It also
+    ; doesn't seem likely to be profitable.
     ; CHECK-NEXT: DBG_INSTR_REF 5, 0
     ; CHECK-NEXT: DBG_VALUE $noreg
     DBG_INSTR_REF 8, 0, !11, !DIExpression(), debug-location !12
     ; CHECK-NEXT: DBG_INSTR_REF 8, 0
     ; CHECK-NEXT: DBG_VALUE $noreg
     DBG_INSTR_REF 13, 0, !11, !DIExpression(), debug-location !12
     ; CHECK-NEXT: DBG_INSTR_REF 13, 0
     ; CHECK-NEXT: DBG_VALUE $noreg
     $rax = MOV64rm $rsp, 1, $noreg, 8, $noreg :: (load 8 from %stack.0)
     RETQ $rax, debug-location !12
 ...

llvm/unittests/CodeGen/InstrRefLDVTest.cpp

TEST_F(InstrRefLDVTest, MTransferCopies) {
   TransferMap.resize(1);
   produceMLocTransferFunction(*MF, TransferMap, 1);
 
   // Check that the spill location contains the value defined in rax by
   // instruction 1. The MIR header says -16 offset, but it's stored as -8;
   // it's not completely clear why, but here we only care about correctly
   // identifying the slot, not that all the surrounding data is correct.
   SpillLoc L = {getRegByName("RSP"), StackOffset::getFixed(-8)};
-  Optional<ValueIDNum> V = MTracker->readSpill(L);
-  ASSERT_TRUE(V);
+  SpillLocationNo SpillNo = MTracker->getOrTrackSpillLoc(L);
+  unsigned SpillLocID = MTracker->getLocID(SpillNo, {64, 0});
+  LocIdx SpillLoc = MTracker->getSpillMLoc(SpillLocID);
+  ValueIDNum V = MTracker->readMLoc(SpillLoc);
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->getRegMLoc(RAX);
   ValueIDNum Cmp(0, 1, RaxLoc);
-  EXPECT_EQ(*V, Cmp);
+  EXPECT_EQ(V, Cmp);
 
   // A spill and restore should be recognised.
   MF = readMIRBlock(
    "    $rax = MOV64ri 0\n"
    "    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax :: (store 8 into %stack.0)\n"
    "    $rbx = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load 8 from %stack.0)\n"
    "    RETQ\n");
   setupLDVObj(MF);
   TransferMap.clear();
   TransferMap.resize(1);
   produceMLocTransferFunction(*MF, TransferMap, 1);
 
   // Test that rbx contains rax from instruction 1.
   RAX = getRegByName("RAX");
   RaxLoc = MTracker->getRegMLoc(RAX);
   Register RBX = getRegByName("RBX");
   LocIdx RbxLoc = MTracker->getRegMLoc(RBX);
   Cmp = ValueIDNum(0, 1, RaxLoc);
   ValueIDNum RbxVal = MTracker->readMLoc(RbxLoc);
   EXPECT_EQ(RbxVal, Cmp);
 
-  // FIXME: future work, make sure all the subregisters are transferred too.
+  // Testing that all the subregisters are transferred happens in
+  // MTransferSubregSpills.
 
   // Copies and x86 movs should be recognised and honoured. In addition, all
   // of the subregisters should be copied across too.
   MF = readMIRBlock(
    "    $rax = MOV64ri 0\n"
    "    $rcx = COPY $rax\n"
    "    $rbx = MOV64rr $rcx\n"
    "    RETQ\n");
TEST_F(InstrRefLDVTest, MTransferCopies) {
 
   // But rcx should contain a value defined by the COPY.
   LocIdx RcxLoc = MTracker->getRegMLoc(getRegByName("RCX"));
   ValueIDNum RcxVal = MTracker->readMLoc(RcxLoc);
   ValueIDNum RcxDefVal(0, 2, RcxLoc); // instr 2 -> the copy
   EXPECT_EQ(RcxVal, RcxDefVal);
 }
 
+TEST_F(InstrRefLDVTest, MTransferSubregSpills) {
+  SmallVector<MLocTransferMap, 1> TransferMap;
+  MachineFunction *MF = readMIRBlock(
+   "    $rax = MOV64ri 0\n"
+   "    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax :: (store 8 into %stack.0)\n"
+   "    $rbx = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load 8 from %stack.0)\n"
+   "    RETQ\n");
+  setupLDVObj(MF);
+  TransferMap.clear();
+  TransferMap.resize(1);
+  produceMLocTransferFunction(*MF, TransferMap, 1);
+
+  // Check that all the subregs of rax and rbx contain the same values. One
+  // should completely transfer to the other.
+  const char *ARegs[] = {"AL", "AH", "AX", "EAX", "HAX", "RAX"};
+  const char *BRegs[] = {"BL", "BH", "BX", "EBX", "HBX", "RBX"};
+  for (unsigned int I = 0; I < 6; ++I) {
+    LocIdx A = MTracker->getRegMLoc(getRegByName(ARegs[I]));
+    LocIdx B = MTracker->getRegMLoc(getRegByName(BRegs[I]));
+    EXPECT_EQ(MTracker->readMLoc(A), MTracker->readMLoc(B));
+  }
+
+  // Explicitly check what's in the different subreg slots, on the stack.
+  // Pair up subreg idx fields with the corresponding subregister in $rax.
+  MLocTracker::StackSlotPos SubRegIdxes[] = {{8, 0}, {8, 8}, {16, 0}, {32, 0}, {64, 0}};
+  const char *SubRegNames[] = {"AL", "AH", "AX", "EAX", "RAX"};
+  for (unsigned int I = 0; I < 5; ++I) {
+    // Value number where it's defined,
+    LocIdx RegLoc = MTracker->getRegMLoc(getRegByName(SubRegNames[I]));
+    ValueIDNum DefNum(0, 1, RegLoc);
+    // Read the corresponding subreg field from the stack.
+    SpillLoc L = {getRegByName("RSP"), StackOffset::getFixed(-8)};
+    SpillLocationNo SpillNo = MTracker->getOrTrackSpillLoc(L);
+    unsigned SpillID = MTracker->getLocID(SpillNo, SubRegIdxes[I]);
+    LocIdx SpillLoc = MTracker->getSpillMLoc(SpillID);
+    ValueIDNum SpillValue = MTracker->readMLoc(SpillLoc);
+    EXPECT_EQ(DefNum, SpillValue);
+  }
+
+  // If we have exactly the same code, but we write $eax to the stack slot after
+  // $rax, then we should still have exactly the same output in the lower five
+  // subregisters. Storing $eax to the start of the slot will overwrite with the
+  // same values. $rax, as an aliasing register, should be reset to something
+  // else by that write.
+  // In theory, we could try and recognise that we're writing the _same_ values
+  // to the stack again, and so $rax doesn't need to be reset to something else.
+  // It seems vanishingly unlikely that LLVM would generate such code though,
+  // so the benefits would be small.
+  MF = readMIRBlock(
+   "    $rax = MOV64ri 0\n"
+   "    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax :: (store 8 into %stack.0)\n"
+   "    MOV32mr $rsp, 1, $noreg, 16, $noreg, $eax :: (store 4 into %stack.0)\n"
+   "    $rbx = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load 8 from %stack.0)\n"
+   "    RETQ\n");
+  setupLDVObj(MF);
+  TransferMap.clear();
+  TransferMap.resize(1);
+  produceMLocTransferFunction(*MF, TransferMap, 1);
+
+  // Check lower five registers up to and include $eax == $ebx,
+  for (unsigned int I = 0; I < 5; ++I) {
+    LocIdx A = MTracker->getRegMLoc(getRegByName(ARegs[I]));
+    LocIdx B = MTracker->getRegMLoc(getRegByName(BRegs[I]));
+    EXPECT_EQ(MTracker->readMLoc(A), MTracker->readMLoc(B));
+  }
+
+  // $rbx should contain something else; today it's a def at the spill point
+  // of the 4 byte value.
+  SpillLoc L = {getRegByName("RSP"), StackOffset::getFixed(-8)};
+  SpillLocationNo SpillNo = MTracker->getOrTrackSpillLoc(L);
+  unsigned SpillID = MTracker->getLocID(SpillNo, {64, 0});
+  LocIdx Spill64Loc = MTracker->getSpillMLoc(SpillID);
+  ValueIDNum DefAtSpill64(0, 3, Spill64Loc);
+  LocIdx RbxLoc = MTracker->getRegMLoc(getRegByName("RBX"));
+  EXPECT_EQ(MTracker->readMLoc(RbxLoc), DefAtSpill64);
+
+  // Same again, test that the lower four subreg slots on the stack are the
+  // value defined by $rax in instruction 1.
+  for (unsigned int I = 0; I < 4; ++I) {
+    // Value number where it's defined,
+    LocIdx RegLoc = MTracker->getRegMLoc(getRegByName(SubRegNames[I]));
+    ValueIDNum DefNum(0, 1, RegLoc);
+    // Read the corresponding subreg field from the stack.
+    SpillNo = MTracker->getOrTrackSpillLoc(L);
+    SpillID = MTracker->getLocID(SpillNo, SubRegIdxes[I]);
+    LocIdx SpillLoc = MTracker->getSpillMLoc(SpillID);
+    ValueIDNum SpillValue = MTracker->readMLoc(SpillLoc);
+    EXPECT_EQ(DefNum, SpillValue);
+  }
+
+  // Stack slot for $rax should be a different value, today it's EmptyValue.
+  ValueIDNum SpillValue = MTracker->readMLoc(Spill64Loc);
+  EXPECT_EQ(SpillValue, DefAtSpill64);
+
+  // If we write something to the stack, then over-write with some register
+  // from a completely different hierarchy, none of the "old" values should be
+  // readable.
+  // NB: slight hack, store 16 in to a 8 byte stack slot.
+  MF = readMIRBlock(
+   "    $rax = MOV64ri 0\n"
+   "    MOV64mr $rsp, 1, $noreg, 16, $noreg, $rax :: (store 8 into %stack.0)\n"
+   "    $xmm0 = IMPLICIT_DEF\n"
+   "    MOVUPDmr $rsp, 1, $noreg, 16, $noreg, killed $xmm0 :: (store (s128) into %stack.0)\n"
+   "    $rbx = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load 8 from %stack.0)\n"
+   "    RETQ\n");
+  setupLDVObj(MF);
+  TransferMap.clear();
+  TransferMap.resize(1);
+  produceMLocTransferFunction(*MF, TransferMap, 1);
+
+  for (unsigned int I = 0; I < 5; ++I) {
+    // Read subreg fields from the stack.
+    SpillLocationNo SpillNo = MTracker->getOrTrackSpillLoc(L);
+    unsigned SpillID = MTracker->getLocID(SpillNo, SubRegIdxes[I]);
+    LocIdx SpillLoc = MTracker->getSpillMLoc(SpillID);
+    ValueIDNum SpillValue = MTracker->readMLoc(SpillLoc);
+
+    // Value should be defined by the spill-to-xmm0 instr, get value of a def
+    // at the point of the spill.
+    ValueIDNum SpillDef(0, 4, SpillLoc);
+    EXPECT_EQ(SpillValue, SpillDef);
+  }
+
+  // Read xmm0's position and ensure it has a value. Should be the live-in
+  // value to the block, as IMPLICIT_DEF isn't a real def.
+  SpillNo = MTracker->getOrTrackSpillLoc(L);
+  SpillID = MTracker->getLocID(SpillNo, {128, 0});
+  LocIdx Spill128Loc = MTracker->getSpillMLoc(SpillID);
+  SpillValue = MTracker->readMLoc(Spill128Loc);
+  Register XMM0 = getRegByName("XMM0");
+  LocIdx Xmm0Loc = MTracker->getRegMLoc(XMM0);
+  EXPECT_EQ(ValueIDNum(0, 0, Xmm0Loc), SpillValue);
+
+  // What happens if we spill ah to the stack, then load al? It should find
+  // the same value.
+  MF = readMIRBlock(
+   "    $rax = MOV64ri 0\n"
+   "    MOV8mr $rsp, 1, $noreg, 16, $noreg, $ah :: (store 1 into %stack.0)\n"
+   "    $al = MOV8rm $rsp, 1, $noreg, 0, $noreg :: (load 1 from %stack.0)\n"
+   "    RETQ\n");
+  setupLDVObj(MF);
+  TransferMap.clear();
+  TransferMap.resize(1);
+  produceMLocTransferFunction(*MF, TransferMap, 1);
+
+  Register AL = getRegByName("AL");
+  Register AH = getRegByName("AH");
+  LocIdx AlLoc = MTracker->getRegMLoc(AL);
+  LocIdx AhLoc = MTracker->getRegMLoc(AH);
+  ValueIDNum AHDef(0, 1, AhLoc);
+  ValueIDNum ALValue = MTracker->readMLoc(AlLoc);
+  EXPECT_EQ(ALValue, AHDef);
+}
+
 TEST_F(InstrRefLDVTest, MLocSingleBlock) {
   // Test some very simple properties about interpreting the transfer function.
   setupSingleBlock();
 
   // We should start with a single location, the stack pointer.
   ASSERT_TRUE(MTracker->getNumLocs() == 1);
   LocIdx RspLoc(0);