[PR58315] reset inlined debug vars at return-to point

[PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

This patch fixes a problem that has been with us for several years.
Variable tracking has no idea about the end of the lifetime of inlined
variables, so it keeps on computing locations for them over and over,
even though the computed locations make no sense whatsoever because the
variable can't even be accessed any more.

With this patch, we unbind all inlined variables at the point the
inlined function returns to, so that the locations for those variables
will not be touched any further.

In theory, we could do something similar to non-inlined auto variables,
when they go out of scope, but their decls apply to the entire function
and I'm told gdb sort-of expects the variables to be accessible
throughout the function, so I'm not tackling that in this patch, for I'm
happy enough with what this patch gets us:

- almost 99% reduction in the output asm for the PR testcase

- more than 90% reduction in the peak memory use compiling that testcase

- 63% reduction in the compile time for that testcase

What's scary is that the testcase is not particularly pathological.  Any
function that calls a longish sequence of inlined functions, that in
turn call other inline functions, and so on, something that's not
particularly unusual in C++, will likely observe significant
improvement, as we won't see growing sequences of var_location notes
after each call or so, as var-tracking computes a new in-stack location
for the implicit this argument of each previously-inlined function.

Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?


Reset inlined debug variables at the end of the inlined function

From: Alexandre Oliva <aoliva@redhat.com>

for  gcc/ChangeLog

	PR debug/58315
	* tree-inline.c (reset_debug_binding): New.
	(reset_debug_bindings): Likewise.
	(expand_call_inline): Call it.
---
 gcc/tree-inline.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index d8abe03..5b58d8b 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -4345,6 +4345,60 @@ add_local_variables (struct function *callee, struct function *caller,
       }
 }
 
+/* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might
+   have brought in or introduced any debug stmts for SRCVAR.  */
+
+static inline void
+reset_debug_binding (copy_body_data *id, tree srcvar, gimple_seq *bindings)
+{
+  tree *remappedvarp = id->decl_map->get (srcvar);
+
+  if (!remappedvarp)
+    return;
+
+  if (TREE_CODE (*remappedvarp) != VAR_DECL)
+    return;
+
+  if (*remappedvarp == id->retvar || *remappedvarp == id->retbnd)
+    return;
+
+  tree tvar = target_for_debug_bind (*remappedvarp);
+  if (!tvar)
+    return;
+
+  gdebug *stmt = gimple_build_debug_bind (tvar, NULL_TREE,
+					  id->call_stmt);
+  gimple_seq_add_stmt (bindings, stmt);
+}
+
+/* For each inlined variable for which we may have debug bind stmts,
+   add before GSI a final debug stmt resetting it, marking the end of
+   its life, so that var-tracking knows it doesn't have to compute
+   further locations for it.  */
+
+static inline void
+reset_debug_bindings (copy_body_data *id, gimple_stmt_iterator gsi)
+{
+  tree var;
+  unsigned ix;
+  gimple_seq bindings = NULL;
+
+  if (!gimple_in_ssa_p (id->src_cfun))
+    return;
+
+  if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
+    return;
+
+  for (var = DECL_ARGUMENTS (id->src_fn);
+       var; var = DECL_CHAIN (var))
+    reset_debug_binding (id, var, &bindings);
+
+  FOR_EACH_LOCAL_DECL (id->src_cfun, ix, var)
+    reset_debug_binding (id, var, &bindings);
+
+  gsi_insert_seq_before_without_update (&gsi, bindings, GSI_SAME_STMT);
+}
+
 /* If STMT is a GIMPLE_CALL, replace it with its inline expansion.  */
 
 static bool
@@ -4650,6 +4704,8 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id)
   	     GCOV_COMPUTE_SCALE (cg_edge->frequency, CGRAPH_FREQ_BASE),
 	     bb, return_block, NULL);
 
+  reset_debug_bindings (id, stmt_gsi);
+
   /* Reset the escaped solution.  */
   if (cfun->gimple_df)
     pt_solution_reset (&cfun->gimple_df->escaped);


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Wed, Feb 25, 2015 at 10:40 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> This patch fixes a problem that has been with us for several years.
> Variable tracking has no idea about the end of the lifetime of inlined
> variables, so it keeps on computing locations for them over and over,
> even though the computed locations make no sense whatsoever because the
> variable can't even be accessed any more.
>
> With this patch, we unbind all inlined variables at the point the
> inlined function returns to, so that the locations for those variables
> will not be touched any further.
>
> In theory, we could do something similar to non-inlined auto variables,
> when they go out of scope, but their decls apply to the entire function
> and I'm told gdb sort-of expects the variables to be accessible
> throughout the function, so I'm not tackling that in this patch, for I'm
> happy enough with what this patch gets us:
>
> - almost 99% reduction in the output asm for the PR testcase
>
> - more than 90% reduction in the peak memory use compiling that testcase
>
> - 63% reduction in the compile time for that testcase
>
> What's scary is that the testcase is not particularly pathological.  Any
> function that calls a longish sequence of inlined functions, that in
> turn call other inline functions, and so on, something that's not
> particularly unusual in C++, will likely observe significant
> improvement, as we won't see growing sequences of var_location notes
> after each call or so, as var-tracking computes a new in-stack location
> for the implicit this argument of each previously-inlined function.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

But code-motion could still move stmts from the inlined functions
across these resets?  That said - shouldn't this simply performed
by proper var-tracking u-ops produced by a backward scan over the
function for "live" scope-blocks?  That is, when you see a scope
block becoming live from exit then add u-ops resetting all
vars from that scope block?

Your patch as-is would add very many debug stmts to for example
tramp3d.  And as you say, the same reasoning applies to scopes
in general, not just for inlines.

Richard.

> Reset inlined debug variables at the end of the inlined function
>
> From: Alexandre Oliva <aoliva@redhat.com>
>
> for  gcc/ChangeLog
>
>         PR debug/58315
>         * tree-inline.c (reset_debug_binding): New.
>         (reset_debug_bindings): Likewise.
>         (expand_call_inline): Call it.
> ---
>  gcc/tree-inline.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index d8abe03..5b58d8b 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4345,6 +4345,60 @@ add_local_variables (struct function *callee, struct function *caller,
>        }
>  }
>
> +/* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might
> +   have brought in or introduced any debug stmts for SRCVAR.  */
> +
> +static inline void
> +reset_debug_binding (copy_body_data *id, tree srcvar, gimple_seq *bindings)
> +{
> +  tree *remappedvarp = id->decl_map->get (srcvar);
> +
> +  if (!remappedvarp)
> +    return;
> +
> +  if (TREE_CODE (*remappedvarp) != VAR_DECL)
> +    return;
> +
> +  if (*remappedvarp == id->retvar || *remappedvarp == id->retbnd)
> +    return;
> +
> +  tree tvar = target_for_debug_bind (*remappedvarp);
> +  if (!tvar)
> +    return;
> +
> +  gdebug *stmt = gimple_build_debug_bind (tvar, NULL_TREE,
> +                                         id->call_stmt);
> +  gimple_seq_add_stmt (bindings, stmt);
> +}
> +
> +/* For each inlined variable for which we may have debug bind stmts,
> +   add before GSI a final debug stmt resetting it, marking the end of
> +   its life, so that var-tracking knows it doesn't have to compute
> +   further locations for it.  */
> +
> +static inline void
> +reset_debug_bindings (copy_body_data *id, gimple_stmt_iterator gsi)
> +{
> +  tree var;
> +  unsigned ix;
> +  gimple_seq bindings = NULL;
> +
> +  if (!gimple_in_ssa_p (id->src_cfun))
> +    return;
> +
> +  if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
> +    return;
> +
> +  for (var = DECL_ARGUMENTS (id->src_fn);
> +       var; var = DECL_CHAIN (var))
> +    reset_debug_binding (id, var, &bindings);
> +
> +  FOR_EACH_LOCAL_DECL (id->src_cfun, ix, var)
> +    reset_debug_binding (id, var, &bindings);
> +
> +  gsi_insert_seq_before_without_update (&gsi, bindings, GSI_SAME_STMT);
> +}
> +
>  /* If STMT is a GIMPLE_CALL, replace it with its inline expansion.  */
>
>  static bool
> @@ -4650,6 +4704,8 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id)
>              GCOV_COMPUTE_SCALE (cg_edge->frequency, CGRAPH_FREQ_BASE),
>              bb, return_block, NULL);
>
> +  reset_debug_bindings (id, stmt_gsi);
> +
>    /* Reset the escaped solution.  */
>    if (cfun->gimple_df)
>      pt_solution_reset (&cfun->gimple_df->escaped);
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Jakub Jelinek]

On Wed, Feb 25, 2015 at 11:54:16AM +0100, Richard Biener wrote:
> > Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?
> 
> But code-motion could still move stmts from the inlined functions
> across these resets?  That said - shouldn't this simply performed
> by proper var-tracking u-ops produced by a backward scan over the
> function for "live" scope-blocks?  That is, when you see a scope
> block becoming live from exit then add u-ops resetting all
> vars from that scope block?

