llvm dev - Jun 2018 - [RFC] Removing debug locations from ConstantSDNodes

FWIW: Debug information on constants feels odd to me. They are just values not
something that is executed so conceptually I would not expect them to
"happen" at a specific time/place in the program. That said most
numbers are copied into registers or stored into memory and that is of course an
interesting action. So in the original example I would hope to see debug info on
whatever instructions are used to fill the array with values.

That said I'm not familiar with the inner workings of dwarf or other
debugger formats, so it may very well be reasonable to backfill the information
in a late pass to avoid having assembler instructions without debug info as some
people proposed.

- Matthias
> On Jun 20, 2018, at 11:09 AM, via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> DwarfDebug::beginInstruction sets the location to line 0 because otherwise
the location is implicitly the same location as the last instruction of the
physically preceding block.  That location is often completely unrelated to
what's at the top of the new block.  Line 0 isn't great, but at least
it's not a complete lie.
> --paulr
>   <>
> From: vsk at apple.com [mailto:vsk at apple.com] 
> Sent: Wednesday, June 20, 2018 1:48 PM
> To: Reid Kleckner
> Cc: Robinson, Paul; llvm-dev; Justin Bogner; David Li; David Blaikie
> Subject: Re: [RFC] Removing debug locations from ConstantSDNodes
>  
> On Jun 19, 2018, at 6:36 PM, Reid Kleckner <rnk at google.com
<mailto:rnk at google.com>> wrote:
>  
> On Tue, Jun 19, 2018 at 5:46 PM Vedant Kumar <vsk at apple.com
<mailto:vsk at apple.com>> wrote:
> Someone (Reid?) mentioned that we could try sinking constants to their
point of first use as an alternative, and (IIUC) create new nodes with distinct
DebugLocs for each use of a constant. I don't recall the details of that
alternative clearly. Based on my (likely incorrect) understanding of it,
dropping locations from constants outright might be simpler.
>  
> Our use case was in fastisel, so things are different. I don't think my
solution will help you.
>  
> In our case, users were complaining about code like this:
> volatile int do_something;
> void f() {
>   ++do_something;
>   foo(1, 2, 3);
>   ++do_something;
> }
>  
> We'd generate locations like:
>   .loc line 1
>   incl do_something(%rip)
>   movl $1, %ecx
>   movl $2, %edx
>   movl $3, %r8d
>   .loc line 2 # line 2 starts here, instead of 3 instructions earlier
>   callq foo
>   .loc line 3
>   incl do_something(%rip)
> 
> 
> Our users really wanted the line table entry for line 2 to include the
constant materialization code for some VS debugger feature.
>  
> Got it. (For others following along, there's more discussion about this
debugger feature in the commit description for r327581.)
> 
> 
> 
> I think if you remove locations from ConstantSDNodes, you might want to add
a late pass that propagates source locations backwards onto location-less
instructions. This would also avoid some special cases when a basic block starts
with an instruction that lacks location information. See
CodeViewDebug::beginInstruction and DwarfDebug::beginInstruction for what we do
today.
>  
> Interesting, I hadn't considered doing this.
>  
> What happens in the special case where a block starts with a location-less
instruction? Ah, I see that CodeViewDebug::beginInstruction does a forward scan
to find the first instruction with location. This can fail though: there might
not be such an instruction, in which case... I assume we either apply a line-0
location, or nothing at all, similar to Dwarf::beginInstruction?
>  
> It's a bit unclear to me what the benefits of a late pass that
backwards-propagated locations would be. I suppose we'd have fewer forward
scans in CodeViewDebug::beginInstruction, so is the benefit a compile-time win
(which would offset the cost of a late pass)? Would the final debug info quality
would be better in some way I'm just missing?
>  
> thanks!
> vedant
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180620/4f7e7be5/attachment.html>

I don't think this would affect SamplePGO because it is unlikely that
constants are the only instructions in the basic block. Other instructions
in the same BB should be sufficient to reserve the debug info.

Dehao

