[PATCH] D83047: [LiveDebugValues] 2/4 Add instruction-referencing LiveDebugValues implementation

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

In D83047#2133722 <https://reviews.llvm.org/D83047#2133722>, @vsk wrote:

> Thanks for this, Jeremy. It'll take me multiple passes to page all of this in. I hope to get to the core algorithm changes in my next review. In the interest of getting some feedback to you sooner rather than later, I've included some minor suggestions and questions inline.


Thanks for taking the time!,



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:241
+public:
+  uint64_t BlockNo : 20;       /// The block where the def happens.
+  uint64_t InstNo : 20;        /// The Instruction where the def happens.
----------------
vsk wrote:
> You might find it convenient to use the new bitfield utilities from https://reviews.llvm.org/D82454.
I'll read up on this for a further revision, I'm generally unfamiliar with the bitfield utilities as they are.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:245
+  LocIdx LocNo : NUM_LOC_BITS; /// The machine location where the def happens.
+  // (No idea why this can work as a LocIdx, it probably shouldn't)
+
----------------
vsk wrote:
> I don't follow this caveat, could you rephrase this?
Hmm, I don't think I meant to upload that, now removed.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:313
+  static bool isEqual(LocIdx A, LocIdx B) { return A == B; }
+};
+
----------------
vsk wrote:
> Wdyt of getting rid of these DenseMapInfo specializations? Having special reserved values complicates things a bit. If profiling demonstrates that std::map is a bottleneck, they could be added back.
Sounds like a plan,


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:321
+/// the the value, and Boolean of whether or not it's indirect.
+typedef std::pair<const DIExpression *, bool> MetaVal;
+
----------------
vsk wrote:
> Seems worthwhile to make this a proper class, with a constructor that accepts a MachineInstr and fills out the structure. I also find the name somewhat non-specific. Wdyt of "DbgValueProperties"?
Replaced with a DbgValueProperties class. This flushes out several circumstances where the class was potentially being default-constructed on map-assignment, which it's probably good to suppress.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:360
+  /// as the number of registers on the target -- if the value in the register
+  /// is not being tracked, then the LocIdx value will be zero. New entries are
+  /// appended if a new spill slot begins being tracked.
----------------
vsk wrote:
> Why does there need to be a LocIdx reserved for the case where the value in a register isn't tracked? It doesn't look like this is done for stack slots.
Ah, this is the curse of early optimisation: it's a shortcut so that LocIDToLocIdx can be an array with constant-time lookup to identify whether the corresponding register is tracked or not (signalled by the element being nonzero). This could easily be eliminated by making LocIDToLocIdx an associative array. It isn't necessary for stack slots as they're identified by an associative array elsewhere.

However, outside of MLocTracker, I've been using a zero LocIdx as shorthand for "this is an invalid/empty value/location", in the manner of Optional<> and None. It's not really tied to either registers or stack slots.

This isn't good practice, but there was a stage in prototyping where the machine value number live-in / live-outs of a block could be an empty or null value.  I don't think this is the case any more (95% sure) , I'll spend some time polishing this "feature" out of the implementation.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:669
+/// (DebugVariable specific) dataflow analysis.
+class ValueRec {
+public:
----------------
vsk wrote:
> nit -- "Rec" is suggestive of "recurrence". Wdyt of naming this "DbgValue"?
Works for me, and I guess it drives home that this is the value of the variable.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:674
+  /// If Kind is Const, the MachineOperand defining this value.
+  Optional<MachineOperand> MO;
+  /// Qualifiers for the ValueIDNum above.
----------------
vsk wrote:
> Wdyt of grouping 'ID' and 'MO' in a union? This would make it clear that they cannot both be in use at the same time.
Works for me too.


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

https://reviews.llvm.org/D83047