Yeah, wanted to say the same, I'm afraid such a change will very much affect
debugging experience close to the end of inlined functions, as sinking,
scheduling and various other passes may move statements from the inline
functions across those resets.  And, various tools and users really want to
be able to inspect variables and parameters on the return statement.

So, IMHO to do something like this, we'd need to mark those debug stmts
some way to say that they aren't normal debug binds, but debug binds at the
end of scopes (whether inlined functions or just lexical blocks), and
optimization passes that perform code motion should try to detect the case
when they are moving downward some statements across such debug stmts and
move those debug stmts along with those if possible.

And another thing is the amount of the added debug stmts, right now we don't
add debug stmts all the time for everything, just when something is needed,
while your patch adds it unconditionally, even when debug stmts for those
won't be really emitted.  As they are just resets, that hopefully will not
drastically affect var-tracking time, but might affect other optimization
passes, which would need to deal with much more statements than before.

	Jakub

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 25, 2015, Richard Biener <richard.guenther@gmail.com> wrote:

> But code-motion could still move stmts from the inlined functions
> across these resets?

Sure, just like it could still move stmts across any other debug stmts.
Once you return from a function, it's as if all of its variables ceased
to exist, so what is the problem with this?

The real error, IMHO, is to assume the moved instruction is still inside
the inline function.  It isn't.  If you wanted to inspect the variable
before it went out of scope, the debugger should have helped you stop
there, not stop you at an instruction that is outside the expected flow.

> That said - shouldn't this simply performed by proper var-tracking
> u-ops produced by a backward scan over the function for "live"
> scope-blocks?

Please elaborate (ideally with a patch); I have no idea of how you plan
to map scope blocks to their original (and thus correct) position (i.e.,
before any code motion).

> That is, when you see a scope block becoming live from exit then add
> u-ops resetting all vars from that scope block?

Oh, you want code motion artifacts to leak into the language VM
execution modeled in debug info?  That would be fundamentally against
the model implemented with debug stmts and insns.  /me mumbles something
about mixing metaphors and leaky screwdrivers ;-D

IOW, I don't think that would be appropriate at all.  Remember the VTA
presentation at the GCC Summit years ago, when I showed you just can't
hope to recover debug info from code already mangled by the compiler,
because optimizations are lossy?  You get to a point in which you don't
know what you don't know, so you're bound to make wrong decisions.  So,
take note of what you're going to need when you know it's still
accurate.

> Your patch as-is would add very many debug stmts to for example
> tramp3d.

Do you have any evidence that this would have a negative impact on
compile time, memory use or any other relevant metric?  Odds are that,
if it makes any difference whatsoever, it will be a very positive one.

The absolute worst case of this patch is doubling the debug stmt count
(proof: it will add at most one debug stmt per inlined variable that had
at least one debug stmt).

Now, if you're concerned about debug stmt count, we could introduce
another debug stmt/insn form that can reset multiple variables in a
single swoop.  It does not seem to me like this would be worth the
effort.

> And as you say, the same reasoning applies to scopes in general, not
> just for inlines.

I actually said the opposite.  We turn block-local variables into
function-wide declarations very early, so apparently we *can* reference
the variables even when they're out of scope.  But we *cannot* do that
for inlined variables.  That's why I drew the line where I did.  (Plus,
introducing the debug temps at the point I did was easy to try, and it
had a huge positive impact :-)

Sure we *could* introduce debug unbind stmts at the end of scopes.  If
you have evidence or even a hunch that this will have a positive effect
on relevant metrics, go for it!

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 25, 2015, Jakub Jelinek <jakub@redhat.com> wrote:

> various tools and users really want to
> be able to inspect variables and parameters on the return statement.

This patch won't affect the return statement.  The resets are at the
return-to statement; if you stop at the return statement (assuming you
have code generated for it in place), you should still get the same
bindings.  Now, if *all* the code for the return statement was optimized
away, and you stop at the subsequent instruction, that happens to be
past the return, then, yeah, you're out of luck, but then you were
already out of luck before.

Now, there is of course the case in which there is some code left in
place for the return stmt, but it is no longer in its natural position
in the code flow.  You stop there, you're technically out of the inline
scope, but now you're also past the "clobbering" of the inlined
variables.  The real solution for this is to implement the stmt
frontiers I presented at some GCC Summit, so that, when you stop at a
statement, you get the state you expect regardless of code motion,
because you get the state at the natural flow of control, even if node
actual code remained at that point.

> And another thing is the amount of the added debug stmts, right now we don't
> add debug stmts all the time for everything, just when something is needed,

We add debug stmts whenever a trackable (auto "gimple register")
variable is modified.  They are "clobbered" at the end of the inline
function they expanded out of, so this just corrects an long-standing
and quite expensive oversight.

You won't get debug stmts for unused inlined variables, for example: you
should only get them for variables that were remapped while inlining the
code to begin with.  If you got code for them and they are trackable,
you certainly got debug stmts for them as well.

> while your patch adds it unconditionally, even when debug stmts for those
> won't be really emitted.

It shouldn't.  Please show me?

> As they are just resets, that hopefully will not
> drastically affect var-tracking time

They will.  But in a positive way :-)

> but might affect other optimization passes, which would need to deal
> with much more statements than before.

It shouldn't be hard to test this hypothesis one way or another.  Tweak
the code that introduces the new debug temps so that they all refer to
the same fake variable, as opposed to resetting the intended variables,
and then you'll have an exact measure of the compile-time impact
*without* the savings afforded by the resets.  Then we can compare
whether it's an overall win or loss.

My measurements, for a not particularly unusual testcase, showed an
overall reduction of 63% in compile time, as indicated yesterday.  Now,
who should bear the burden of collecting evidence to back up the claims
against the change?  Are those concerns enough to hold it up?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Jakub Jelinek]

On Wed, Feb 25, 2015 at 06:17:33PM -0300, Alexandre Oliva wrote:
> My measurements, for a not particularly unusual testcase, showed an
> overall reduction of 63% in compile time, as indicated yesterday.  Now,
> who should bear the burden of collecting evidence to back up the claims
> against the change?  Are those concerns enough to hold it up?

Can you e.g. run dwlocstat on some larger C++ binaries built without and
with your patch?  I believe dwlocstat is supposed to count only the
instructions where the variables or parameters are in scope, so should be
exactly what we care about here.
E.g. cc1plus and libstdc++.so.6 might be good candidates from gcc itself,
perhaps firefox or similar as something even larger.

	Jakub

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 25, 2015, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Feb 25, 2015 at 06:17:33PM -0300, Alexandre Oliva wrote:
>> My measurements, for a not particularly unusual testcase, showed an
>> overall reduction of 63% in compile time, as indicated yesterday.  Now,
>> who should bear the burden of collecting evidence to back up the claims
>> against the change?  Are those concerns enough to hold it up?

> Can you e.g. run dwlocstat on some larger C++ binaries built without and
> with your patch?  I believe dwlocstat is supposed to count only the
> instructions where the variables or parameters are in scope, so should be
> exactly what we care about here.

Erhm...  I don't think that would cover the case you were worried about,
namely inspecting variables of an inlined function while at a statement
moved out of the function ranges.

Anyway, I've run dwlocstat and inspected the results.  There is indeed a
significant reduction in the coverage, so I looked into that.

What I found out is that the reduction is an improvement on yet another
long-standing var-tracking issue: if a function is called within a loop
and we inline it, bindings from the call in one iteration of the loop
will carry over onto the subsequent iteration until a new binding
occurs.  This accounts for all of the coverage reductions I've
investigated.

This, in turn, suggests that introducing reset stmts when variables go
out of scope even in local blocks will further reduce debug info,
although in some cases it might have the opposite effect.  E.g., if the
actual live range of a variable is scattered across multiple
non-contiguous blocks, stopping the binding from being used in
intervening blocks where the variable is not live will cause additional
entries in the location list.  I've observed this effect with the
proposed patch, too, but it removes a lot more nonsensical entries than
it adds entries to account for not covering intervening ranges by
accident.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Jakub Jelinek]

On Wed, Feb 25, 2015 at 09:01:09PM -0300, Alexandre Oliva wrote:
> > On Wed, Feb 25, 2015 at 06:17:33PM -0300, Alexandre Oliva wrote:
> >> My measurements, for a not particularly unusual testcase, showed an
> >> overall reduction of 63% in compile time, as indicated yesterday.  Now,
> >> who should bear the burden of collecting evidence to back up the claims
> >> against the change?  Are those concerns enough to hold it up?
> 
> > Can you e.g. run dwlocstat on some larger C++ binaries built without and
> > with your patch?  I believe dwlocstat is supposed to count only the
> > instructions where the variables or parameters are in scope, so should be
> > exactly what we care about here.
> 
> Erhm...  I don't think that would cover the case you were worried about,
> namely inspecting variables of an inlined function while at a statement
> moved out of the function ranges.
> 
> Anyway, I've run dwlocstat and inspected the results.  There is indeed a
> significant reduction in the coverage, so I looked into that.

Significant reduction in the coverage should be a red flag.

I admit I haven't read dwlocstat sources recently, but:

dwlocstat
=========

