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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

Looks good, but with what feels like low test coverage: remind me, the plumbing required to track variadic locations in LiveDebugValues already landed, and we have tests to cover those new code paths, yes?



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:595-596
+        return false;
+      SmallVector<uint64_t> NonVariadicOps(make_range(
+        DIExpr->expr_op_begin().getNext().getBase(), DIExpr->elements_end()));
+      DIExpr = DIExpression::get(DIExpr->getContext(), NonVariadicOps);
----------------
Definitely wants a comment as to the intention behind this, I'm struggling to work it out.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1604
+  // filled in later.
+  for (DbgOp Op : DbgOps) {
+    if (!Op.IsConst)
----------------
Recommend `const DbgOp &` to avoid unnecessary copies cropping up


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/variadic-instr-ref.mir:1-3
+# RUN: llc %s -o - -experimental-debug-variable-locations=true \
+# RUN:     -run-pass=livedebugvalues  | \
+# RUN: FileCheck %s --implicit-check-not=DBG_VALUE
----------------
Super-nit -- I don't like the file name, ideally there would be some kind of indicator that it's touching LiveDebugValues in some way.


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/variadic-instr-ref.mir:50
+#
+## TODO: The debug value range should be terminated when the value in $ebx is clobbered.
+#
----------------
Sounds like a DbgEntityHistoryCalculator matter rather than one for LiveDebugValues, right?


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/variadic-instr-ref.mir:154
+---
+name:            _Z3fooii
+alignment:       16
----------------
Ideally we'd remove these redundant keys (all the false / zero ones) below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133927