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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