This is a tool for examining Dwarf location info coverage.  It goes
through DIEs of given binary's debug info that represent variables and
function parameters.  For each such DIE, it computes coverage of that
DIE's range (list of addresses of DIE's scope) by location expressions
(a description of where given variable is located at given location:
e.g. a variable can be in a register).

matches what I wanted the tool to do, namely, if you have say some
DW_TAG_variable that is in scope of say an inline function and that
DW_TAG_inlined_subroutine has DW_AT_ranges L1-L2, L3-L4, L5-L6,
it counts on what percentage of bytes in those ranges (or instructions?)
the variable has defined location.
The fear I have is that due to scheduling or any other code motion toward
the end of function, you simply have instructions like L5-L6 above that are
considered part of the function, but that the newly added resets are say on
L4 or so and thus whenever you stop on the L5-L6 instructions, the debugger
will show you you are in the inline function, but you won't be able to
inspect most if not any variables.

> What I found out is that the reduction is an improvement on yet another
> long-standing var-tracking issue: if a function is called within a loop
> and we inline it, bindings from the call in one iteration of the loop
> will carry over onto the subsequent iteration until a new binding
> occurs.  This accounts for all of the coverage reductions I've
> investigated.

It really depends.
Say if you have some
inline void foo (void) { 
  int somevar;
LARGE CODE NOT INITIALIZING somevar;
  somevar = VALUE;
USE somevar;
}
and
for (...)
{
  foo ();
}
and the loop is unrolled, then indeed, lost of coverage between the start of
the second foo and somevar = VALUE; is acceptable, if it previously just
contained the value from previous iteration.  But as I said above, if it is
about the last few stmts in the inline scope, it is undesirable, and if e.g.
the instructions from different unrolled iterations are intermixed, then it
is undesirable too.  Which is why I was suggesting special debug binds that
the optimizers would try to move downward (towards the exit block) on (as
much as possible) any code motions across them, unless the debug binds would
be moved across a debug bind for the same decl - in that case the end of
scope debug bind should be removed rather than moved over.  In most cases,
there won't be much of code motion across the whole function, but only
limited scope, so in the common case you'll still get the effect you are
looking for or close to that, but it won't actually penalize the debug info
quality.

	Jakub

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Thu, Feb 26, 2015 at 1:01 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 25, 2015, Jakub Jelinek <jakub@redhat.com> wrote:
>
>> On Wed, Feb 25, 2015 at 06:17:33PM -0300, Alexandre Oliva wrote:
>>> My measurements, for a not particularly unusual testcase, showed an
>>> overall reduction of 63% in compile time, as indicated yesterday.  Now,
>>> who should bear the burden of collecting evidence to back up the claims
>>> against the change?  Are those concerns enough to hold it up?
>
>> Can you e.g. run dwlocstat on some larger C++ binaries built without and
>> with your patch?  I believe dwlocstat is supposed to count only the
>> instructions where the variables or parameters are in scope, so should be
>> exactly what we care about here.
>
> Erhm...  I don't think that would cover the case you were worried about,
> namely inspecting variables of an inlined function while at a statement
> moved out of the function ranges.
>
> Anyway, I've run dwlocstat and inspected the results.  There is indeed a
> significant reduction in the coverage, so I looked into that.
>
> What I found out is that the reduction is an improvement on yet another
> long-standing var-tracking issue: if a function is called within a loop
> and we inline it, bindings from the call in one iteration of the loop
> will carry over onto the subsequent iteration until a new binding
> occurs.  This accounts for all of the coverage reductions I've
> investigated.
>
> This, in turn, suggests that introducing reset stmts when variables go
> out of scope even in local blocks will further reduce debug info,
> although in some cases it might have the opposite effect.  E.g., if the
> actual live range of a variable is scattered across multiple
> non-contiguous blocks, stopping the binding from being used in
> intervening blocks where the variable is not live will cause additional
> entries in the location list.  I've observed this effect with the
> proposed patch, too, but it removes a lot more nonsensical entries than
> it adds entries to account for not covering intervening ranges by
> accident.

Answering your questions on my mail here (because it fits I think),
and more concentrating on the effect of your patch as opposed to
debug stmt philosophy.

Your patch ends up pruning the var-tracking sets at the additional
reset-points you introduce.  You introduce them at inline boundaries
(which looks reasonable minus code-motion issues).

I claim you can achieve the same result by effectively inserting
those reset debug insns right before var-tracking itself and by
re-computing BLOCK "liveness".  That will automatically deal
with code motion that extends the life-range of an inlined BLOCK
by moving stmts (still associated with that BLOCK) across the
return point of the inlined call (and thus across your debug resets).
And it will allow var-tracking to eventually compute locations for
vars at the "entry" of that BLOCK piece.

After all you say correctly that what matters is location info
for X in places where a debug consumer can successfully lookup
X (that is, in PC ranges where the scope X was declared in is
active).  In other places there is no reason to emit location info
for X (but we might still want to compute it during var-tracking
if at a later PC range the scope will be active again).

Now I said you could do var-tracking uops for those resets somehow,
but I now realize that I have not much internal knowledge of var-tracking.
Thus consider the suggestion to insert reset debug insns at the beginning
of var-tracking and at points where a scope becomes dead (thus after
points where in a backward CFG walk a scope becomes live).

Richard.

> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Jakub Jelinek]

On Thu, Feb 26, 2015 at 11:29:35AM +0100, Richard Biener wrote:
> I claim you can achieve the same result by effectively inserting
> those reset debug insns right before var-tracking itself and by
> re-computing BLOCK "liveness".  That will automatically deal
> with code motion that extends the life-range of an inlined BLOCK
> by moving stmts (still associated with that BLOCK) across the
> return point of the inlined call (and thus across your debug resets).
> And it will allow var-tracking to eventually compute locations for
> vars at the "entry" of that BLOCK piece.

If that would work, it would be nice, but I'm not sure how you can do that.
Using something like final.c (reemit_insn_block_notes) you can discover
the ranges of scopes (inline functions and lexical blocks).
If some scope has a single range, it is the easy case, you know where it
starts and where it ends.  For scopes with multiple ranges, how can you find
out what case it is though?  I mean, either it can be just the case that
some instruction(s) with different scope got scheduled (or sunk / whatever)
in between the instructions of the scope, in that case resetting the vars
there is highly undesirable (would majorly grow the .debug_loc lists and
break debuggers, e.g. when you try to watch some variable through the live
of some inline, if you reset it any time a single unrelated insn is
encountered, the debugger would need to tell you the var got lost/changed).
Or it can be the case of unrolled loop which has lexical scopes or inlines
in it, in that case you will have multiple "same" scopes, but in that case
it is unnecessary to preserve variables in between those, it is really
different inlines.

	Jakub

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Thu, Feb 26, 2015 at 11:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Feb 26, 2015 at 11:29:35AM +0100, Richard Biener wrote:
>> I claim you can achieve the same result by effectively inserting
>> those reset debug insns right before var-tracking itself and by
>> re-computing BLOCK "liveness".  That will automatically deal
>> with code motion that extends the life-range of an inlined BLOCK
>> by moving stmts (still associated with that BLOCK) across the
>> return point of the inlined call (and thus across your debug resets).
>> And it will allow var-tracking to eventually compute locations for
>> vars at the "entry" of that BLOCK piece.
>
> If that would work, it would be nice, but I'm not sure how you can do that.
> Using something like final.c (reemit_insn_block_notes) you can discover
> the ranges of scopes (inline functions and lexical blocks).
> If some scope has a single range, it is the easy case, you know where it
> starts and where it ends.  For scopes with multiple ranges, how can you find
> out what case it is though?  I mean, either it can be just the case that
> some instruction(s) with different scope got scheduled (or sunk / whatever)
> in between the instructions of the scope, in that case resetting the vars
> there is highly undesirable (would majorly grow the .debug_loc lists and
> break debuggers, e.g. when you try to watch some variable through the live
> of some inline, if you reset it any time a single unrelated insn is
> encountered, the debugger would need to tell you the var got lost/changed).
> Or it can be the case of unrolled loop which has lexical scopes or inlines
> in it, in that case you will have multiple "same" scopes, but in that case
> it is unnecessary to preserve variables in between those, it is really
> different inlines.

I don't claim you can get the "maximal" pruning of the var-tracking
solutions.  I just claim that you can do something sensible by computing
where BLOCKs go dead.  And that this would be better than simply
ignoring the code motion issue alltogether.  If Alexs solution would be
acceptable in terms of that issue then we can as well just insert
debug resets for each variable at initial BLOCK boundaries.  After all
if the inliner inserts resets just for vars that already have debug stmts
then I cook up a testcase where those debug stmts only appear
after inlining.

