Patchwork [PR,debug/47106] account used vars only once

login
register
mail settings
Submitter Alexandre Oliva
Date Feb. 14, 2011, 8 p.m.
Message ID <or8vxi9yjm.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/83154/
State New
Headers show

Comments

Alexandre Oliva - Feb. 14, 2011, 8 p.m.
On Feb 14, 2011, Richard Guenther <richard.guenther@gmail.com> wrote:

> referenced-var-lookup-fn-arg.patch, for-each-referenced-var-fn-arg.patch,
> estimated-stack-frame-size-cgraph-node-arg.patch and
> compute-inline-params-ret-void.patch are obvious (and ok), you can apply
> them independently now.

Thanks

> which does look like a hack.  Looking at decl_address_invariant_p it
> _seems_ that the test
>           || DECL_CONTEXT (op) == current_function_decl
>           || decl_function_context (op) == current_function_decl)
> could simply be
>    DECL_CONTEXT (op) && TREE_CODE (DECL_CONTEX (op)) == FUNCTION_DECL

Err...  How would this tell whether the decl is local to the current
function?

> that said, I'm not sure about the necessity of this patch at this stage,
> I'd like to defer it (or rather a better solution) to 4.7 if possible.
> (you might want to open a bug so we don't forget)

*nod*

> Does dont-push-cfun-in-estimate-stack-frame-size.patch depend on this
> patch?

Nope.

> I guess dont-push-cfun-in-estimate-stack-frame-size.patch depends on
> ipa-inline-stack-growth-referenced-vars-pr47106.patch at least?

Yup.

> Similar dont-push-cfun-in-analyze-function.patch?

Yep, but the analyze_function change also depends on the
current_function_decl setting within the body size estimation.

> ipa-inline-stack-growth-referenced-vars-pr47106.patch was attached twice,

Oops, sorry, I meant to attach the -compare patch the second time.

> so I'm not sure about inter-dependencies.  If the do-not-set-cfun patches
> pass testing ontop of the obvious cleanup patches they are ok as well.

Ok, I'll retest and install.

> Now onto ipa-inline-stack-growth-referenced-vars-pr47106.patch

> it looks ok apart from the new pass_inlinable stuff, that at least sounds
> odd and I'd want Honza to have a look here.  The testcase you say
> the patch breaks otherwise redefines a _static_ inline function

Err, no, it's 3 separate functions with different names (foo, __foo and
____foo)

> I also wonder why computing -> inlineable should have any effect on
> that testcase ...

Computing inlinable early enables edges to not be marked as
non-inlinable early.  Computing disregard_* early enables cycles to be
broken at the right spots when deciding the order in which we're going
to process functions.  See the comment right before the two occurrences
of disregard in ipa.c:cgraph_postorder().

> So, can we get the approved pre-requesties applied?

Will do shortly.

Thanks for the reviews.
Richard Guenther - Feb. 15, 2011, 10:38 a.m.
On Mon, Feb 14, 2011 at 9:00 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 14, 2011, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> referenced-var-lookup-fn-arg.patch, for-each-referenced-var-fn-arg.patch,
>> estimated-stack-frame-size-cgraph-node-arg.patch and
>> compute-inline-params-ret-void.patch are obvious (and ok), you can apply
>> them independently now.
>
> Thanks
>
>> which does look like a hack.  Looking at decl_address_invariant_p it
>> _seems_ that the test
>>           || DECL_CONTEXT (op) == current_function_decl
>>           || decl_function_context (op) == current_function_decl)
>> could simply be
>>    DECL_CONTEXT (op) && TREE_CODE (DECL_CONTEX (op)) == FUNCTION_DECL
>
> Err...  How would this tell whether the decl is local to the current
> function?
>
>> that said, I'm not sure about the necessity of this patch at this stage,
>> I'd like to defer it (or rather a better solution) to 4.7 if possible.
>> (you might want to open a bug so we don't forget)
>
> *nod*
>
>> Does dont-push-cfun-in-estimate-stack-frame-size.patch depend on this
>> patch?
>
> Nope.
>
>> I guess dont-push-cfun-in-estimate-stack-frame-size.patch depends on
>> ipa-inline-stack-growth-referenced-vars-pr47106.patch at least?
>
> Yup.
>
>> Similar dont-push-cfun-in-analyze-function.patch?
>
> Yep, but the analyze_function change also depends on the
> current_function_decl setting within the body size estimation.
>
>> ipa-inline-stack-growth-referenced-vars-pr47106.patch was attached twice,
>
> Oops, sorry, I meant to attach the -compare patch the second time.
>
>> so I'm not sure about inter-dependencies.  If the do-not-set-cfun patches
>> pass testing ontop of the obvious cleanup patches they are ok as well.
>
> Ok, I'll retest and install.
>
>> Now onto ipa-inline-stack-growth-referenced-vars-pr47106.patch
>
>> it looks ok apart from the new pass_inlinable stuff, that at least sounds
>> odd and I'd want Honza to have a look here.  The testcase you say
>> the patch breaks otherwise redefines a _static_ inline function
>
> Err, no, it's 3 separate functions with different names (foo, __foo and
> ____foo)
>
>> I also wonder why computing -> inlineable should have any effect on
>> that testcase ...
>
> Computing inlinable early enables edges to not be marked as
> non-inlinable early.  Computing disregard_* early enables cycles to be
> broken at the right spots when deciding the order in which we're going
> to process functions.  See the comment right before the two occurrences
> of disregard in ipa.c:cgraph_postorder().

