[PATCH] D70644: [DebugInfo] Support for DW_OP_implicit_pointer (llvm.dbg_derefval)

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 12 inline comments as done.
alok added inline comments.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:261
+This intrinsic is identical to `llvm.dbg.value`, except that it is used to
+describe dereferenced value of pointer variable. The first argument is metadata.
+The second argument is a `local variable <LangRef.html#dilocalvariable>`_
----------------
jmorse wrote:
> I'd add some motivation to the first sentence, such as: "it it used to describe the dereferenced value of a pointer variable, when the pointer itself is no longer available in the program".
Thanks for comment. It will be incorporated in next version.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:266-267
+interpreted based on the third argument. If the third argument represents
+DW_OP_LLVM_implicit_pointer, the first argument is
+ `local variable <LangRef.html#dilocalvariable>`_ , otherwise it is the value
+(wrapped as metadata).
----------------
jmorse wrote:
> IMO "is" should read "must", i.e. "the first argument must be a local variable", to indicate this is required for correctness.
Thanks for your comment. It will be included in next version.


================
Comment at: llvm/docs/SourceLevelDebugging.rst:270
+
+An `llvm.dbg.derefval` intrinsic is usefull when location which the variable
+points to is optimized out, but the dereferenced value is known.
----------------
StephenTozer wrote:
> Correct usefull -> useful
Thanks for your comment. It will be incorporated in next version.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:369
 
+  /// Build and insert a DBG_VALUE instructions specifying that dereferenced
+  /// value of \p Variable is given by \p V (suitably modified by \p Expr).
----------------
jmorse wrote:
> "that the dereferenced"?
Thanks for your comment. It will be incorporated in next version.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstrBuilder.h:227-238
+  const MachineInstrBuilder &addMetadata(const MDNode *MD,
+                                         bool IsImplicitPointer = false) const {
     MI->addOperand(*MF, MachineOperand::CreateMetadata(MD));
-    assert((MI->isDebugValue() ? static_cast<bool>(MI->getDebugVariable())
-                               : true) &&
+    assert(((MI->isDebugValue() && !IsImplicitPointer)
+                ? static_cast<bool>(MI->getDebugVariable())
+                : true) &&
            "first MDNode argument of a DBG_VALUE not a variable");
----------------
jmorse wrote:
> Perhaps it's worth adding a new "add" method, i.e. "addImplicitPtrMetadata"? That avoids the extra argument, and in the call sites below it will be a little clearer what's happening, as a "true" argument isn't especially helpful.
> 
> Plus, then you can assert that only DILocalVariables (or whatever) are added as the implicit pointer metadata.
Thanks for your comment. It will be incorporated in next version.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:717-718
   if (isDbgValueDescribedByReg(MI) || MI.getOperand(0).isImm() ||
-      MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm()) {
+      MI.getOperand(0).isFPImm() || MI.getOperand(0).isCImm() ||
+      MI.getOperand(0).isMetadata()) {
     // Use normal VarLoc constructor for registers and immediates.
----------------
jmorse wrote:
> The main VarLoc constructor called on line 720 is going to not recognise the metadata operand and leave some fields uninitialized (it should probably actually assert in this case). I reckon you'll need to add a new location "Kind", and have it emitted in the BuildDbgValue method.
> 
> The LiveDebugValues algorithm should (TM) handle and propagate this kind of location fine.
Thanks for pointing this out. This will be incorporated in next version.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/FastISel.cpp:1447
   }
+  case Intrinsic::dbg_derefval: {
+    // This form of DBG_VALUE is target-independent.
----------------
jmorse wrote:
> Perhaps easier here instead to have dbg_derefval and dbg_value share code, with an additional assertion that MetadataAsValue operands can only come from dbg_value's?
Thanks for your comment. It will be incorporated in next version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70644