Indeed if we want to be as close to the source as possible we should
insert debug stmts from the start (where the values are still computed)
so that code-motion will simply make it unavailable (and also reset
locations so you don't get gdb jumping around).

Richard.

>
>         Jakub

Re: [PR58315] reset inlined debug vars at return-to point

[Petr Machata]

Jakub Jelinek <jakub@redhat.com> writes:

> it counts on what percentage of bytes in those ranges (or instructions?)
> the variable has defined location.

Yes, it counts bytes.  It doesn't know anything about instruction
lengths etc.

Thanks,
Petr

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 26, 2015, Richard Biener <richard.guenther@gmail.com> wrote:

> and more concentrating on the effect of your patch as opposed to
> debug stmt philosophy.

> (which looks reasonable minus code-motion issues).

> (but we might still want to compute it during var-tracking
> if at a later PC range the scope will be active again).

By taking debug stmt philosophy out of your reasoning, you come to a
conclusion that directly contradicts the very philosophy.  I.e., you're
not setting the philosophy aside, you're setting yourself up for failure
within that philosophy.

Your note on code-motion issues shows another weakness, not in the
philosophy I endorse, but on rather on the one you do.  If annotations
don't move, and they carry all the information debuggers need to care
about, there's no code-motion issue whatsoever, and moving executable
instructions would not have any effects whatsoever.  Sure, we're not
there yet, but if we want to get there, taking steps in the opposite
direction won't take us there any time soon.

> Thus consider the suggestion to insert reset debug insns at the beginning
> of var-tracking and at points where a scope becomes dead (thus after
> points where in a backward CFG walk a scope becomes live).

The point where the scope becomes dead under what philosophical line?

I believe we've already established that trying to recover that
information after throwing it away is cheaper but error prone.

I believe we've already agreed that we don't want debug information to
be incorrect.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 26, 2015, Richard Biener <richard.guenther@gmail.com> wrote:

> After all if the inliner inserts resets just for vars that already
> have debug stmts then I cook up a testcase where those debug stmts
> only appear after inlining.

Please try that.

Hint: the actual requirement is that the VTA-trackable var has been
remapped during inlining of executable code.  Its having debug stmts is
a consequence of its being there.  I wonder, thus, how the compiler
would bring a var that hadn't even been remapped back from the dead, so
as to introduce a debug stmt for it.  Tough! :-)

> Indeed if we want to be as close to the source as possible we should
> insert debug stmts from the start (where the values are still computed)
> so that code-motion will simply make it unavailable (and also reset
> locations so you don't get gdb jumping around).

*nod*

Statement frontiers notes are a proposed solution for the jumping around
and eliminating code motion issues from debug info.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 25, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:

> if a function is called within a loop and we inline it, bindings from
> the call in one iteration of the loop will carry over onto the
> subsequent iteration until a new binding occurs.

Wait, I have to take that back and revisit the code I looked at.
var-tracking handles confluences differently between VTA-tracked and
old-style var-tracking vars, and I was probably confused by that: it
occurred to me this morning that confluence points of VTA-tracked
variables perform a set intersection, whereas old-style VTA performs set
union across locations at all incoming edges.  So, what I have concluded
should only apply to old-style var-tracking vars, or to VTA-tracked vars
in unrolled loops whose intermediate conditions were optimized out (so
that there isn't any confluence with incoming edges without bindings for
the var).

I'm afraid I'll have to look again and figure out what I got wrong.

Apologies for the noise.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 26, 2015, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Feb 25, 2015 at 09:01:09PM -0300, Alexandre Oliva wrote:
>> > On Wed, Feb 25, 2015 at 06:17:33PM -0300, Alexandre Oliva wrote:
>> >> My measurements, for a not particularly unusual testcase, showed an
>> >> overall reduction of 63% in compile time, as indicated yesterday.  Now,
>> >> who should bear the burden of collecting evidence to back up the claims
>> >> against the change?  Are those concerns enough to hold it up?
>> 
>> > Can you e.g. run dwlocstat on some larger C++ binaries built without and
>> > with your patch?  I believe dwlocstat is supposed to count only the
>> > instructions where the variables or parameters are in scope, so should be
>> > exactly what we care about here.
>> 
>> Erhm...  I don't think that would cover the case you were worried about,
>> namely inspecting variables of an inlined function while at a statement
>> moved out of the function ranges.
>> 
>> Anyway, I've run dwlocstat and inspected the results.  There is indeed a
>> significant reduction in the coverage, so I looked into that.

> Significant reduction in the coverage should be a red flag.

Ok, I looked into it further, after patching dwlocstat to dump
per-variable per-range coverage/length info, so as to be able to compare
object files more easily.

So far, all the differences I looked at were caused by padding at the
end of BBs, and by jump stmts without line numbers at the end of BBs,
both right after the debug reset stmts the proposed patch introduces.

I'll dig further, because so far I've only looked at a few cases.  I
have to figure out some way to automate the investigation of these
differences, because it has been too time-intensive, and not really
fruitful in terms of finding the scenarios you're concerned with.

The good news is that, looking deeper into cases that appeared to leak
info across loop iterations, I confirmed I was mistaken in yesterday's
analysis.  Phew! :-)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Petr Machata]

Alexandre Oliva <aoliva@redhat.com> writes:

> Ok, I looked into it further, after patching dwlocstat to dump
> per-variable per-range coverage/length info, so as to be able to compare
> object files more easily.

If you send me those patches, I can finish them, bind the functionality
to a command line option, and merge upstream.

Thanks,
Petr

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Feb 27, 2015, Petr Machata <pmachata@redhat.com> wrote:

> Alexandre Oliva <aoliva@redhat.com> writes:
>> Ok, I looked into it further, after patching dwlocstat to dump
>> per-variable per-range coverage/length info, so as to be able to compare
>> object files more easily.

> If you send me those patches, I can finish them, bind the functionality
> to a command line option, and merge upstream.

Here's what I've got so far.  It's ugly and needs polishing, that I
planned to do once this issue was resolved, but if you feel like beating
me to it, you're surely welcome to ;-)

I'm sure there must be a better way to compute the CIE offset, or even
the base address of the debug info section (at least the "- 11" needs to
be replaced by something more sensible and more portable), but I didn't
try very hard to find it out :-)  Anyway, ideally, dumping it would be
optional; I've added and removed it numerous times depending on the
exact object file I was comparing and what info I sought.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dump-var-range-coverage.patch --]
[-- Type: text/x-patch, Size: 3580 bytes --]

diff --git a/locstats.cc b/locstats.cc
index 8a458cb..0b71ceb 100644
--- a/locstats.cc
+++ b/locstats.cc
@@ -240,7 +240,8 @@ static die_action process_location (Dwarf_Attribute *locattr,
 				    bool full_implicit,
 				    std::bitset<dt__count> &die_type,
 				    mutability_t &mut,
-				    int &coverage);
+				    int &coverage,
+				    bool dump_partials = false);
 
 static die_action process_implicit_pointer (Dwarf_Attribute *locattr,
 					    Dwarf_Op *op,
@@ -400,7 +401,8 @@ process_location (Dwarf_Attribute *locattr,
 		  bool full_implicit,
 		  std::bitset<dt__count> &die_type,
 		  mutability_t &mut,
-		  int &coverage)
+		  int &coverage,
+		  bool dump_partials)
 {
   Dwarf_Op *expr;
   size_t len;
@@ -459,6 +461,7 @@ process_location (Dwarf_Attribute *locattr,
 	{
 	  Dwarf_Addr low = rit->first;
 	  Dwarf_Addr high = rit->second;
+	  size_t const covered_before = covered;
 	  length += high - low;
 	  //std::cerr << " " << low << ".." << high << std::endl;
 
@@ -525,6 +528,10 @@ process_location (Dwarf_Attribute *locattr,
 	      if (cover)
 		covered++;
 	    }
+
+	  if (dump_partials)
+	    std::cout << ' ' << (covered - covered_before)
+		      << '/' << (high - low);
 	}
 
       if (length == 0 || covered == 0)
@@ -562,6 +569,8 @@ is_inlined (Dwarf_Die *die)
 void
 process (Dwarf *dw)
 {
+  const bool dump_partials = true;
+
   // map percentage->occurrences.  Percentage is cov_00..100, where
   // 0..100 is rounded-down integer division.
   std::map<int, unsigned long> tally;
@@ -586,11 +595,16 @@ process (Dwarf *dw)
     for (cu_iterator it = cu_iterator (dw); it != cu_iterator::end (); ++it)
       last_cit = it;
 
+  char *base = NULL;
+
   for (all_dies_iterator it (dw); it != all_dies_iterator::end (); ++it)
     {
       std::bitset<dt__count> die_type;
       Dwarf_Die *die = *it;
 
+      if (!base)
+	base = (char*)die->addr - 11;
+
       if (show_progress)
 	{
 	  cu_iterator cit = it.cu ();
@@ -677,17 +691,29 @@ process (Dwarf *dw)
       if (locattr == NULL)
 	locattr = dwarf_attr (die, DW_AT_const_value, &locattr_mem);
 
-      /*
-      Dwarf_Attribute name_attr_mem,
-	*name_attr = dwarf_attr_integrate (die, DW_AT_name, &name_attr_mem);
-      std::string name = name_attr != NULL
-	? dwarf_formstring (name_attr)
-	: (dwarf_hasattr_integrate (die, DW_AT_artificial)
-	   ? "<artificial>" : "???");
+      if (dump_partials)
+	{
+	  std::string name;
 
-      std::cerr << "die=" << std::hex << die.offset ()
-		<< " '" << name << '\'';
-      */
+	  for (auto pit = it; pit != all_dies_iterator::end ();
+	       pit = pit.parent ())
+	    {
+	      Dwarf_Attribute name_attr_mem,
+		*name_attr = dwarf_attr_integrate (*pit, DW_AT_name, &name_attr_mem);
+	      std::string thisname = name_attr != NULL
+		? dwarf_formstring (name_attr)
+		: (dwarf_hasattr_integrate (*pit, DW_AT_artificial)
+		   ? "<artificial>" : "???");
+
+	      if (name != "")
+		name = thisname + "::" + name;
+	      else
+		name = thisname;
+	    }
+
+	  std::cout // << "die=" << std::hex << (char*)die->addr - base << ' '
+	    << name;
+	}
 
       int coverage;
       mutability_t mut;
@@ -697,7 +723,7 @@ process (Dwarf *dw)
 				interested_mutability,
 				interested_implicit,
 				full_implicit,
-				die_type, mut, coverage) != da_ok)
+				die_type, mut, coverage, dump_partials) != da_ok)
 	    continue;
 	}
       catch (::error &e)
