[PATCH] D133929: [DebugInfo] Produce variadic DBG_INSTR_REFs from ISel

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

This LGTM with a few inline questions. The addition of `DW_OP_arg, 0` to all INSTR_REF expressions //does// add some visual noise but I personally prefer this. Given how convoluted debug instruction handling can get, I like the idea of continuing in this direction and minimising the number of special cases.



================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:1194
     MI.setDesc(RefII);
-    MI.getDebugOperand(0).setReg(0);
+    MI.setDebugValueUndef();
   };
----------------
👍 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:699
 
+  // Emit variadic dbg_value nodes as DBG_VALUE_LIST.
+  if (SD->isVariadic())
----------------
Possibly worth expanding the comment to mention that these will have been emitted as `INSTR_REF` by the code above if instr ref is enabled.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:849-850
+      assert(DbgOperand.getKind() == SDDbgOperand::CONST);
+      // TODO: Probably extract this out into a function shared with
+      // AddDbgValueLocationOps.
+      const Value *V = DbgOperand.getConst();
----------------
Looks like this one slipped through the cracks - can this TODO be done? IMO eliminating duplicated code is especially important around these fiddly areas.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:921
+  }
+  return &*MIB;
+}
----------------
Is there a benefit to preserving the number of operands (rather than just using `DBG_VALUE $noreg`) - does it strip assertions about DW_OP_arg usage or something if we don't?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133929