On Wed, Feb 25, 2015 at 10:40 AM, Alexandre Oliva <aol...@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 <aol...@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