@@ -751,7 +777,7 @@ process (Dwarf *dw)
 
       tally[coverage]++;
       total++;
-      //std::cerr << std::endl;
+      std::cout << std::endl;
     }
 
   if (show_progress)

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Wed, Feb 25, 2015 at 10:40 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> This patch fixes a problem that has been with us for several years.
> Variable tracking has no idea about the end of the lifetime of inlined
> variables, so it keeps on computing locations for them over and over,
> even though the computed locations make no sense whatsoever because the
> variable can't even be accessed any more.
>
> With this patch, we unbind all inlined variables at the point the
> inlined function returns to, so that the locations for those variables
> will not be touched any further.
>
> In theory, we could do something similar to non-inlined auto variables,
> when they go out of scope, but their decls apply to the entire function
> and I'm told gdb sort-of expects the variables to be accessible
> throughout the function, so I'm not tackling that in this patch, for I'm
> happy enough with what this patch gets us:
>
> - almost 99% reduction in the output asm for the PR testcase
>
> - more than 90% reduction in the peak memory use compiling that testcase
>
> - 63% reduction in the compile time for that testcase
>
> What's scary is that the testcase is not particularly pathological.  Any
> function that calls a longish sequence of inlined functions, that in
> turn call other inline functions, and so on, something that's not
> particularly unusual in C++, will likely observe significant
> improvement, as we won't see growing sequences of var_location notes
> after each call or so, as var-tracking computes a new in-stack location
> for the implicit this argument of each previously-inlined function.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

Some numbers for tramp3d at -Ofast -g with an optimized, release checking
(but not bootstrapped) trunk.  Compile-time memory usage seems
unchanged (~980MB, to show differences we'd probably need to
ggc-collect
more often).  Compile-time was slightly faster with the patch, 45s vs. 47s,
but the machine wasn't completely un-loaded.  var-tracking parts:

unpatched:

 variable tracking       :   0.63 ( 1%) usr   0.03 ( 1%) sys   0.82 (
2%) wall   28641 kB ( 2%) ggc
 var-tracking dataflow   :   3.72 ( 8%) usr   0.04 ( 1%) sys   3.65 (
7%) wall    1337 kB ( 0%) ggc
 var-tracking emit       :   2.63 ( 6%) usr   0.02 ( 1%) sys   2.55 (
5%) wall  148835 kB ( 8%) ggc

patched:

 variable tracking       :   0.64 ( 1%) usr   0.01 ( 0%) sys   0.72 (
1%) wall   24202 kB ( 1%) ggc
 var-tracking dataflow   :   1.96 ( 4%) usr   0.01 ( 0%) sys   1.94 (
4%) wall    1326 kB ( 0%) ggc
 var-tracking emit       :   1.46 ( 3%) usr   0.02 ( 0%) sys   1.49 (
3%) wall   46980 kB ( 3%) ggc

we have at the point of RTL expansion 56% more debug statements
(988231 lines with # DEBUG in the .optimized dump out of
1212518 lines in total vs. 630666 out of 854953).  So we go from
roughly 1 debug stmt per real stmt to 1.5 debug stmts per real stmt.

I didn't try to inspect generated debug information or asses its quality.

But I wonder for example what we do for

  _25 = MEM[(const struct DomainD.47403 *)this_11(D) +
8B].D.47690.domain_mD.47464;
  # DEBUG dD.570173 => _25
  # DEBUG dD.570173 => NULL
  # DEBUG thisD.572552 => NULL
  npc_14 = _25 / _27;
  # DEBUG npcD.171771 => npc_14
  # DEBUG D#447 => &this_11(D)->blocks_mD.64412.D.47809
  # DEBUG thisD.572554 => D#447
  # DEBUG dD.570173 => _25
  # DEBUG dD.570173 => NULL
  # DEBUG thisD.572554 => NULL
  remainder_16 = _25 % _27;

we have two pairs of DEBUG stmts for dD.570173 here, binding
to _25 and then immediately resetting.  Apart from that we might
possibly DCE those I wonder if this isn't breaking debug experience
vs. what we had previously:

  _25 = MEM[(const struct DomainD.47403 *)this_11(D) +
8B].D.47690.domain_mD.47464;
  # DEBUG dD.570173 => _25
  npc_14 = _25 / _27;
  # DEBUG npcD.171771 => npc_14
  # DEBUG D#447 => &this_11(D)->blocks_mD.64412.D.47809
  # DEBUG thisD.572554 => D#447
  # DEBUG dD.570173 => _25
  remainder_16 = _25 % _27;

dD.570173 is never reset anywhere in the function without the patch.

Just for curiosity here is the last dump piece again, this time with
location information (we should dump the associated BLOCK someow)

  [tramp3d-v4.cpp:3457:45] _25 = [tramp3d-v4.cpp:3457:45] MEM[(const
struct DomainD.47403 *)this_11(D) + 8B].D.47690.domain_mD.47464;
  [tramp3d-v4.cpp:3457:45] # DEBUG dD.570173 => _25
  [tramp3d-v4.cpp:53176:32] npc_14 = _25 / _27;
  [tramp3d-v4.cpp:53176:32] # DEBUG npcD.171771 => npc_14
  [tramp3d-v4.cpp:53177:33] # DEBUG D#447 => [tramp3d-v4.cpp:53177:33]
&[tramp3d-v4.cpp:53177:19] this_11(D)->blocks_mD.64412.D.47809
  [tramp3d-v4.cpp:53177:33] # DEBUG thisD.572554 => D#447
  [tramp3d-v4.cpp:3457:45] # DEBUG dD.570173 => _25
  [tramp3d-v4.cpp:53177:38] remainder_16 = _25 % _27;

like 3457 is

  inline
  Element_t first() const { return DT::first(this->domain_m); }

and 5317[67] are

  int npc = blocks_m.first() / contexts;
  int remainder = blocks_m.first() % contexts;

'd' comes from DT::first(this->domain_m); which is on line 4604:

  inline
  static Element_t first(Storage_t d) { return d; }

so it assigns a name to this->domain_m but we don't have any
assembler instruction left for that BLOCK.  Which means my
pragmatic approach would have re-set the variable after the
second bind only.  We likely still keep the inline BLOCK
(just wonder what PC range we assign to it and if I could
break on it in gdb...).

Richard.

> Reset inlined debug variables at the end of the inlined function
>
> From: Alexandre Oliva <aoliva@redhat.com>
>
> for  gcc/ChangeLog
>
>         PR debug/58315
>         * tree-inline.c (reset_debug_binding): New.
>         (reset_debug_bindings): Likewise.
>         (expand_call_inline): Call it.
> ---
>  gcc/tree-inline.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index d8abe03..5b58d8b 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4345,6 +4345,60 @@ add_local_variables (struct function *callee, struct function *caller,
>        }
>  }
>
> +/* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might
> +   have brought in or introduced any debug stmts for SRCVAR.  */
> +
> +static inline void
> +reset_debug_binding (copy_body_data *id, tree srcvar, gimple_seq *bindings)
> +{
> +  tree *remappedvarp = id->decl_map->get (srcvar);
> +
> +  if (!remappedvarp)
> +    return;
> +
> +  if (TREE_CODE (*remappedvarp) != VAR_DECL)
> +    return;
> +
> +  if (*remappedvarp == id->retvar || *remappedvarp == id->retbnd)
> +    return;
> +
> +  tree tvar = target_for_debug_bind (*remappedvarp);
> +  if (!tvar)
> +    return;
> +
> +  gdebug *stmt = gimple_build_debug_bind (tvar, NULL_TREE,
> +                                         id->call_stmt);
> +  gimple_seq_add_stmt (bindings, stmt);
> +}
> +
> +/* For each inlined variable for which we may have debug bind stmts,
> +   add before GSI a final debug stmt resetting it, marking the end of
> +   its life, so that var-tracking knows it doesn't have to compute
> +   further locations for it.  */
> +
> +static inline void
> +reset_debug_bindings (copy_body_data *id, gimple_stmt_iterator gsi)
> +{
> +  tree var;
> +  unsigned ix;
> +  gimple_seq bindings = NULL;
> +
> +  if (!gimple_in_ssa_p (id->src_cfun))
> +    return;
> +
> +  if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
> +    return;
> +
> +  for (var = DECL_ARGUMENTS (id->src_fn);
> +       var; var = DECL_CHAIN (var))
> +    reset_debug_binding (id, var, &bindings);
> +
> +  FOR_EACH_LOCAL_DECL (id->src_cfun, ix, var)
> +    reset_debug_binding (id, var, &bindings);
> +
> +  gsi_insert_seq_before_without_update (&gsi, bindings, GSI_SAME_STMT);
> +}
> +
>  /* If STMT is a GIMPLE_CALL, replace it with its inline expansion.  */
>
>  static bool
> @@ -4650,6 +4704,8 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id)
>              GCOV_COMPUTE_SCALE (cg_edge->frequency, CGRAPH_FREQ_BASE),
>              bb, return_block, NULL);
>
> +  reset_debug_bindings (id, stmt_gsi);
> +
>    /* Reset the escaped solution.  */
>    if (cfun->gimple_df)
>      pt_solution_reset (&cfun->gimple_df->escaped);
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Mar  4, 2015, Richard Biener <richard.guenther@gmail.com> wrote:

> Compile-time was slightly faster with the patch, 45s vs. 47s,
> but the machine wasn't completely un-loaded.  var-tracking parts:

> unpatched:

>  variable tracking       :   0.63 ( 1%) usr   0.03 ( 1%) sys   0.82 (
> 2%) wall   28641 kB ( 2%) ggc
>  var-tracking dataflow   :   3.72 ( 8%) usr   0.04 ( 1%) sys   3.65 (
> 7%) wall    1337 kB ( 0%) ggc
>  var-tracking emit       :   2.63 ( 6%) usr   0.02 ( 1%) sys   2.55 (
> 5%) wall  148835 kB ( 8%) ggc

> patched:

>  variable tracking       :   0.64 ( 1%) usr   0.01 ( 0%) sys   0.72 (
> 1%) wall   24202 kB ( 1%) ggc
>  var-tracking dataflow   :   1.96 ( 4%) usr   0.01 ( 0%) sys   1.94 (
> 4%) wall    1326 kB ( 0%) ggc
>  var-tracking emit       :   1.46 ( 3%) usr   0.02 ( 0%) sys   1.49 (
> 3%) wall   46980 kB ( 3%) ggc

> we have at the point of RTL expansion 56% more debug statements
> (988231 lines with # DEBUG in the .optimized dump out of
> 1212518 lines in total vs. 630666 out of 854953).  So we go from
> roughly 1 debug stmt per real stmt to 1.5 debug stmts per real stmt.

So, if I got this right, all these extra debug stmts and insns had no
effect whatsoever on compile time proper.  The reduction in compile time
can be entirely accounted for by the time they save in the var-tracking
parts, and any compile time increase they bring about in other passes is
negligible.

Does this match your assessment of the impact of the patch?


> we have two pairs of DEBUG stmts for dD.570173 here, binding
> to _25 and then immediately resetting.

They're at different lines, and associated with different statements, so
once we have stmt frontiers support in GCC and GDB, you will be able to
stop between them an inspect the state, regardless of the absence of
executable code between them.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On March 5, 2015 8:26:42 PM CET, Alexandre Oliva <aoliva@redhat.com> wrote:
>On Mar  4, 2015, Richard Biener <richard.guenther@gmail.com> wrote:
>
>> Compile-time was slightly faster with the patch, 45s vs. 47s,
>> but the machine wasn't completely un-loaded.  var-tracking parts:
>
>> unpatched:
>
>>  variable tracking       :   0.63 ( 1%) usr   0.03 ( 1%) sys   0.82 (
>> 2%) wall   28641 kB ( 2%) ggc
>>  var-tracking dataflow   :   3.72 ( 8%) usr   0.04 ( 1%) sys   3.65 (
>> 7%) wall    1337 kB ( 0%) ggc
>>  var-tracking emit       :   2.63 ( 6%) usr   0.02 ( 1%) sys   2.55 (
>> 5%) wall  148835 kB ( 8%) ggc
>
>> patched:
>
>>  variable tracking       :   0.64 ( 1%) usr   0.01 ( 0%) sys   0.72 (
>> 1%) wall   24202 kB ( 1%) ggc
>>  var-tracking dataflow   :   1.96 ( 4%) usr   0.01 ( 0%) sys   1.94 (
>> 4%) wall    1326 kB ( 0%) ggc
>>  var-tracking emit       :   1.46 ( 3%) usr   0.02 ( 0%) sys   1.49 (
>> 3%) wall   46980 kB ( 3%) ggc
>
>> we have at the point of RTL expansion 56% more debug statements
>> (988231 lines with # DEBUG in the .optimized dump out of
>> 1212518 lines in total vs. 630666 out of 854953).  So we go from
>> roughly 1 debug stmt per real stmt to 1.5 debug stmts per real stmt.
>
>So, if I got this right, all these extra debug stmts and insns had no
>effect whatsoever on compile time proper.  The reduction in compile
>time
>can be entirely accounted for by the time they save in the var-tracking
>parts, and any compile time increase they bring about in other passes
>is
>negligible.
>
>Does this match your assessment of the impact of the patch?

For the effect on tramp3d, yes.

The positive effect on var-tracking compile time really looks good.  So I'm tempted to approve the patch for 5.0.

>> we have two pairs of DEBUG stmts for dD.570173 here, binding
>> to _25 and then immediately resetting.
>
>They're at different lines, and associated with different statements,
>so
>once we have stmt frontiers support in GCC and GDB, you will be able to
>stop between them an inspect the state, regardless of the absence of
>executable code between them.

I wonder why we use the same decl uid for two different inline instances though. Do we not remap them during inlining?

Richard.

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On Feb 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:

> So far, all the differences I looked at were caused by padding at the
> end of BBs, and by jump stmts without line numbers at the end of BBs,
> both right after the debug reset stmts the proposed patch introduces.

Further investigation showed there were other sources of spurious
differences:

- copies arising from the expansion of PHI nodes; source code
  information associated with these copies points at the source of the
  copy, which is hardly useful and sensible.

- "optimization" of single-range location lists to a location expression
  covering the entire function, which extends, often incorrectly, the
  range of the expression, and thus the coverage of the function.

By patching GCC so as to eliminate these differences (patches attached
for reference), I managed to reduce the coverage differences in
libgcc_s.so to a manageable size (about a dozen variables in 3 functions
in 2 object files), so I could manually investigate each one of them.

All remaining differences amounted to single insns, originally before
the reset debug stmt introduced by the patch, computing subexpressions
of expressions expanded after the return point of the inlined function.
Because of TER, this often causes stmts to be moved past corresponding
debug stmts.  This is a long-standing and long-known issue to me, thus
nothing particularly new brought about or worsened by the proposed
patch, and that would be fixed with stmt frontiers.

Ultimately, the issue is not so much that the insn gets emitted at a
different spot, but rather the possibility that no insn will remain at
the point where it was expected, which might make it impossible (with
current compiler and debugger) to inspect the results of that
computation.  The worst the reset debug stmts introduced by this patch
could do is to make those effects not visible at other points of the
computation, where they are not guaranteed or expected to be visible to
begin with.

That said, it might be nice if we emitted partial of full expansions of
subexpressions at their original location, relative to debug stmts
expanded to debug insns, rather than all at the point of the full
expression.  I recall having started something along these lines years
ago, but that project never got to a working state.


The differences in libstdc++.so, even after the patches intended to
minimize differences, are too many to investigate in depth, but from
what I can tell from a quick glance at the diff in dwlocstat
per-variable per-range coverage dumps, nearly all of the differences
amount to one insn in the final range, which likely means TER-introduced
differences.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-end-scopes-before-locless-insns.patch --]
[-- Type: text/x-patch, Size: 2714 bytes --]

End scopes before location-less insns

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/final.c |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/gcc/final.c b/gcc/final.c
index 1fa93d9..6fab871 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -1634,13 +1634,19 @@ choose_inner_scope (tree s1, tree s2)
 /* Emit lexical block notes needed to change scope from S1 to S2.  */
 
 static void
-change_scope (rtx_insn *orig_insn, tree s1, tree s2)
+change_scope (rtx_insn *prev_insn, rtx_insn *orig_insn, tree s1, tree s2)
 {
   rtx_insn *insn = orig_insn;
+  rtx_insn *einsn;
   tree com = NULL_TREE;
   tree ts1 = s1, ts2 = s2;
   tree s;
 
+  if (!prev_insn)
+    einsn = insn;
+  else
+    einsn = NEXT_INSN (prev_insn);
+
   while (ts1 != ts2)
     {
       gcc_assert (ts1 && ts2);
@@ -1660,7 +1666,7 @@ change_scope (rtx_insn *orig_insn, tree s1, tree s2)
   s = s1;
   while (s != com)
     {
-      rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, insn);
+      rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, einsn);
       NOTE_BLOCK (note) = s;
       s = BLOCK_SUPERCONTEXT (s);
     }
