[PATCH] D112133: [DebugInfo][Instr] Track subregisters across stack spills/restores

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

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.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:751
+      LocIdxToLocID[Idx] = L;
+      // Initialize to PHI value; corresponds to the locations live-in value
+      // during transfer function construction.
----------------
nit: location's


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1201
+    // and then spilt.
+    unsigned CandidateSizes[] = {64, 32, 16, 8};
+    Optional<ValueIDNum> Result = None;
----------------
`std::array` here could be preferable so the `size` can be used in the loop below?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1383
+  // Strictly limit ourselves to plain loads and stores, not all instructions
+  // that can access the stack. Fetch the frame index too.
+  int FI = -1;
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1457
+    // subregisters in the destination register line up with positions in the
+    // stack slot.
 
----------------
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?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:288
 /// 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 un-necessarily tracking
+/// any location.
----------------
nit: unnecessarily


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:350
+  /// Unique-ification of spill. Used to number them -- their LocID number is
+  // the index in SpillLocs minus one plus NumRegs.
   UniqueVector<SpillLoc> SpillLocs;
----------------
nit: missing the 3rd `/` in `///`


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:362
+  /// slot that can take on the value of a subregister, when a super-register
+  // is written to the stack.
+  unsigned NumSlotIdxes;
----------------
nit: missing the 3rd `/` in `///`


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:373
+  /// bits, then the offset.
+  typedef std::pair<unsigned short, unsigned short> StackSlotPos;
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:435
+  /// \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) {
----------------
nit: apram -> param


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:458
+    assert(ID >= NumRegs);
+    ID -= NumRegs;
+    ID /= NumSlotIdxes;
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112133/new/

https://reviews.llvm.org/D112133