[PATCH] D82363: [DebugInfo] Add new instruction and expression operator for variadic debug values

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This tentatively looks LGTM to me, modulo some nits and the comment on dbg-value-list-spill.mir.



================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:432
+      MachineOperand &MO = DBG->getDebugOperand(0);
+      if (MO.isReg() && MO.getReg() == 0) {
+        updateDbgValueForSpill(*DBG, FI, 0);
----------------
Is this rewriting
  DBG_VALUE $noreg
to
  DBG_VALUE %stack.0
?

Obviously this patch just preserves old behaviour, which is fine, but I get the feeling that the old behaviour is wrong.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1291-1294
+  for (auto Op : Expr->expr_ops()) {
+    Op.appendToVector(NewOps);
+    if (Op.getOp() == dwarf::DW_OP_LLVM_arg && Op.getArg(0) == ArgNo)
+      NewOps.insert(NewOps.end(), Ops.begin(), Ops.end());
----------------
Mega-nit: Could I suggest "ExprOp" as the iterating variable -- having `Op` and `Ops` in the same method makes me think that `Op` is a member of `Ops`, but that's not the case.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp:84-87
+            SmallVector<uint64_t, 8> Ops;
+            DIExpression::appendOffset(Ops, Offset);
+            unsigned OpIdx = MI.getDebugOperandIndex(&MI.getOperand(i));
+            DIExpr = DIExpression::appendOpsToArg(DIExpr, Ops, OpIdx);
----------------
Should this be using the new "getOffsetOpcodes" hook?


================
Comment at: llvm/lib/Target/SystemZ/SystemZRegisterInfo.cpp:280-286
+      unsigned OpIdx = MI->getDebugOperandIndex(&MI->getOperand(FIOperandNum));
+      SmallVector<uint64_t, 3> Ops;
+      DIExpression::appendOffset(
+          Ops, TFI->getFrameIndexReference(MF, FrameIndex, BasePtr).getFixed());
+      MI->getDebugExpressionOp().setMetadata(
+          DIExpression::appendOpsToArg(MI->getDebugExpression(), Ops, OpIdx));
+    }
----------------
Should this be using the new "getOffsetOpcodes" hook?


================
Comment at: llvm/lib/Target/X86/X86OptimizeLEAs.cpp:587
+          DIExpression::prepend(Expr, DIExpression::StackValue, AddrDispShift);
+    else {
+      // Update the Expression, appending an offset of `AddrDispShift` to the
----------------
The if that this "else" applies to is ambiguous -- curly braces everywhere please


================
Comment at: llvm/test/CodeGen/MIR/Generic/dbg-value-list-spill.mir:1
+# RUN: llc -run-pass regallocfast -o - %s | FileCheck %s
+# Test that when virtual registers referenced by a DBG_VALUE_LIST are spilled,
----------------
Seeing how the input file doesn't have any virtual registers, surely running regalloc is going to do nothing?

Plus: the code below is X86, it probably deserves to be in the MIR/x86 directory and have a triple on the command line (I've been bitten by missing triples several times)


================
Comment at: llvm/test/CodeGen/MIR/Generic/dbg-value-list.mir:3
+# Simple round-trip test for DBG_VALUE_LIST.
+# CHECK: DBG_VALUE_LIST !14, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value), $edi, $esi, debug-location !15
+--- |
----------------
Needs to use metadata node captures rather than hard-coded numbers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363