@@ -1684,6 +1690,7 @@ reemit_insn_block_notes (void)
   tree cur_block = DECL_INITIAL (cfun->decl);
   rtx_insn *insn;
   rtx_note *note;
+  rtx_insn *prev_insn = NULL;
 
   insn = get_insns ();
   for (; insn; insn = NEXT_INSN (insn))
@@ -1693,10 +1700,16 @@ reemit_insn_block_notes (void)
       /* Prevent lexical blocks from straddling section boundaries.  */
       if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
         {
+	  rtx_insn *einsn;
+	  if (prev_insn)
+	    einsn = NEXT_INSN (prev_insn);
+	  else
+	    einsn = insn;
+	  prev_insn = NULL;
           for (tree s = cur_block; s != DECL_INITIAL (cfun->decl);
                s = BLOCK_SUPERCONTEXT (s))
             {
-              rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, insn);
+              rtx_note *note = emit_note_before (NOTE_INSN_BLOCK_END, einsn);
               NOTE_BLOCK (note) = s;
               note = emit_note_after (NOTE_INSN_BLOCK_BEG, insn);
               NOTE_BLOCK (note) = s;
@@ -1732,14 +1745,16 @@ reemit_insn_block_notes (void)
 
       if (this_block != cur_block)
 	{
-	  change_scope (insn, cur_block, this_block);
+	  change_scope (prev_insn, insn, cur_block, this_block);
 	  cur_block = this_block;
 	}
+
+      prev_insn = insn;
     }
 
   /* change_scope emits before the insn, not after.  */
   note = emit_note (NOTE_INSN_DELETED);
-  change_scope (note, cur_block, DECL_INITIAL (cfun->decl));
+  change_scope (NULL, note, cur_block, DECL_INITIAL (cfun->decl));
   delete_insn (note);
 
   reorder_blocks ();

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-no-locs-for-out-of-ssa-copies.patch --]
[-- Type: text/x-patch, Size: 1606 bytes --]

