[PATCH] D129372: [DebugInfo][NFC?] Add new MachineOperand type and change DBG_INSTR_REF syntax

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:599-602
+  unsigned getOpIdx() const {
+    assert(isDbgInstrRef() && "Wrong MachineOperand accessor");
+    return Contents.InstrRef.OpIdx;
+  }
----------------
jmorse wrote:
> I fear that this method name is going to be misinterpreted by the reader as returning what the operands index is in the owning instruction. Obviously this is a bother to think about, but worth considering. Possibly prefixing Dbg before 'Instr' and 'Op' will be clearer?
> 
> Also, for consistency, IMO it should be "Index" instead of Idx, the following method says "Index".
> 
> Similarly with the setters.
That's a reasonable point - I'd probably object to using `Dbg` as the prefix though, as `DbgOp` is used elsewhere to refer to the operands in debug value (and as of this patch, in a DBG_INSTR_REF!). It's a long function name, but I think using `InstrRef` as the prefix is pretty unambiguous.


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1192-1194
+    const MCInstrDesc &RefII = TII->get(TargetOpcode::DBG_VALUE_LIST);
     MI.setDesc(RefII);
+    MI.getDebugOperand(0).setReg(0);
----------------
jmorse wrote:
> AFAIUI this isn't necessary at this stage (and could lead to test breakage), is that right? If so, better to fold into a "making DBG_VALUE_LIST everywhere" (or whatever it gets called in the end) patch.
The reason for making this change is just that it keeps the undef-ing simple, as converting it to a DBG_VALUE would now require us to move and add operands, and after the next patch in the stack also modify the DIExpression. Since it's an undef value either way it doesn't make any difference in the final output, and AFAIK doesn't cause much/any breakage with existing tests. But if you think the tradeoff is worth it then I'm open to doing the DBG_VALUE conversion, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129372