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

Some time ago I saw a report of strange single-stepping behavior given Swift
code that looks like this:

1|  var input = [16, 14, 10, 9, 8, 7, 4, 3, 2, 1] //< Only number 1 is
relevant.
2|  print("start")

TL;DR: The debugger steps from line 2 -> 1 -> 2 in this code example. One
clean way to fix this bug is to remove debug locations from ConstantSDNodes, and
I'm asking if there are any objections to doing that.

In more detail:

The problem appears to be that we assign a debug location to the ConstantSDNode
for "1". When this constant is used again (as it happens to be in the
lowered version of the call to print()) the debugger steps back to the first use
of the constant. Here's a snippet of MIR output that illustrates the
problem:

	%19:gr64 = ADD64ri8 %18, 8, implicit-def dead %eflags; GR64:%19,%18
dbg:destroy-after-foreach.swift:2:7
	%20:gr32 = MOV32ri64 1; GR32:%20 dbg:destroy-after-foreach.swift:1:44
	%21:gr64 = SUBREG_TO_REG 0, killed %20, sub_32bit; GR64:%21 GR32:%20
dbg:destroy-after-foreach.swift:1:44
	%rdi = COPY %21; GR64:%21 dbg:destroy-after-foreach.swift:2:7

The out-of-order stepping behavior is confusing and unexpected to users. ISTM
that the simplest way to fix this bug is to not attach debug locations to
constant SDNodes. This matches the model used by IR (Constants don't have
DebugLocs). As an added benefit, it would let us delete a fair amount of code
which traffics in SDLocs. I'm not sure what (if any) impact this would have
on sampling-based profilers. SamplePGO, at least, has smoothing methods
specifically designed to paper over this sort of "out-of-place" debug
location issue (see
https://storage.googleapis.com/pub-tools-public-publication-data/pdf/45290.pdf
<https://storage.googleapis.com/pub-tools-public-publication-data/pdf/45290.pdf>,
Section 4.1.4, "Sources of Profile Inaccuracies").

I brought all of this up a while ago on IRC: some feedback was positive, but I
didn't get a clear +1 or -1 to proceed.

Please let me know what you think, if there are cleaner alternatives to
consider, etc. Any comments/feedback would be appreciated.

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.

thanks,
vedant
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180619/edf6fd5f/attachment.html>

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180619/460de41a/attachment.html>

Our users really wanted the line table entry for line 2 to include the constant
materialization code for some VS debugger feature.

Same here.  I have serious doubts that the fastisel tactic of bubbling constants
up to the top of the block really helps code size or performance that much, and
it distinctly hurts the debugging experience.  It's reordering code at –O0
and that is generally a Bad Thing™.
--paulr

From: Reid Kleckner [mailto:rnk at google.com]
Sent: Tuesday, June 19, 2018 9:37 PM
To: Vedant Kumar; Robinson, Paul
Cc: llvm-dev; Justin Bogner; David Li; David Blaikie
Subject: Re: [RFC] Removing debug locations from ConstantSDNodes

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180620/85e0dcb0/attachment-0001.html>

+Dehao Chen <dehao at google.com>  to comment on the potential impact. In
general, I prefer the debug info is kept when sample PGO is on.

David


On Tue, Jun 19, 2018 at 5:46 PM Vedant Kumar <vsk at apple.com> wrote:
> Some time ago I saw a report of strange single-stepping behavior given
> Swift code that looks like this:
>
> 1|  var input = [16, 14, 10, 9, 8, 7, 4, 3, 2, 1] //< Only number 1 is
> relevant.
> 2|  print("start")
>
> TL;DR: The debugger steps from line 2 -> 1 -> 2 in this code example.
One
> clean way to fix this bug is to remove debug locations from
> ConstantSDNodes, and I'm asking if there are any objections to doing
that.
>
> In more detail:
>
> The problem appears to be that we assign a debug location to the
> ConstantSDNode for "1". When this constant is used again (as it
happens to
> be in the lowered version of the call to print()) the debugger steps back
> to the first use of the constant. Here's a snippet of MIR output that
> illustrates the problem:
>
> %19:gr64 = ADD64ri8 %18, 8, implicit-def dead %eflags; GR64:%19,%18
> dbg:destroy-after-foreach.swift:2:7
> %20:gr32 = MOV32ri64 1; GR32:%20 dbg:destroy-after-foreach.swift:1:44
> %21:gr64 = SUBREG_TO_REG 0, killed %20, sub_32bit; GR64:%21 GR32:%20
> dbg:destroy-after-foreach.swift:1:44
> %rdi = COPY %21; GR64:%21 dbg:destroy-after-foreach.swift:2:7
>
> The out-of-order stepping behavior is confusing and unexpected to users.
> ISTM that the simplest way to fix this bug is to not attach debug locations
> to constant SDNodes. This matches the model used by IR (Constants don't
> have DebugLocs). As an added benefit, it would let us delete a fair amount
> of code which traffics in SDLocs. I'm not sure what (if any) impact
this
> would have on sampling-based profilers. SamplePGO, at least, has smoothing
> methods specifically designed to paper over this sort of
"out-of-place"
> debug location issue (see
>
https://storage.googleapis.com/pub-tools-public-publication-data/pdf/45290.pdf,
> Section 4.1.4, "Sources of Profile Inaccuracies").
>
> I brought all of this up a while ago on IRC: some feedback was positive,
> but I didn't get a clear +1 or -1 to proceed.
>
> Please let me know what you think, if there are cleaner alternatives to
> consider, etc. Any comments/feedback would be appreciated.
>
> 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.
>
> thanks,
> vedant
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180619/a3131a07/attachment.html>

> 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
<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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180620/b1d5f899/attachment.html>