Ah ok, that indeed makes sense.  Computing node->local separately
from inline-limits is a nice cleanup indeed.

Let's wait for Honzas comment and if he's happy with that part the
patch is ok.

Thanks,
Richard.

>> So, can we get the approved pre-requesties applied?
>
> Will do shortly.
>
> Thanks for the reviews.
>
>
>
>
> --
> 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 Brazil Compiler Engineer
>
>

Patch

FTR, testing and reporting only, not to be installed.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c.orig	2011-02-10 09:27:16.004373533 -0200
+++ gcc/cfgexpand.c	2011-02-10 09:29:49.418374663 -0200
@@ -1311,18 +1311,41 @@  create_stack_guard (void)
   crtl->stack_protect_guard = guard;
 }
 
+/* A subroutine of expand_used_vars.  Walk down through the BLOCK tree
+   expanding variables.  Those variables that can be put into registers
+   are allocated pseudos; those that can't are put on the stack.
+
+   TOPLEVEL is true if this is the outermost BLOCK.  */
+
+static HOST_WIDE_INT
+account_used_vars_for_block (tree block, bool toplevel)
+{
+  tree t;
+  HOST_WIDE_INT size = 0;
+
+  /* Expand all variables at this level.  */
+  for (t = BLOCK_VARS (block); t ; t = DECL_CHAIN (t))
+    size += expand_one_var (t, toplevel, false);
+
+  /* Expand all variables at containing levels.  */
+  for (t = BLOCK_SUBBLOCKS (block); t ; t = BLOCK_CHAIN (t))
+    size += account_used_vars_for_block (t, false);
+
+  return size;
+}
+
 /* Prepare for expanding variables.  */
 static void
-init_vars_expansion (void)
+init_vars_expansion (tree decl)
 {
   tree t;
   unsigned ix;
   /* Set TREE_USED on all variables in the local_decls.  */
-  FOR_EACH_LOCAL_DECL (cfun, ix, t)
+  FOR_EACH_LOCAL_DECL (DECL_STRUCT_FUNCTION (decl), ix, t)
     TREE_USED (t) = 1;
 
   /* Clear TREE_USED on all variables associated with a block scope.  */
-  clear_tree_used (DECL_INITIAL (current_function_decl));
+  clear_tree_used (DECL_INITIAL (decl));
 
   /* Initialize local stack smashing state.  */
   has_protected_decls = false;
@@ -1354,6 +1377,8 @@  HOST_WIDE_INT
 estimated_stack_frame_size (struct cgraph_node *node)
 {
   HOST_WIDE_INT size = 0;
+  HOST_WIDE_INT delta;
+  unsigned ix;
   size_t i;
   tree var;
   referenced_var_iterator rvi;
@@ -1377,6 +1402,34 @@  estimated_stack_frame_size (struct cgrap
       fini_vars_expansion ();
     }
 
+  delta = size;
+
+  init_vars_expansion (node->decl);
+
+  FOR_EACH_LOCAL_DECL (fn, ix, var)
+    {
+      /* TREE_USED marks local variables that do not appear in lexical
+	 blocks.  We don't want to expand those that do twice.  */
+      if (TREE_USED (var))
+        delta -= expand_one_var (var, true, false);
+    }
+  delta -= account_used_vars_for_block (DECL_INITIAL (node->decl), true);
+
+  if (stack_vars_num > 0)
+    {
+      /* Fake sorting the stack vars for account_stack_vars ().  */
+      stack_vars_sorted = XNEWVEC (size_t, stack_vars_num);
+      for (i = 0; i < stack_vars_num; ++i)
+	stack_vars_sorted[i] = i;
+      delta -= account_stack_vars ();
+      fini_vars_expansion ();
+    }
+
+  if (delta)
+    inform (input_location,
+	    "guestimated stack size for %q+D changed by %li to %li",
+	    node->decl, (long)delta, (long)size);
+
   current_function_decl = save;
   pop_cfun ();
 
@@ -1400,7 +1453,7 @@  expand_used_vars (void)
     frame_phase = off ? align - off : 0;
   }
 
-  init_vars_expansion ();
+  init_vars_expansion (current_function_decl);
 
   for (i = 0; i < SA.map->num_partitions; i++)
     {