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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
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:
> 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?
The static version is needed to share code between the const and non-const versions of this function; it doesn't need to be called directly outside of those two functions.. If this function wasn't static, then it would have to either a) be const and thus unable to return non-const MachineOperands, or b) be non-const and thus not be callable from a `const MachineInstr`. The static version on the other hand can be called with either a `const MachineInstr` to return `const MachineOperand*`, or called with a `MachineInstr` to return `MachineOperand*`.


================
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);
+
----------------
scott.linder wrote:
> 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.
> Why not just assert that `MCID.Opcode == TargetOpcode::DBG_VALUE_LIST` instead? This seems like surprising behavior to me.

Right now this function is called for both `DBG_VALUE` and `DBG_VALUE_LIST`; in places like LiveDebugValues where we remove delete debug value instructions, store them in some other format, modify them, and then re-emit them, this function will be called with `MCID = DebugValue.WasList() ? TargetOpcode::DBG_VALUE_LIST : TargetOpcode::DBG_VALUE;`. The MCID argument could be removed, with this function split up into two copies for single and list debug values; for what it's worth though, we'd still be making the same asserts as we do here, just done immediately prior calling this function in several other places.

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

Given that this only produced DBG_VALUEs in the first place I'm not sure why it was ever needed as an argument; the best time to remove it might be when we go back to only producing one type of instruction, i.e. when we remove DBG_VALUE further down the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82363