[PATCH] D87233: [POC][DebugInfo] Use entry values within IR

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1596
 
+Value *llvm::findEntryValue(DIVariable *Var) {
+  // TODO: Find an entry value for local variables,
----------------
djtodoro wrote:
> dblaikie wrote:
> > Orlando wrote:
> > > djtodoro wrote:
> > > > Orlando wrote:
> > > > > Hi @djtodoro. IIUC this function assumes that the returned `dbg.value` - the first one we find - for parameter variable `Var` uses the corresponding argument value (as opposed to some other value that a mutable parameter could be assigned). If that is correct, why is that assumption safe?
> > > > //(Hmm.... I've forgotten to take another look into this, in spite of the fact that I've had an item for it within my TODO list...)//
> > > > 
> > > > Hi @Orlando.
> > > > I had a crazy premise in my mind, that every parameter should have a dbg_value on the beginning of the bb.entry. In some situations, the Value could be `undef` (due to some optimizations), but it will indicate just that the value has been optimized out (but it should be still there in the code). Is that correct? I've prepared this POC in rush, and have forgotten to check this once again. If the `users()` aren't sorted out, this should be reimplemented.
> > > > Every parameter should have a dbg_value on the beginning of the bb.entry
> > > 
> > > My understanding is that there should be _at least_ one `dbg.value` for each param at the top of the function. E.g. in the following source, a parameter `test` has been split into two argument values and there's a fragment expression for each.
> > > ```
> > > $ cat test.c
> > > struct S {
> > >   long long one;
> > >   long long two;
> > > };
> > > void foo(struct S test) {}
> > > 
> > > $ clang -O2 -g -S -emit-llvm -o - test.c
> > > ...
> > > define dso_local void @foo(i64 %test.coerce0, i64 %test.coerce1) local_unnamed_addr #0 !dbg !7 {
> > > entry:
> > >   call void @llvm.dbg.value(metadata i64 %test.coerce0, metadata !16, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !17
> > >   call void @llvm.dbg.value(metadata i64 %test.coerce1, metadata !16, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !17
> > >   ret void, !dbg !18
> > > }
> > > ...
> > > ```
> > > 
> > > > If the users() aren't sorted out, this should be reimplemented
> > > I am not sure if `users()` is sorted. I had a hunch that it isn't and I can't see any comments indicating that it is. @jmorse - or anyone else - do you know for sure?
> > > 
> > > Supposing that `users()` isn't sorted and we take the first assumption as truth (that every parameter should have a dbg.value at the top of the function, ignoring fragmented parameters), then I don't think it should be too hard to go forward from here. I think we could just order the `dbg.values` on `Instruction::comesBefore(const Instruction *)`.
> > > 
> > > Additionally, shouldn't we be filtering out dbg.values from inlined uses of `Var`?
> > > I am not sure if users() is sorted. I had a hunch that it isn't and I can't see any comments indicating that it is. @jmorse - or anyone else - do you know for sure?
> > 
> > I'm about 98% sure that use list order is not stable. (that's why there's this: https://llvm.org/doxygen/UseListOrder_8h_source.html )
> Thanks!
> 
> We can simply iterate throughout the `entry-bb` and find the first `dbg.value` for a given parameter.
> 
> >Additionally, shouldn't we be filtering out dbg.values from inlined uses of Var?
> Yes. We should avoid fragmented variables as well, since we do not support such entry values at the moment.
> 
This function (and the unit test) LGTM now, thanks.


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

https://reviews.llvm.org/D87233