On Wed, Jun 20, 2018 at 4:32 PM Matthias Braun via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> FWIW: Debug information on constants feels odd to me. They are just values
> not something that is executed so conceptually I would not expect them to
> "happen" at a specific time/place in the program. That said most
numbers
> are copied into registers or stored into memory and that is of course an
> interesting action. So in the original example I would hope to see debug
> info on whatever instructions are used to fill the array with values.
>
> That said I'm not familiar with the inner workings of dwarf or other
> debugger formats, so it may very well be reasonable to backfill the
> information in a late pass to avoid having assembler instructions without
> debug info as some people proposed.
>
> - Matthias
>
> On Jun 20, 2018, at 11:09 AM, via llvm-dev <llvm-dev at
lists.llvm.org>
> wrote:
>
> DwarfDebug::beginInstruction sets the location to line 0 because otherwise
> the location is implicitly the same location as the last instruction of the
> physically preceding block.  That location is often completely unrelated to
> what's at the top of the new block.  Line 0 isn't great, but at
least it's
> not a complete lie.
> --paulr
>
> *From:* vsk at apple.com [mailto:vsk at apple.com <vsk at apple.com>]
> *Sent:* Wednesday, June 20, 2018 1:48 PM
> *To:* Reid Kleckner
> *Cc:* Robinson, Paul; llvm-dev; Justin Bogner; David Li; David Blaikie
> *Subject:* Re: [RFC] Removing debug locations from ConstantSDNodes
>
>
> On Jun 19, 2018, at 6:36 PM, Reid Kleckner <rnk at google.com> wrote:
>
>
> On Tue, Jun 19, 2018 at 5:46 PM Vedant Kumar <vsk at apple.com>
wrote:
>
> Someone (Reid?) mentioned that we could try sinking constants to their
> point of first use as an alternative, and (IIUC) create new nodes with
> distinct DebugLocs for each use of a constant. I don't recall the
details
> of that alternative clearly. Based on my (likely incorrect) understanding
> of it, dropping locations from constants outright might be simpler.
>
>
> Our use case was in fastisel, so things are different. I don't think my
> solution will help you.
>
> In our case, users were complaining about code like this:
> volatile int do_something;
> void f() {
>   ++do_something;
>   foo(1, 2, 3);
>   ++do_something;
> }
>
> We'd generate locations like:
>   .loc line 1
>   incl do_something(%rip)
>   movl $1, %ecx
>   movl $2, %edx
>   movl $3, %r8d
>   .loc line 2 # line 2 starts here, instead of 3 instructions earlier
>   callq foo
>   .loc line 3
>   incl do_something(%rip)
>
>
> Our users really wanted the line table entry for line 2 to include the
> constant materialization code for some VS debugger feature.
>
>
> Got it. (For others following along, there's more discussion about this
> debugger feature in the commit description for r327581.)
>
>
>
> I think if you remove locations from ConstantSDNodes, you might want to
> add a late pass that propagates source locations backwards onto
> location-less instructions. This would also avoid some special cases when a
> basic block starts with an instruction that lacks location information.
> See CodeViewDebug::beginInstruction and DwarfDebug::beginInstruction for
> what we do today.
>
> Interesting, I hadn't considered doing this.
>
> What happens in the special case where a block starts with a location-less
> instruction? Ah, I see that CodeViewDebug::beginInstruction does a forward
> scan to find the first instruction with location. This can fail though:
> there might not be such an instruction, in which case... I assume we either
> apply a line-0 location, or nothing at all, similar to
> Dwarf::beginInstruction?
>
> It's a bit unclear to me what the benefits of a late pass that
> backwards-propagated locations would be. I suppose we'd have fewer
forward
> scans in CodeViewDebug::beginInstruction, so is the benefit a compile-time
> win (which would offset the cost of a late pass)? Would the final debug
> info quality would be better in some way I'm just missing?
>
> thanks!
> vedant
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180620/22f0df13/attachment-0001.html>

mbraun at apple.com<mailto:mbraun at apple.com> wrote:

FWIW: Debug information on constants feels odd to me. They are just values not
something that is executed so conceptually I would not expect them to
"happen" at a specific time/place in the program. That said most
numbers are copied into registers or stored into memory and that is of course an
interesting action. So in the original example I would hope to see debug info on
whatever instructions are used to fill the array with values.

+1.  Constants are not inherently interesting, but loading one as an action
preparatory to computing a value or setting up a parameter in a call, the
instruction ought to be associated with the relevant source statement.
--paulr

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180621/24d9383a/attachment.html>

Isn't this a typcial situation when the is_stmt field in the DWARF line
table should be used?

If we set is_stmt=0 for the instruction loading the constant, then a debugger
can choose not to stop on that instruction when doing "step" on source
level. That way we can keep the original source location for a ConstantSDNode,
but also telling the debugger that this isn't a recommended breakpoint
location for that line. Using is_stmt=0 is ofcourse only interesting if the
constant load I hoisted so that it isn't adjacent to other is_stmt=1
instructions on the same line.

/Björn

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of via
llvm-dev
Sent: den 21 juni 2018 15:08
To: mbraun at apple.com
Cc: llvm-dev at lists.llvm.org; davidxl at google.com; jbogner at apple.com
Subject: Re: [llvm-dev] [RFC] Removing debug locations from ConstantSDNodes

mbraun at apple.com<mailto:mbraun at apple.com> wrote:

FWIW: Debug information on constants feels odd to me. They are just values not
something that is executed so conceptually I would not expect them to
"happen" at a specific time/place in the program. That said most
numbers are copied into registers or stored into memory and that is of course an
interesting action. So in the original example I would hope to see debug info on
whatever instructions are used to fill the array with values.

