[PATCH] D133927: [DebugInfo] Add support for variadic DBG_INSTR_REFs in LiveDebugValues

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looks good with some tiny comments -- as ever it'll want tests, which should be folded into this patch, could you add those please.

This is also reassuringly small, which is encouraging and suggest it composes well with the other variadic work.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1595
   // (This information could be stored in TransferTracker to make it faster).
-  Optional<LocIdx> FoundLoc;
+  DenseMap<ValueIDNum, LocIdx> FoundLocs;
+  // Initialized the preferred-location map with illegal locations, to be
----------------
I believe DenseMap does an initial allocation, and the common case for DBG_INSTR_REF is going to be single value -- could I recommend using a SmallDenseMap instead? It'll expand to be larger when needed.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1626
   SmallVector<ResolvedDbgOp> NewLocs;
-  if (FoundLoc)
-    NewLocs.push_back(*FoundLoc);
+  for (DbgOp DbgOp : DbgOps) {
+    if (DbgOp.IsConst) {
----------------
Aliasing the type name (DbgOp DbgOp) is going to confuse someone at some point, just "Op" perhaps?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:1190
 
+  Optional<ValueIDNum> getValueForInstrRef(unsigned InstNo, unsigned OpNo,
+                                           MachineInstr &MI,
----------------
This will want a docu-comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133927