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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

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.



================
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.
----------------
You might find it convenient to use the new bitfield utilities from https://reviews.llvm.org/D82454.


================
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)
+
----------------
I don't follow this caveat, could you rephrase this?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:313
+  static bool isEqual(LocIdx A, LocIdx B) { return A == B; }
+};
+
----------------
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.


================
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;
+
----------------
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"?


================
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.
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:393
+    LocIdxToIDNum.push_back({0, 0, LocIdx(0)});
+    LocIDToLocIdx.resize(NumRegs);
+    memset(&LocIDToLocIdx[0], 0, NumRegs * sizeof(LocIdx));
----------------
Could just be `LocIDToLocIdx.assign(NumRegs, LocIdx(0))`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:669
+/// (DebugVariable specific) dataflow analysis.
+class ValueRec {
+public:
----------------
nit -- "Rec" is suggestive of "recurrence". Wdyt of naming this "DbgValue"?


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83047