+1.  Constants are not inherently interesting, but loading one as an action
preparatory to computing a value or setting up a parameter in a call, the
instruction ought to be associated with the relevant source statement.
--paulr

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180621/2c2cbf24/attachment.html>

Yeah, I'm with you here. I don't think there's any particular debug
info
gain to having debug info on materializing a constant either.

Vedant: Ultimately I think I'm in favor of this plan.

Thanks!

-eric

On Wed, Jun 20, 2018 at 4:32 PM Matthias Braun via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> FWIW: Debug information on constants feels odd to me. They are just values
> not something that is executed so conceptually I would not expect them to
> "happen" at a specific time/place in the program. That said most
numbers
> are copied into registers or stored into memory and that is of course an
> interesting action. So in the original example I would hope to see debug
> info on whatever instructions are used to fill the array with values.
>
> That said I'm not familiar with the inner workings of dwarf or other
> debugger formats, so it may very well be reasonable to backfill the
> information in a late pass to avoid having assembler instructions without
> debug info as some people proposed.
>
> - Matthias
>
> On Jun 20, 2018, at 11:09 AM, via llvm-dev <llvm-dev at
lists.llvm.org>
> wrote:
>
> DwarfDebug::beginInstruction sets the location to line 0 because otherwise
> the location is implicitly the same location as the last instruction of the
> physically preceding block.  That location is often completely unrelated to
> what's at the top of the new block.  Line 0 isn't great, but at
least it's
> not a complete lie.
> --paulr
>
> *From:* vsk at apple.com [mailto:vsk at apple.com <vsk at apple.com>]
> *Sent:* Wednesday, June 20, 2018 1:48 PM
> *To:* Reid Kleckner
> *Cc:* Robinson, Paul; llvm-dev; Justin Bogner; David Li; David Blaikie
> *Subject:* Re: [RFC] Removing debug locations from ConstantSDNodes
>
>
> On Jun 19, 2018, at 6:36 PM, Reid Kleckner <rnk at google.com> wrote:
>
>
> On Tue, Jun 19, 2018 at 5:46 PM Vedant Kumar <vsk at apple.com>
wrote:
>
> Someone (Reid?) mentioned that we could try sinking constants to their
> point of first use as an alternative, and (IIUC) create new nodes with
> distinct DebugLocs for each use of a constant. I don't recall the
details
> of that alternative clearly. Based on my (likely incorrect) understanding
> of it, dropping locations from constants outright might be simpler.
>
>
> Our use case was in fastisel, so things are different. I don't think my
> solution will help you.
>
> In our case, users were complaining about code like this:
> volatile int do_something;
> void f() {
>   ++do_something;
>   foo(1, 2, 3);
>   ++do_something;
> }
>
> We'd generate locations like:
>   .loc line 1
>   incl do_something(%rip)
>   movl $1, %ecx
>   movl $2, %edx
>   movl $3, %r8d
>   .loc line 2 # line 2 starts here, instead of 3 instructions earlier
>   callq foo
>   .loc line 3
>   incl do_something(%rip)
>
>
> Our users really wanted the line table entry for line 2 to include the
> constant materialization code for some VS debugger feature.
>
>
> Got it. (For others following along, there's more discussion about this
> debugger feature in the commit description for r327581.)
>
>
>
> I think if you remove locations from ConstantSDNodes, you might want to
> add a late pass that propagates source locations backwards onto
> location-less instructions. This would also avoid some special cases when a
> basic block starts with an instruction that lacks location information.
> See CodeViewDebug::beginInstruction and DwarfDebug::beginInstruction for
> what we do today.
>
> Interesting, I hadn't considered doing this.
>
> What happens in the special case where a block starts with a location-less
> instruction? Ah, I see that CodeViewDebug::beginInstruction does a forward
> scan to find the first instruction with location. This can fail though:
> there might not be such an instruction, in which case... I assume we either
> apply a line-0 location, or nothing at all, similar to
> Dwarf::beginInstruction?
>
> It's a bit unclear to me what the benefits of a late pass that
> backwards-propagated locations would be. I suppose we'd have fewer
forward
> scans in CodeViewDebug::beginInstruction, so is the benefit a compile-time
> win (which would offset the cost of a late pass)? Would the final debug
> info quality would be better in some way I'm just missing?
>
> thanks!
> vedant
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180621/9860ef2c/attachment.html>

Thanks for your feedback everyone.

I'll try to summarize what people have said:

- There seems to be general consensus that we should move away from applying a
static debug location to constant SD nodes. This should improve single-stepping
and be more consistent with how constants are modeled in IR.
(I think Björn suggested working around this by applying an is_stmt attribute to
constant materialization instructions, but there are a number of barriers to
doing that (e.g we only have partial compiler/debugger support for doing this).)

