Page MenuHomePhabricator

[DebugInfo] Support for DW_OP_implicit_pointer (llvm.dbg_derefval)
Needs ReviewPublic

Authored by alok on Nov 24 2019, 9:53 AM.

Details

Summary
  This patch (3/N) stems from D69787 as suggested by
  This is suggested by @jmorse.

  New intrinsic llvm.dbg.derefval is introduced.

Summary:
  Since dbg.value currently represents (VAR=VALUE), the new intrinsic dbg.derefval will represent de-referenced value (*VAR = VAL)
    - Below represents ptr=null
      call void @llvm.dbg.value(metadata i32* null, metadata !21, metadata !DIExpression())
    - And below represents *ptr=var
      call void @llvm.dbg.derefval(metadata !16, metadata !21, metadata !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_arg0, 0))
    - And below represents *ptr=arr[1]
      call void @llvm.dbg.derefval(metadata !16, metadata !21, metadata !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_arg0, 4))
    - We should be able to represent the case when a variable points to temporary (initialized by constant) and temporary is optimized out.
      tmp=[CONST]; ptr=&tmp;
      call void @llvm.dbg.derefval(metadata [CONST], metadata !21, metadata !DIExpression(DW_OP_LLVM_arg0))

Diff Detail

Event Timeline

alok created this revision.Nov 24 2019, 9:53 AM
asbirlea removed a subscriber: asbirlea.Nov 25 2019, 7:37 AM
StephenTozer added inline comments.
llvm/docs/SourceLevelDebugging.rst
270

Correct usefull -> useful

Thanks for the patch, comments inline. A couple of things in general:

As mentioned on the mailing list thread, it bothers me a little that DbgVariableIntrinsic::getVariableLocation is going to try and return something that isn't a "Real" location. In fact, wouldn't the assertion at the end of getVariableLocation fire if the operand was a non-null non-ValueAsMetadata piece of metadata? It might make sense to add another level to the class hierarchy, to distinguish variable intrinsics that definitely always have a location, and the "more complicated" situation dbg.derefval has.

(I tried to think up what I'd like a class hierarchy to look like for all the debugging intrinsics, but they all have substantially different use cases).

I haven't reviewed the changes to SelectionDAG, I think those are probably better placed in D69886 with the other CodeGen changes, would you be able to move the changes into that review? (Both modify SelectionDAGBuilder.cpp). That way all the SelectionDAG modifications are in one place, and have the tests with them.

llvm/docs/SourceLevelDebugging.rst
261

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

266–267

IMO "is" should read "must", i.e. "the first argument must be a local variable", to indicate this is required for correctness.

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
369

"that the dereferenced"?

llvm/include/llvm/CodeGen/MachineInstrBuilder.h
227–236

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.

llvm/lib/CodeGen/LiveDebugValues.cpp
818–820

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.

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1458

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?

alok marked 12 inline comments as done.Dec 17 2019, 2:32 AM
alok added inline comments.
llvm/docs/SourceLevelDebugging.rst
261

Thanks for comment. It will be incorporated in next version.

266–267

Thanks for your comment. It will be included in next version.

270

Thanks for your comment. It will be incorporated in next version.

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
369

Thanks for your comment. It will be incorporated in next version.

llvm/include/llvm/CodeGen/MachineInstrBuilder.h
227–236

Thanks for your comment. It will be incorporated in next version.

llvm/lib/CodeGen/LiveDebugValues.cpp
818–820

Thanks for pointing this out. This will be incorporated in next version.

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1458

Thanks for your comment. It will be incorporated in next version.

alok marked 5 inline comments as done.Dec 17 2019, 3:16 AM
alok updated this revision to Diff 234356.Dec 17 2019, 11:58 AM

This version is updated to rebase and to incorporate comments from @jmorse and @StephenTozer.

alok updated this revision to Diff 235772.Wed, Jan 1, 10:14 AM

Update to re-base and minor correction.

alok updated this revision to Diff 235919.Thu, Jan 2, 12:00 PM

Rebased and updated.