[PATCH] D87357: [SelectionDAG][DebugInfo] Use entry-values to recover parameters values

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:718
+    // a DW_OP_entry_value.
+    if (cast<DILocalVariable>(Var)->isParameter())
+      EntryValue =
----------------
djtodoro wrote:
> StephenTozer wrote:
> > djtodoro wrote:
> > > djtodoro wrote:
> > > > StephenTozer wrote:
> > > > > djtodoro wrote:
> > > > > > Orlando wrote:
> > > > > > > I think this we should only be doing this for immutable parameters (and mutable parameters which are never assigned to), right?
> > > > > > > 
> > > > > > > I.e. the entry_value of a parameter register is only a valid location for a parameter variable if that variable is never assigned another value.
> > > > > > We are expressing the modification in terms of its entry value here.
> > > > > As far as I understand, we want to recover variables by describing them in relation to the initial values of the parameters. In that case, it only makes sense to consider variables whose values can be expressed in relation to that; this may include variables that aren't parameters themselves, and may exclude dbg.values for parameters. For example:
> > > > > 
> > > > > ```
> > > > > void foo(int param) {
> > > > >   int x = param + 2;
> > > > >   param = SomeInt();
> > > > >   ...
> > > > > }
> > > > > ```
> > > > > 
> > > > > In this example it should be possible to describe `x` in terms of `param`'s initial value, while `param` itself could not be described by an entry value after its assignment within the function. In that case, I believe it would make more sense to look for dbg.values that use a parameter Value, or can be expressed in terms of one, rather than using `DILocalVariable::isParameter`. Does that seem correct, or have I misunderstood the work or the purpose of this block?
> > > > Hi @StephenTozer.
> > > > Yes, but is a more complex use case, so we do not cover it (yet). There is a `TODO` marker within the `findEntryValue()` for that.
> > > > The debug entry values feature is complex, relying on various things, so we've initially implemented the support for simple registers locations as entry values of *unmodified* parameters *only*, during the LiveDebugValues phase. Currently, we are extending the support onto IR level, for some (simple) cases, but there is space for improvements for sure, since the potential goes beyond the current usage.
> > > > I think this we should only be doing this for immutable parameters (and mutable parameters which are never assigned to), right?
> > > > 
> > > > I.e. the entry_value of a parameter register is only a valid location for a parameter variable if that variable is never assigned another value.
> > > > 
> > > 
> > > Hi @Orlando.
> > > I might have needed to add some more context here. You are right, and all of that has been already checked within the main debug-entry-values place of the production, within LiveDebugValues. We perform analysis there, and at the moment, use entry values for unmodified params (since it is the basic case). There is space for improvements, by using the entry values for describing/expressing the modification in terms of its entry value, but we do not support it (yet), there.
> > > 
> > > On the IR level, at the moment, we are trying to extend this support for unused arguments, since it seems to be safe/simple use case. First place for that is the `DeadArgElimination` pass, and we are working on sorting out all the pieces for that purpose (but it takes some additional magic in order to get it working for both callee and caller sides). The next spot this could be used is here, where we also have unused parameters, that are not being copied into any register, so the `SelectionDAG` marks them as invalid/unable to track it location anymore. Please comment on this, since I might have missed something.
> > Not handling the `x` case is fine then, since it will be much easier to get the basic change completed and then extend it to cover more cases later. I'm still not sure how this new extension avoids the second case however. Because we only use the variable to determine what entry value (if any) to produce, this code would still produce entry value DBG_VALUEs for a parameter even after an assignment to that parameter. Is there anything I've missed that would prevent us producing invalid entry values right here, such as for `param`'s post-assignment value in the example code above?
> Probably we need some additional/stronger statements here. Can you please share the test case, so I can reproduce it?
//statements -- in the code/extension implementation terms//


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

https://reviews.llvm.org/D87357