- It may be helpful to have a late pass backfill missing locations. This would
make a constant materialization look like a part of the expression it's for.
I think this needs a bit more thought. For example: how would we avoid
backfilling bogus locations onto instructions which aren't used to
materialize constants?

For now, I'll get started on patches to drop debug locations from constant
nodes.

vedant
> On Jun 21, 2018, at 11:50 AM, Eric Christopher via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Yeah, I'm with you here. I don't think there's any particular
debug info gain to having debug info on materializing a constant either.
> 
> Vedant: Ultimately I think I'm in favor of this plan.
> 
> Thanks!
> 
> -eric
> 
> On Wed, Jun 20, 2018 at 4:32 PM Matthias Braun via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> FWIW: Debug information on constants feels odd to me. They are just values
not something that is executed so conceptually I would not expect them to
"happen" at a specific time/place in the program. That said most
numbers are copied into registers or stored into memory and that is of course an
interesting action. So in the original example I would hope to see debug info on
whatever instructions are used to fill the array with values.
> 
> That said I'm not familiar with the inner workings of dwarf or other
debugger formats, so it may very well be reasonable to backfill the information
in a late pass to avoid having assembler instructions without debug info as some
people proposed.
> 
> - Matthias
> 
>> On Jun 20, 2018, at 11:09 AM, via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> DwarfDebug::beginInstruction sets the location to line 0 because
otherwise the location is implicitly the same location as the last instruction
of the physically preceding block.  That location is often completely unrelated
to what's at the top of the new block.  Line 0 isn't great, but at least
it's not a complete lie.
>> --paulr
>>   <>
>> From: vsk at apple.com <mailto:vsk at apple.com> [mailto:vsk at
apple.com <mailto:vsk at apple.com>]
>> Sent: Wednesday, June 20, 2018 1:48 PM
>> To: Reid Kleckner
>> Cc: Robinson, Paul; llvm-dev; Justin Bogner; David Li; David Blaikie
>> Subject: Re: [RFC] Removing debug locations from ConstantSDNodes
>>  
>> On Jun 19, 2018, at 6:36 PM, Reid Kleckner <rnk at google.com
<mailto:rnk at google.com>> wrote:
>>  
>> On Tue, Jun 19, 2018 at 5:46 PM Vedant Kumar <vsk at apple.com
<mailto:vsk at apple.com>> wrote:
>> Someone (Reid?) mentioned that we could try sinking constants to their
point of first use as an alternative, and (IIUC) create new nodes with distinct
DebugLocs for each use of a constant. I don't recall the details of that
alternative clearly. Based on my (likely incorrect) understanding of it,
dropping locations from constants outright might be simpler.
>>  
>> Our use case was in fastisel, so things are different. I don't
think my solution will help you.
>>  
>> In our case, users were complaining about code like this:
>> volatile int do_something;
>> void f() {
>>   ++do_something;
>>   foo(1, 2, 3);
>>   ++do_something;
>> }
>>  
>> We'd generate locations like:
>>   .loc line 1
>>   incl do_something(%rip)
>>   movl $1, %ecx
>>   movl $2, %edx
>>   movl $3, %r8d
>>   .loc line 2 # line 2 starts here, instead of 3 instructions earlier
>>   callq foo
>>   .loc line 3
>>   incl do_something(%rip)
>> 
>> 
>> Our users really wanted the line table entry for line 2 to include the
constant materialization code for some VS debugger feature.
>>  
>> Got it. (For others following along, there's more discussion about
this debugger feature in the commit description for r327581.)
>> 
>> 
>> 
>> I think if you remove locations from ConstantSDNodes, you might want to
add a late pass that propagates source locations backwards onto location-less
instructions. This would also avoid some special cases when a basic block starts
with an instruction that lacks location information. See
CodeViewDebug::beginInstruction and DwarfDebug::beginInstruction for what we do
today.
>>  
>> Interesting, I hadn't considered doing this.
>>  
>> What happens in the special case where a block starts with a
location-less instruction? Ah, I see that CodeViewDebug::beginInstruction does a
forward scan to find the first instruction with location. This can fail though:
there might not be such an instruction, in which case... I assume we either
apply a line-0 location, or nothing at all, similar to Dwarf::beginInstruction?
>>  
>> It's a bit unclear to me what the benefits of a late pass that
backwards-propagated locations would be. I suppose we'd have fewer forward
scans in CodeViewDebug::beginInstruction, so is the benefit a compile-time win
(which would offset the cost of a late pass)? Would the final debug info quality
would be better in some way I'm just missing?
>>  
>> thanks!
>> vedant
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180621/02ce20db/attachment.html>