Re: RFA: New optimization: Convert local statics into non-statics

["Steven Bosscher" <stevenb dot gcc at gmail dot com>]

Nick Clifton wrote:

> +@c optimize-local-statics
> +@item slo-local-variables-threshold
> +This parameter controls whether the static local optimization
> +(@option{-foptimize-local-statics}), if it has been enabled on the
> +command line, will actually be run on a particular function.  The
> +threshold is the maximum number of active (ie actually used) local
> +variables in the function.  If the function has more than the
> +threshold number of active local variables then the optimization will
> +not be run.

Seems rather user unfriendly.  How does the user know how many active
local variables there are?


> *** /dev/null	2008-07-14 11:16:10.618000671 +0100
> --- gcc/tree-optimize-local-statics.c	2008-07-14 15:31:33.000000000 +0100
> ***************
> *** 0 ****
> --- 1,560 ----
> + /* Copyright (C) 2008 Free Software Foundation, Inc.
> +    Contributed by Diego Novillo <dnovillo@redhat.com>

Diego is not at Redhat anymore AFAIK.


> + static inline void
> + mark_as_seen (tree var)
> + {
> +   pointer_set_insert (visited, var);
> + }

Please add a comment before this function.
(see http://gcc.gnu.org/codingconventions.html and especially
http://www.gnu.org/prep/standards/html_node/Comments.html#Comments)


> + static inline bool
> + already_seen (tree var)
> + {
> +   return pointer_set_contains (visited, var);
> + }

Same here.


> + static pending_conversion *
> + get_free_pending (void)
> + {

Same here.


> + static inline void
> + remove_from_pending_list (pending_conversion * pend,
> + 			  pending_conversion * prev)

Same here.


> + static void
> + remove_all_occurences_from_pending_list (tree var)

Same here.


> +
> + 		if (dump_file && (dump_flags & TDF_DETAILS))
> + 		  fprintf (dump_file, "\
> + '%s' not converted because DEF in block %d did not dominate USE in block %d.\n",
> + 			   lang_hooks.decl_printable_name (var, 0),
> + 			   pends_bb->index,
> + 			   bb->index);

Why not just
+ 		  fprintf (dump_file,
+                    "'%s' not converted because DEF in block %d did not "
+                    "dominate USE in block %d.\n",
+ 			   lang_hooks.decl_printable_name (var, 0),
+ 			   pends_bb->index,
+ 			   bb->index);

You do this in many places.  Why?  You can just start the string on
the next line. And you can break the string before the 80th column and
continue on the next line if your string is too long.



> + 	      fprintf (dump_file, "\
> + DEF of '%s' in block %d dominates USE in block %d.\n",
> + 		       lang_hooks.decl_printable_name (var, 0),
> + 		       pend->bb->index, bb->index);

Same here.


> +
> + 	ret = TRUE;
> +       }

Not GNU style.  Note in general that bool values are 'true' and
'false', not 'TRUE' and 'FALSE'.



> +     fprintf (dump_file, "\
> + '%s' USEd in block %d and not on pending list, so it cannot be made auto.\n",
> + 	     lang_hooks.decl_printable_name (var, 0),
> + 	     bb->index);

Strange string again.


> + static void
> + make_auto (tree var)

No comment before the function again.


> +   TREE_STATIC (var) = 0;
> +   /* Since the var is no longer static, it no
> +      longer needs space in the assembler file.  */
> +   TREE_ASM_WRITTEN (var) = 1;

This looks like a hack.  What do we do for normal non-static local
variables?  Set TREE_ASM_WRITTEN for them too?


> + static void
> + do_not_make_auto (tree var)

Again no comment.  I sometimes wonder why people just keep
contributing patches with the same mistakes.


> +   if (dump_file && (dump_flags & TDF_DETAILS))
> +     fprintf (dump_file, "\
> + Static local '%s' is not suitable for conversion to auto.\n",
> + 	     lang_hooks.decl_printable_name (var, 0));
> + }

Strange string again.


> + static inline bool
> + is_local_static_var (tree decl)

No comment before the function again.

> + {
> +   return
> +     decl != NULL
> +     /* Check that we are looking at a variable declaration.  */
> +     && TREE_CODE (decl) == VAR_DECL
> +     /* That has storeage allocated in this
> +        compilation unit (rather than on the stack).  */
> +     && TREE_STATIC (decl)
> +     /* Which is not visible externally to this compilation unit.  */
> +     && ! TREE_PUBLIC (decl)
> +     /* Which is allocated inside a function.  */
> +     && decl_function_context (decl) != NULL
> +     /* Which was not created by the compiler.  */
> +     && ! DECL_ARTIFICIAL (decl)
> +     /* And which is not used in a nested function.  */
> +     && ! DECL_NONLOCAL (decl);
> + }

Please split this up in multiple statements.


> + static inline bool
> + is_local_auto_var (tree decl)

No comment before the function.  Also please split up in multiple statements.


> + /* Transform static declarations into auto declarations.  This allows the
> +    variable to be allocated to a register.  A static variable V can be
> +    converted iff:
> +
> +    1- V is local to the function.

What about inlining?  It's not really clear to me how this
optimization works in this case.  You only look at the current
function.


> + static void
> + process (tree rhs, tree lhs, basic_block bb)

No comment before the function. Long and strange strings. Etc.


> + static void
> + locate_and_convert_statics (struct dom_walk_data * walk_data ATTRIBUTE_UNUSED,
> + 			    basic_block bb,

No comment before the function.


> + static unsigned int
> + tree_optimize_local_statics (void)

No comment before the function. Long and strange strings. Etc.


> +   <ul>
> +     <li>A new optimization has been added to convert static variables local to a
> +     function into non-static variables when it can be shown that it is safe to
> +     do so.  (Ie when it can be proved that they are always initialized before

do so (i.e. ...).

> +     <p>The optimization has to be explicitly enabled, using the new command
> +     line option <code>-foptimize-local-statics</code>

Adding *any* optimization that isn't enabled by default is a bad idea
IMHO.  GCC has many of them and they all suffer from severe bit rot,
if they ever even worked at all.  If I had a say in it, it'd be "no".
Why can't this be enabled by default at some optimization level?  Is
it too uncommon?

How often does this optimization trigger in a GCC bootstrap? Or for a
Fortran 77 benchmark (where all variables begin as static storage)?

Is there any way to make the pass quicker by e.g. adding a pre-pass
over all local variables to see if there are any static locals at all
in the function, before traversing the whole function?

And what I said above: What about inlining?  GCC can actually inline
functions with static local variables.  Is that why you run this pass
before all other optimizations?

It seems you run it before all inlining and not on SSA form, right?
So the optimization puts constraints on the passes pipeline.  Can't
this pass be implemented much more efficiently in tree-ssa by
following DEF-USE chains where the DEF is a load from the local static
variable?

To be honest, I wonder why we need this whole optimization at all.
What is the performance impact of this optimization on this benchmark
you apparently can't mention?  Have you analyzed whether you can get a
similar speedup with tricks like enabling gcse load and store motion?
And if that doesn't help for your benchmark: Why not?

Gr.
Steven