[PATCH] D69886: [DebugInfo] Support for DW_OP_implicit_pointer (CodeGen phase)

Thu Nov 7 21:12:12 PST 2019

alok added a comment.

In D69886#1736182 <https://reviews.llvm.org/D69886#1736182>, @vsk wrote:

> Hey @alok, thanks for working on this.

Thanks Vedant for your time in reviewing this.

> Currently, the operands of dbg.value constitute a `{value, variable, expression}` tuple. This patch seems to change that to `{value | variable-to-be-deref'd, variable, expression | expression-with-implicit-pointer}`. What alternatives have you considered? While reading your tests, I found it hard to reason about how implicit pointer values are referenced & defined. This made me concerned about the amount of hidden state that the current approach would introduce into IR. Further,

While implementing this, my priority was to avoid changing the interface of dbg.value which is

  declare void @llvm.dbg.value(metadata, metadata, metadata)

Which permits to have another variable metadata at first argument. Since DW_OP_implicit_pointer suggests that check the value of (optimized out) variable at another variable. Last argument is expression, which can be used to identify that first argument is redirection variable (using isImplicitPointer()). For example

  call void @llvm.dbg.value(metadata !16, metadata !25, metadata !DIExpression(DW_OP_implicit_pointer, 0)), !dbg !31
  !16 = !DILocalVariable(name: "arr", scope: !12, file: !3, line: 24, type: !17)
  !25 = !DILocalVariable(name: "ptr", arg: 1, scope: !26, file: !3, line: 15, type: !29)

which suggests, "ptr" is optimized out but its value is same as "arr", check "arr"

> I'm not really convinced this does the right thing in the presence of inlining (at least, not yet).

Currently I have two test cases which test the implementation for inlining. Handling of inilining is done in future patch (IR transformation phase). I can explain it later once I submit that patch. Please do share any scenario which you think should be considered.

> And I'm keen to avoid extending SelectionDAG to represent implicit pointer debug values, as that presumably creates work on the GlobalISel side as well.
> 
> One possible alternative (haven't fully thought this through yet, but here goes!) might be to have `dbg.value(value, variable, expr)` return a token value, instead of void. That token could then be referenced later, like: `dbg.value(%implicit_pointer_token, <variable>, DIExpression(DW_OP_implicit_pointer(<offset>)))`. The main advantage of this is that it makes the link between a description of the implicit pointer and its dereference explicit. I think this will be a lot easier to reason about. But there are more concrete benefits. E.g., if a pass were to move the DW_OP_implicit_pointer dbg.value //before// the def of its implicit pointer token, we'd detect the use-before-def instead of silently emitting the wrong value.

Alternatively, what approach i have taken is to identify the kind of first argument based on third argument, and that solves the same purpose (using function isImplicitPointer()). Code movement part is handled in IR transformation phase (future patch). Please let me know if you have any scenario that should be considered. please let me know in case i did not understand anything.

> p.s: Could you number your patches (e.g. [1/3], [2/3], etc) to make them easier to find? Could you also make the tests for each patch independent so that they can be landed incrementally?

Sure I shall keep this in mind.

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

https://reviews.llvm.org/D69886