Don't assign locations to out-of-ssa copies.

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/tree-outof-ssa.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/tree-outof-ssa.c b/gcc/tree-outof-ssa.c
index e6310cd..7d15a9a 100644
--- a/gcc/tree-outof-ssa.c
+++ b/gcc/tree-outof-ssa.c
@@ -289,6 +289,7 @@ insert_partition_copy_on_edge (edge e, int dest, int src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   var = partition_to_var (SA.map, src);
   seq = emit_partition_copy (copy_rtx (SA.partition_to_pseudo[dest]),
@@ -327,6 +328,7 @@ insert_value_copy_on_edge (edge e, int dest, tree src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   start_sequence ();
 
@@ -382,6 +384,7 @@ insert_rtx_to_part_on_edge (edge e, int dest, rtx src, int unsignedsrcp,
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   /* We give the destination as sizeexp in case src/dest are BLKmode
      mems.  Usually we give the source.  As we result from SSA names
@@ -418,6 +421,7 @@ insert_part_to_rtx_on_edge (edge e, rtx dest, int src, source_location locus)
   /* If a locus is provided, override the default.  */
   if (locus)
     set_curr_insn_location (locus);
+  set_curr_insn_location (0);
 
   var = partition_to_var (SA.map, src);
   seq = emit_partition_copy (dest,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: vta-no-single-loc-list.patch --]
[-- Type: text/x-patch, Size: 1070 bytes --]

do not optimize single-loc list

From: Alexandre Oliva <aoliva@redhat.com>


---
 gcc/dwarf2out.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index ebf41c8..308c070 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -14209,7 +14209,7 @@ dw_loc_list (var_loc_list *loc_list, tree decl, int want_address)
      representable, we don't want to pretend a single entry that was
      applies to the entire scope in which the variable is
      available.  */
-  if (list && loc_list->first->next)
+  if (list && (1 || loc_list->first->next))
     gen_llsym (list);
 
   return list;
@@ -16049,7 +16049,7 @@ add_location_or_const_value_attribute (dw_die_ref die, tree decl, bool cache_p,
      a constant value.  That way we are better to use add_const_value_attribute
      rather than expanding constant value equivalent.  */
   loc_list = lookup_decl_loc (decl);
-  if (loc_list
+  if (0 && loc_list
       && loc_list->first
       && loc_list->first->next == NULL
       && NOTE_P (loc_list->first->loc)

[-- Attachment #5: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Fri, Mar 6, 2015 at 7:04 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> So far, all the differences I looked at were caused by padding at the
>> end of BBs, and by jump stmts without line numbers at the end of BBs,
>> both right after the debug reset stmts the proposed patch introduces.
>
> Further investigation showed there were other sources of spurious
> differences:
>
> - copies arising from the expansion of PHI nodes; source code
>   information associated with these copies points at the source of the
>   copy, which is hardly useful and sensible.

Care to explain?  We spend quite some resources to maintain them
(locations on PHI args, that is).

Btw, I'd expect tree-ssa-copyrename.c to be a source of bogus
base variables still.

Richard.

> - "optimization" of single-range location lists to a location expression
>   covering the entire function, which extends, often incorrectly, the
>   range of the expression, and thus the coverage of the function.
>
> By patching GCC so as to eliminate these differences (patches attached
> for reference), I managed to reduce the coverage differences in
> libgcc_s.so to a manageable size (about a dozen variables in 3 functions
> in 2 object files), so I could manually investigate each one of them.
>
> All remaining differences amounted to single insns, originally before
> the reset debug stmt introduced by the patch, computing subexpressions
> of expressions expanded after the return point of the inlined function.
> Because of TER, this often causes stmts to be moved past corresponding
> debug stmts.  This is a long-standing and long-known issue to me, thus
> nothing particularly new brought about or worsened by the proposed
> patch, and that would be fixed with stmt frontiers.
>
> Ultimately, the issue is not so much that the insn gets emitted at a
> different spot, but rather the possibility that no insn will remain at
> the point where it was expected, which might make it impossible (with
> current compiler and debugger) to inspect the results of that
> computation.  The worst the reset debug stmts introduced by this patch
> could do is to make those effects not visible at other points of the
> computation, where they are not guaranteed or expected to be visible to
> begin with.
>
> That said, it might be nice if we emitted partial of full expansions of
> subexpressions at their original location, relative to debug stmts
> expanded to debug insns, rather than all at the point of the full
> expression.  I recall having started something along these lines years
> ago, but that project never got to a working state.
>
>
> The differences in libstdc++.so, even after the patches intended to
> minimize differences, are too many to investigate in depth, but from
> what I can tell from a quick glance at the diff in dwlocstat
> per-variable per-range coverage dumps, nearly all of the differences
> amount to one insn in the final range, which likely means TER-introduced
> differences.
>
>
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
>

Re: [PR58315] reset inlined debug vars at return-to point

[Jeff Law]

On 03/09/15 08:38, Richard Biener wrote:
> On Fri, Mar 6, 2015 at 7:04 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Feb 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>>
>>> So far, all the differences I looked at were caused by padding at the
>>> end of BBs, and by jump stmts without line numbers at the end of BBs,
>>> both right after the debug reset stmts the proposed patch introduces.
>>
>> Further investigation showed there were other sources of spurious
>> differences:
>>
>> - copies arising from the expansion of PHI nodes; source code
>>    information associated with these copies points at the source of the
>>    copy, which is hardly useful and sensible.
>
> Care to explain?  We spend quite some resources to maintain them
> (locations on PHI args, that is).
I almost responded to that claim as well, but then thought better of it 
as that patch (AFAICT) wasn't proposed for inclusion, but was being used 
for testing purposes.

Expansion of the PHI into copies should have locations which point to 
the source of the various PHI args.  Those are quite meaningful.

Jeff

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Mar  9, 2015, Jeff Law <law@redhat.com> wrote:

> On 03/09/15 08:38, Richard Biener wrote:
>> On Fri, Mar 6, 2015 at 7:04 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> On Feb 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> 
>>>> So far, all the differences I looked at were caused by padding at the
>>>> end of BBs, and by jump stmts without line numbers at the end of BBs,
>>>> both right after the debug reset stmts the proposed patch introduces.
>>> 
>>> Further investigation showed there were other sources of spurious
>>> differences:
>>> 
>>> - copies arising from the expansion of PHI nodes; source code
>>> information associated with these copies points at the source of the
>>> copy, which is hardly useful and sensible.
>> 
>> Care to explain?  We spend quite some resources to maintain them
>> (locations on PHI args, that is).

The situation I remember, for looking more extensively into, involved a
variable local to the inlined function, a copy of that variable to a
caller's variable, and that caller's variable being further modified
elsewhere.

We coalesced the two variables into one, removed the copy statement
altogether, and the location from the initial set within the inline
function made it to PHI nodes related with the caller's variable, and
that survived various PHI recomputations due to various CFG
reorganizations.  When the time came to expand the PHI nodes, copies
were required and line number info internal to the inlined function was
added to the copy arising from the edge that had in it the SSA name set
inside the inline function.  And so the ranges of the inline function
were extended far past its actual end, because a coalesced copy from one
of its variables was optimized away and then reintroduced with line
number info pertaining to the inlined function.

> I almost responded to that claim as well, but then thought better of
> it as that patch (AFAICT) wasn't proposed for inclusion, but was being
> used for testing purposes.

The patch that removed line number annotation from copies arising from
expanding PHI nodes was included only for reference, yes.  The upthread
patch for PR58315 still stands.

> Expansion of the PHI into copies should have locations which point to
> the source of the various PHI args.  Those are quite meaningful.

I can see those can be meaningful in many cases.  Not in the ones I
looked at as part of investigating the coverage changes, though.  We
have a substantial number of copies that arise from processes like the
one I described above, and those are most certainly not sensible, mainly
because the relevant line number info was dropped on the floor along
with copies optimized away due to coalescing of variables from different
scopes.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Alexandre Oliva]

On Feb 25, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:

> This patch fixes a problem that has been with us for several years.
> Variable tracking has no idea about the end of the lifetime of inlined
> variables, so it keeps on computing locations for them over and over,
> even though the computed locations make no sense whatsoever because the
> variable can't even be accessed any more.

> With this patch, we unbind all inlined variables at the point the
> inlined function returns to, so that the locations for those variables
> will not be touched any further.

> In theory, we could do something similar to non-inlined auto variables,
> when they go out of scope, but their decls apply to the entire function
> and I'm told gdb sort-of expects the variables to be accessible
> throughout the function, so I'm not tackling that in this patch, for I'm
> happy enough with what this patch gets us:

> - almost 99% reduction in the output asm for the PR testcase

> - more than 90% reduction in the peak memory use compiling that testcase

> - 63% reduction in the compile time for that testcase

> What's scary is that the testcase is not particularly pathological.  Any
> function that calls a longish sequence of inlined functions, that in
> turn call other inline functions, and so on, something that's not
> particularly unusual in C++, will likely observe significant
> improvement, as we won't see growing sequences of var_location notes
> after each call or so, as var-tracking computes a new in-stack location
> for the implicit this argument of each previously-inlined function.

> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

Ping?

for  gcc/ChangeLog

	PR debug/58315
	* tree-inline.c (reset_debug_binding): New.
	(reset_debug_bindings): Likewise.
	(expand_call_inline): Call it.
---
 gcc/tree-inline.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 71d75d9..c1578e5 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -4346,6 +4346,60 @@ add_local_variables (struct function *callee, struct function *caller,
       }
 }
 
+/* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might
+   have brought in or introduced any debug stmts for SRCVAR.  */
+
+static inline void
+reset_debug_binding (copy_body_data *id, tree srcvar, gimple_seq *bindings)
+{
+  tree *remappedvarp = id->decl_map->get (srcvar);
+
+  if (!remappedvarp)
+    return;
+
+  if (TREE_CODE (*remappedvarp) != VAR_DECL)
+    return;
+
+  if (*remappedvarp == id->retvar || *remappedvarp == id->retbnd)
+    return;
+
+  tree tvar = target_for_debug_bind (*remappedvarp);
+  if (!tvar)
+    return;
+
+  gdebug *stmt = gimple_build_debug_bind (tvar, NULL_TREE,
+					  id->call_stmt);
+  gimple_seq_add_stmt (bindings, stmt);
+}
+
+/* For each inlined variable for which we may have debug bind stmts,
+   add before GSI a final debug stmt resetting it, marking the end of
+   its life, so that var-tracking knows it doesn't have to compute
+   further locations for it.  */
+
+static inline void
+reset_debug_bindings (copy_body_data *id, gimple_stmt_iterator gsi)
+{
+  tree var;
+  unsigned ix;
+  gimple_seq bindings = NULL;
+
+  if (!gimple_in_ssa_p (id->src_cfun))
+    return;
+
+  if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
+    return;
+
+  for (var = DECL_ARGUMENTS (id->src_fn);
+       var; var = DECL_CHAIN (var))
+    reset_debug_binding (id, var, &bindings);
+
+  FOR_EACH_LOCAL_DECL (id->src_cfun, ix, var)
+    reset_debug_binding (id, var, &bindings);
+
+  gsi_insert_seq_before_without_update (&gsi, bindings, GSI_SAME_STMT);
+}
+
 /* If STMT is a GIMPLE_CALL, replace it with its inline expansion.  */
 
 static bool
@@ -4659,6 +4713,8 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id)
   	     GCOV_COMPUTE_SCALE (cg_edge->frequency, CGRAPH_FREQ_BASE),
 	     bb, return_block, NULL);
 
+  reset_debug_bindings (id, stmt_gsi);
+
   /* Reset the escaped solution.  */
   if (cfun->gimple_df)
     pt_solution_reset (&cfun->gimple_df->escaped);


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PR58315] reset inlined debug vars at return-to point

[Richard Biener]

On Wed, Jun 3, 2015 at 11:55 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 25, 2015, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> This patch fixes a problem that has been with us for several years.
>> Variable tracking has no idea about the end of the lifetime of inlined
>> variables, so it keeps on computing locations for them over and over,
>> even though the computed locations make no sense whatsoever because the
>> variable can't even be accessed any more.
>
>> With this patch, we unbind all inlined variables at the point the
>> inlined function returns to, so that the locations for those variables
>> will not be touched any further.
>
>> In theory, we could do something similar to non-inlined auto variables,
>> when they go out of scope, but their decls apply to the entire function
>> and I'm told gdb sort-of expects the variables to be accessible
>> throughout the function, so I'm not tackling that in this patch, for I'm
>> happy enough with what this patch gets us:
>
>> - almost 99% reduction in the output asm for the PR testcase
>
>> - more than 90% reduction in the peak memory use compiling that testcase
>
>> - 63% reduction in the compile time for that testcase
>
>> What's scary is that the testcase is not particularly pathological.  Any
>> function that calls a longish sequence of inlined functions, that in
>> turn call other inline functions, and so on, something that's not
>> particularly unusual in C++, will likely observe significant
>> improvement, as we won't see growing sequences of var_location notes
>> after each call or so, as var-tracking computes a new in-stack location
>> for the implicit this argument of each previously-inlined function.
>
>> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?
>
> Ping?

Ok for trunk and 5.2 after a while with no issues popping up.

Thanks,
Richard.

> for  gcc/ChangeLog
>
>         PR debug/58315
>         * tree-inline.c (reset_debug_binding): New.
>         (reset_debug_bindings): Likewise.
>         (expand_call_inline): Call it.
> ---
>  gcc/tree-inline.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index 71d75d9..c1578e5 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4346,6 +4346,60 @@ add_local_variables (struct function *callee, struct function *caller,
>        }
>  }
>
> +/* Add to BINDINGS a debug stmt resetting SRCVAR if inlining might
> +   have brought in or introduced any debug stmts for SRCVAR.  */
> +
> +static inline void
> +reset_debug_binding (copy_body_data *id, tree srcvar, gimple_seq *bindings)
> +{
> +  tree *remappedvarp = id->decl_map->get (srcvar);
> +
> +  if (!remappedvarp)
> +    return;
> +
> +  if (TREE_CODE (*remappedvarp) != VAR_DECL)
> +    return;
> +
> +  if (*remappedvarp == id->retvar || *remappedvarp == id->retbnd)
> +    return;
> +
> +  tree tvar = target_for_debug_bind (*remappedvarp);
> +  if (!tvar)
> +    return;
> +
> +  gdebug *stmt = gimple_build_debug_bind (tvar, NULL_TREE,
> +                                         id->call_stmt);
> +  gimple_seq_add_stmt (bindings, stmt);
> +}
> +
> +/* For each inlined variable for which we may have debug bind stmts,
> +   add before GSI a final debug stmt resetting it, marking the end of
> +   its life, so that var-tracking knows it doesn't have to compute
> +   further locations for it.  */
> +
> +static inline void
> +reset_debug_bindings (copy_body_data *id, gimple_stmt_iterator gsi)
> +{
> +  tree var;
> +  unsigned ix;
> +  gimple_seq bindings = NULL;
> +
> +  if (!gimple_in_ssa_p (id->src_cfun))
> +    return;
> +
> +  if (!opt_for_fn (id->dst_fn, flag_var_tracking_assignments))
> +    return;
> +
> +  for (var = DECL_ARGUMENTS (id->src_fn);
> +       var; var = DECL_CHAIN (var))
> +    reset_debug_binding (id, var, &bindings);
> +
> +  FOR_EACH_LOCAL_DECL (id->src_cfun, ix, var)
> +    reset_debug_binding (id, var, &bindings);
> +
> +  gsi_insert_seq_before_without_update (&gsi, bindings, GSI_SAME_STMT);
> +}
> +
>  /* If STMT is a GIMPLE_CALL, replace it with its inline expansion.  */
>
>  static bool
> @@ -4659,6 +4713,8 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id)
>              GCOV_COMPUTE_SCALE (cg_edge->frequency, CGRAPH_FREQ_BASE),
>              bb, return_block, NULL);
>
> +  reset_debug_bindings (id, stmt_gsi);
> +
>    /* Reset the escaped solution.  */
>    if (cfun->gimple_df)
>      pt_solution_reset (&cfun->gimple_df->escaped);
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer