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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

I'm still working through the whole patch series (thank you for the careful breakdown of the changes and their dependencies, it has been a big help!) but I'm going to be out for a bit around the holidays and wanted to post at least some of the feedback I do have.

These are mostly just nits, I haven't really considered the design any more fundamentally. I'll also post some of the feedback I have on other patches in the series.



================
Comment at: llvm/docs/SourceLevelDebugging.rst:606
+And has the following operands:
+ * Operand 1 is the Variable field of the original debug intrinsic.
+ * Operand 2 is the Expression field of the original debug intrinsic.
----------------
Not that there is a strong convention, but could you make these consistent with the "The Nth operand..." phrasing above?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:506
+  bool hasDebugOperandForReg(Register Reg) const {
+    return count_if(debug_operands(), [Reg](const MachineOperand &Op) {
+      return Op.isReg() && Op.getReg() == Reg;
----------------



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:514
+  template <typename Operand, typename Instruction>
+  static SmallVector<Operand *, 2> getDebugOperandsForReg(Instruction *MI,
+                                                          Register Reg) {
----------------
Since https://reviews.llvm.org/D92522


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:514
+  template <typename Operand, typename Instruction>
+  static SmallVector<Operand *, 2> getDebugOperandsForReg(Instruction *MI,
+                                                          Register Reg) {
----------------
scott.linder wrote:
> Since https://reviews.llvm.org/D92522
What's the advantage of this being a `static` method? It seems like below you wrap it with member versions, but I don't see when this version would be necessary?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:516
+                                                          Register Reg) {
+    assert(MI->isDebugValue() &&
+           "Tried to get debug operands for non-debug_value");
----------------
I'd drop this assert, the call to `debug_operands` asserts the same thing.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:518-522
+    SmallVector<Operand *, 2> Ops;
+    for (Operand &Op : MI->debug_operands()) {
+      if (Op.isReg() && Op.getReg() == Reg)
+        Ops.push_back(&Op);
+    }
----------------



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:525
+  }
+  SmallVector<const MachineOperand *, 2>
+  getDebugOperandsForReg(Register Reg) const {
----------------



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:530
+  }
+  SmallVector<MachineOperand *, 2> getDebugOperandsForReg(Register Reg) {
+    return MachineInstr::getDebugOperandsForReg<MachineOperand, MachineInstr>(
----------------



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:857-859
+  assert((isDebugValue() || isDebugRef()) && "not a DBG_VALUE*");
+  unsigned VariableOp = isDebugValueList() ? 0 : 2;
+  return cast<DILocalVariable>(getOperand(VariableOp).getMetadata());
----------------



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:869-871
+  assert((isDebugValue() || isDebugRef()) && "not a DBG_VALUE*");
+  unsigned ExpressionOp = isDebugValueList() ? 1 : 3;
+  return cast<DIExpression>(getOperand(ExpressionOp).getMetadata());
----------------



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2153-2154
+         "Expected inlined-at fields to agree");
+  if (MCID.Opcode == TargetOpcode::DBG_VALUE)
+    return BuildMI(MF, DL, MCID, IsIndirect, MOs[0], Variable, Expr);
+
----------------
Why not just assert that `MCID.Opcode == TargetOpcode::DBG_VALUE_LIST` instead? This seems like surprising behavior to me.

Alternatively a more generic interface that doesn't require the `MCID` at all and decides which opcode to use would make sense, although I would vote to always choose the `_LIST` variant in that case anyway.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2233-2235
+  if (Orig.isNonListDebugValue()) {
+    NewMI.addFrameIndex(FrameIndex).addImm(0U);
+  }
----------------



================
Comment at: llvm/lib/Target/NVPTX/NVPTXPrologEpilogPass.cpp:71
+              MI.isDebugOperand(&Op) &&
+              "Frame indices can only appear as a debug operand in a DBG_VALUE"
+              " machine instruction");
----------------



================
Comment at: llvm/test/CodeGen/MIR/Generic/dbg-value-list-spill.mir:17
+#     if (c) {
+#       int e = d;
+#       bar();
----------------
I'm curious why a reference to `c` ends up in the `DBG_VALUE*` for `e`? Or maybe put another way, why does the last `CHECK` line refer to `$noreg`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363