llvm dev - Apr 2020 - Question WRT llvm.dbg.value

> On Mar 30, 2020, at 11:57 PM, Sourabh Singh Tomar <sourav0311 at
gmail.com> wrote:
> 
>> > My understanding is that this isn't correct: dbg.declare
specifies the
>> memory address of a variable for the whole lifetime of the function,
>> whereas dbg.value (and dbg.addr) specify the value/address until the
>> next debug intrinsic. Mixing these two kinds of intrinsics creates
>> ambiguity over where the variable location is at different positions
>> in the code.
> 
>           > Correct, you should not be mixing dbg.declare and other
instrinsics for the same variable
> 
> How about patching up llvm for the same, currently the IR showed above is
valid and can be processed by llvm for target code generation.
> Should we move ahead invalidate this behavior as in "declare and value
intrinsic can't be specified for same local variable". ?
> 
> So that no FE should generate this sort of thing in first place. clang
doesn't do that so this change should not affect clang.
Adding this to the Verifier sounds like a good idea to me. It may be possible
that this uncovers existing bugs in the current flow, but that would be a good
thing.

-- adrian

>  Do you mean documenting the desired frontend behavior, or adding some
verifier in
LLVM? A warning for the latter is that SROA may currently emit IR that
contains a
mix of declares and values for different fragments of an aggregate
variable, so I
assume that is something that would need to be fixed then.

I had a quick look on that PR(thanks for sharing). A verifier(if
implemented) at LLVM level would invalidate this too. Would that be good ?
That brings up one other question, After SROA(like in present case) there
can be mix of *dbg.decalre* and *dbg.value*  of the same variable left out.
Snippet from PR
[.]

call void @llvm.dbg.declare(metadata i64* %arr.sroa.0, metadata !15,
metadata  DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !23

store i64 0, i64* %arr.sroa.0, align 16, !dbg !23
 call void @llvm.dbg.value(metadata i64 0, metadata !15, metadata
!DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !23

[.]

Does this presence of *dbg.value* suggest that previous location/value
described by *dbg.declare* is invalidated from this point. ? Not I
suppose since Docs mentions *it's valid for entire lifetime*. Then why
mix the at first place ?
> Adding this to the Verifier sounds like a good idea to me. It may be
possible that this uncovers existing bugs in the current flow, but that would be
a good thing.
As David mentioned, **documenting the desired behavior for frontend**
sounds good to me. this is based on premises that *clang* does not
emit any *dbg.value* at -O0(No opt) level, which is good(since most
assignments and variables are stack allocated, hence no need for
*dbg.value*).
There are some FE (for instance flang), living out-of-tree they should
also follow these semantics correctly.

--Sourabh


On Tue, Mar 31, 2020 at 8:33 PM Adrian Prantl <aprantl at apple.com>
wrote:
>
>
> > On Mar 30, 2020, at 11:57 PM, Sourabh Singh Tomar <sourav0311 at
gmail.com>
> wrote:
> >
> >> > My understanding is that this isn't correct: dbg.declare
specifies the
> >> memory address of a variable for the whole lifetime of the
function,
> >> whereas dbg.value (and dbg.addr) specify the value/address until
the
> >> next debug intrinsic. Mixing these two kinds of intrinsics creates
> >> ambiguity over where the variable location is at different
positions
> >> in the code.
> >
> >           > Correct, you should not be mixing dbg.declare and other
> instrinsics for the same variable
> >
> > How about patching up llvm for the same, currently the IR showed above
> is valid and can be processed by llvm for target code generation.
> > Should we move ahead invalidate this behavior as in "declare and
value
> intrinsic can't be specified for same local variable". ?
> >
> > So that no FE should generate this sort of thing in first place. clang
> doesn't do that so this change should not affect clang.
>
> Adding this to the Verifier sounds like a good idea to me. It may be
> possible that this uncovers existing bugs in the current flow, but that
> would be a good thing.
>
> -- adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200401/8d5d32dd/attachment.html>

> On Apr 1, 2020, at 2:56 AM, Sourabh Singh Tomar <sourav0311 at
gmail.com> wrote:
> 
> >  Do you mean documenting the desired frontend behavior, or adding some
verifier in
> LLVM? A warning for the latter is that SROA may currently emit IR that
contains a
> mix of declares and values for different fragments of an aggregate
variable, so I
> assume that is something that would need to be fixed then.
> 
> I had a quick look on that PR(thanks for sharing). A verifier(if
implemented) at LLVM level would invalidate this too. Would that be good ?
> That brings up one other question, After SROA(like in present case) there
can be mix of *dbg.decalre* and *dbg.value*  of the same variable left out.
> Snippet from PR
> [.]
> call void @llvm.dbg.declare(metadata i64* %arr.sroa.0, metadata !15,
metadata  DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !23
> store i64 0, i64* %arr.sroa.0, align 16, !dbg !23
>  call void @llvm.dbg.value(metadata i64 0, metadata !15, metadata
!DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !23
> [.]
> Does this presence of *dbg.value* suggest that previous location/value
described by *dbg.declare* is invalidated from this point. ? Not I suppose since
Docs mentions *it's valid for entire lifetime*. Then why mix the at first
place ?
The documentation describes our intention and reality often lags behind. I think
that we should fix the code to convert the dbg.declare into a dbg.value (or
dbg.addr?) that points into the alloca as well. I'm guessing that it will
not be easy to do this (we can call Local.cpp's LowerDbgDeclare
functionality beforehand, but that may actually make SROA's job harder
because it needs to find and update more values that may be difficult to
associate in reverse...), but having the verifier should help in the process.

-- adrian
> > Adding this to the Verifier sounds like a good idea to me. It may be
possible that this uncovers existing bugs in the current flow, but that would be
a good thing.
> As David mentioned, **documenting the desired behavior for frontend**
sounds good to me. this is based on premises that *clang* does not emit any
*dbg.value* at -O0(No opt) level, which is good(since most assignments and
variables are stack allocated, hence no need for *dbg.value*).
> There are some FE (for instance flang), living out-of-tree they should also
follow these semantics correctly.
> --Sourabh
> 
> On Tue, Mar 31, 2020 at 8:33 PM Adrian Prantl <aprantl at apple.com
<mailto:aprantl at apple.com>> wrote:
> 
> 
> > On Mar 30, 2020, at 11:57 PM, Sourabh Singh Tomar <sourav0311 at
gmail.com <mailto:sourav0311 at gmail.com>> wrote:
> > 
> >> > My understanding is that this isn't correct: dbg.declare
specifies the
> >> memory address of a variable for the whole lifetime of the
function,
> >> whereas dbg.value (and dbg.addr) specify the value/address until
the
> >> next debug intrinsic. Mixing these two kinds of intrinsics creates
> >> ambiguity over where the variable location is at different
positions
> >> in the code.
> > 
> >           > Correct, you should not be mixing dbg.declare and other
instrinsics for the same variable
> > 
> > How about patching up llvm for the same, currently the IR showed above
is valid and can be processed by llvm for target code generation.
> > Should we move ahead invalidate this behavior as in "declare and
value intrinsic can't be specified for same local variable". ?
> > 
> > So that no FE should generate this sort of thing in first place. clang
doesn't do that so this change should not affect clang.
> 
> Adding this to the Verifier sounds like a good idea to me. It may be
possible that this uncovers existing bugs in the current flow, but that would be
a good thing.
> 
> -- adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200401/b8f08276/attachment.html>