This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PR debug/47106] account used vars only once


On Feb  3, 2011, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Thu, Feb 3, 2011 at 6:50 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Feb Â2, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
>> 
>>> Perhaps relying on the used flag and going over all scope blocks wasn't
>>> such a good idea, after all; I suppose we could get something more
>>> reliable using referenced_vars only. ÂAlthough I'm finding it difficult
>>> to figure out whether presence in referenced_vars should ever be
>>> different from having the used flag marked, it appears that presence in
>>> referenced_vars is maintained more accurately, even during inlining.
>> 
>> Indeed, early in compilation we don't have referenced_vars at all, but
>> we can make do without them, going through the scope blocks.

> I think we can simply move pass_inline_parameters to after
> pass_referenced_vars.

That isn't enough.  We often attempt to estimate the stack size of a
function before we as much as put it in SSA form (let alone compute
referenced_vras), while processing other functions.

> referenced_var_lookup_in should rather take a struct function argument
> than a hashtable (in fact, given the few existing callers I'd just change
> the referenced_var_lookup signature).

*nod*

> Can you do a quick comparison between the old and the new
> stack frame estimations on some random files from gcc?

I guess...  Once we have another working patch.  My previous patch did
away with the need for rearranging passes by using the current
computation when referenced_vars are not available, but if we switch to
referenced_vars only, we have to figure out how to rearrange the passes
so that we make reasonable estimates.

Here's where I stand ATM.  The first patch is supposed to implement the
changes you suggested, plus some attempts to fix things up or otherwise
detect problems.  It finds too many.  The second patch restores some of
the removed functionality to error out if the stack size computations
differ.  None of these are meant for inclusion, I post them for the
record in case any of you guys would like to pick it up while I get some
sleep.  Suggestions on how to proceed are welcome.  I'm thinking of
rearranging the passes so we compute referenced_vars for all functions
before advancing to other passes for any other functions, but I have no
idea of how to do that.  I'll dig it up if nobody beats me to it.

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/47106
	PR debug/47402
	* cfgexpand.c (account_used_vars_for_block): Disregard used flag.
	(estimated_stack_frame_size): Prefer referenced vars over scope
	block vars.
	* tree-flow.h (referenced_var_lookup_in): Declare.
	* tree-inline.c (remap_decl): Only mark VAR_DECLs as referenced
	that were referenced in the original function.
	(copy_decl_for_dup_finish): Likewise.
	* tree-dfa.c (referenced_var_lookup_in): Split out of...
	(referenced_var_lookup): ... this.

for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/47106
	PR debug/47402
	* g++.dg/debug/pr47106.C: New.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c.orig	2011-02-04 05:33:51.822915638 -0200
+++ gcc/cfgexpand.c	2011-02-04 05:34:59.067593689 -0200
@@ -520,7 +520,7 @@ update_alias_info_with_stack_vars (void)
 	     for -O0 where we are preserving even unreferenced variables.  */
 	  gcc_assert (DECL_P (decl)
 		      && (!optimize
-			  || referenced_var_lookup (DECL_UID (decl))));
+			  || referenced_var_lookup (cfun, DECL_UID (decl))));
 	  bitmap_set_bit (part, uid);
 	  *((bitmap *) pointer_map_insert (decls_to_partitions,
 					   (void *)(size_t) uid)) = part;
@@ -1311,30 +1311,6 @@ 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))
-    if (var_ann (t) && is_used_p (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)
@@ -1379,22 +1355,19 @@ estimated_stack_frame_size (tree decl)
 {
   HOST_WIDE_INT size = 0;
   size_t i;
-  tree var, outer_block = DECL_INITIAL (current_function_decl);
-  unsigned ix;
+  tree var;
   tree old_cur_fun_decl = current_function_decl;
+  referenced_var_iterator rvi;
+
+  if (!gimple_referenced_vars (cfun))
+    return -1;
+
   current_function_decl = decl;
   push_cfun (DECL_STRUCT_FUNCTION (decl));
 
-  init_vars_expansion ();
-
-  FOR_EACH_LOCAL_DECL (cfun, 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))
-        size += expand_one_var (var, true, false);
-    }
-  size += account_used_vars_for_block (outer_block, true);
+  gcc_checking_assert (gimple_referenced_vars (cfun));
+  FOR_EACH_REFERENCED_VAR (var, rvi)
+    size += expand_one_var (var, true, false);
 
   if (stack_vars_num > 0)
     {
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h.orig	2011-02-04 05:33:52.413868892 -0200
+++ gcc/tree-flow.h	2011-02-04 05:34:59.082592500 -0200
@@ -319,7 +319,7 @@ typedef struct
        !end_referenced_vars_p (&(ITER)); \
        (VAR) = next_referenced_var (&(ITER)))
 
-extern tree referenced_var_lookup (unsigned int);
+extern tree referenced_var_lookup (struct function *, unsigned int);
 extern bool referenced_var_check_and_insert (tree);
 #define num_referenced_vars htab_elements (gimple_referenced_vars (cfun))
 
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c.orig	2011-02-04 05:33:52.276879729 -0200
+++ gcc/tree-inline.c	2011-02-04 05:34:59.121589412 -0200
@@ -317,7 +317,11 @@ remap_decl (tree decl, copy_body_data *i
 	      || TREE_CODE (t) == RESULT_DECL || TREE_CODE (t) == PARM_DECL))
 	{
 	  get_var_ann (t);
-	  add_referenced_var (t);
+	  if (TREE_CODE (decl) != VAR_DECL
+	      || !gimple_referenced_vars (DECL_STRUCT_FUNCTION (id->src_fn))
+	      || referenced_var_lookup (DECL_STRUCT_FUNCTION (id->src_fn),
+					DECL_UID (decl)))
+	    add_referenced_var (t);
 	}
       return t;
     }
@@ -4753,6 +4757,13 @@ copy_decl_for_dup_finish (copy_body_data
        new function.  */
     DECL_CONTEXT (copy) = id->dst_fn;
 
+  if (TREE_CODE (decl) == VAR_DECL
+      && gimple_referenced_vars (cfun)
+      && gimple_referenced_vars (DECL_STRUCT_FUNCTION (id->src_fn))
+      && referenced_var_lookup (DECL_STRUCT_FUNCTION (id->src_fn),
+				DECL_UID (decl)))
+    add_referenced_var (copy);
+
   return copy;
 }
 
Index: gcc/tree-dfa.c
===================================================================
--- gcc/tree-dfa.c.orig	2011-02-04 05:33:52.126891590 -0200
+++ gcc/tree-dfa.c	2011-02-04 05:34:59.139587983 -0200
@@ -488,12 +488,12 @@ find_referenced_vars_in (gimple stmt)
    variable.  */
 
 tree
-referenced_var_lookup (unsigned int uid)
+referenced_var_lookup (struct function *fn, unsigned int uid)
 {
   tree h;
   struct tree_decl_minimal in;
   in.uid = uid;
-  h = (tree) htab_find_with_hash (gimple_referenced_vars (cfun), &in, uid);
+  h = (tree) htab_find_with_hash (gimple_referenced_vars (fn), &in, uid);
   return h;
 }
 
Index: gcc/testsuite/g++.dg/debug/pr47106.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/g++.dg/debug/pr47106.C	2011-02-04 05:34:59.190583952 -0200
@@ -0,0 +1,37 @@
+// { dg-do compile }
+// { dg-options "-O -fpartial-inlining -flto -fconserve-stack -fcompare-debug" }
+
+void end (int, int) __attribute__ ((__noreturn__));
+
+struct S
+{
+  int i;
+  S *s;
+};
+
+inline bool f (S *s)
+{
+  if (!s->s)
+    end (0, 0);
+  return s->s == s;
+}
+
+inline bool
+baz (S s1, S)
+{
+  while (f (&s1));
+}
+
+inline bool
+bar (S s1, S s2, S)
+{
+  baz (s1, s2);
+}
+
+S getS ();
+
+bool
+foo ()
+{
+  bar (getS (), getS (), getS ());
+}
Index: gcc/gimple-pretty-print.c
===================================================================
--- gcc/gimple-pretty-print.c.orig	2011-02-04 05:33:51.900909541 -0200
+++ gcc/gimple-pretty-print.c	2011-02-04 05:34:59.218581733 -0200
@@ -542,7 +542,7 @@ pp_points_to_solution (pretty_printer *b
       pp_string (buffer, "{ ");
       EXECUTE_IF_SET_IN_BITMAP (pt->vars, 0, i, bi)
 	{
-	  tree var = referenced_var_lookup (i);
+	  tree var = referenced_var_lookup (cfun, i);
 	  if (var)
 	    {
 	      dump_generic_node (buffer, var, 0, dump_flags, false);
Index: gcc/passes.c
===================================================================
--- gcc/passes.c.orig	2011-02-04 05:33:52.192886360 -0200
+++ gcc/passes.c	2011-02-04 05:34:59.244579675 -0200
@@ -729,7 +729,6 @@ init_optimization_passes (void)
   NEXT_PASS (pass_build_cfg);
   NEXT_PASS (pass_warn_function_return);
   NEXT_PASS (pass_build_cgraph_edges);
-  NEXT_PASS (pass_inline_parameters);
   *p = NULL;
 
   /* Interprocedural optimization passes.  */
@@ -753,6 +752,7 @@ init_optimization_passes (void)
 	 inline functions to be inlined even at -O0.  This does not
 	 happen during the first early inline pass.  */
       NEXT_PASS (pass_rebuild_cgraph_edges);
+      NEXT_PASS (pass_inline_parameters);
       NEXT_PASS (pass_early_inline);
       NEXT_PASS (pass_all_early_optimizations);
 	{
Index: gcc/tree-flow-inline.h
===================================================================
--- gcc/tree-flow-inline.h.orig	2011-02-04 05:33:51.976903462 -0200
+++ gcc/tree-flow-inline.h	2011-02-04 05:34:59.256578726 -0200
@@ -103,7 +103,7 @@ next_htab_element (htab_iterator *hti)
 static inline tree
 referenced_var (unsigned int uid)
 {
-  tree var = referenced_var_lookup (uid);
+  tree var = referenced_var_lookup (cfun, uid);
   gcc_assert (var || uid == 0);
   return var;
 }
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c.orig	2011-02-04 05:33:52.052897449 -0200
+++ gcc/tree-into-ssa.c	2011-02-04 05:34:59.280576826 -0200
@@ -1469,7 +1469,7 @@ dump_decl_set (FILE *file, bitmap set)
 
       EXECUTE_IF_SET_IN_BITMAP (set, 0, i, bi)
 	{
-	  tree var = referenced_var_lookup (i);
+	  tree var = referenced_var_lookup (cfun, i);
 	  if (var)
 	    print_generic_expr (file, var, 0);
 	  else
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2011-02-04 05:33:52.345874268 -0200
+++ gcc/tree-ssa.c	2011-02-04 05:34:59.298575401 -0200
@@ -1902,7 +1902,7 @@ maybe_optimize_var (tree var, bitmap add
 
   /* If the variable is not in the list of referenced vars then we
      do not need to touch it nor can we rename it.  */
-  if (!referenced_var_lookup (DECL_UID (var)))
+  if (!referenced_var_lookup (cfun, DECL_UID (var)))
     return false;
 
   if (TREE_ADDRESSABLE (var)
Index: gcc/ipa-inline.c
===================================================================
--- gcc/ipa-inline.c.orig	2011-02-04 05:35:01.984362779 -0200
+++ gcc/ipa-inline.c	2011-02-04 05:35:07.164952668 -0200
@@ -275,11 +275,14 @@ cgraph_clone_inlined_nodes (struct cgrap
     e->callee->global.inlined_to = e->caller->global.inlined_to;
   else
     e->callee->global.inlined_to = e->caller;
+  gcc_checking_assert (inline_summary (e->caller)->estimated_self_stack_size != -1);
   e->callee->global.stack_frame_offset
     = e->caller->global.stack_frame_offset
       + inline_summary (e->caller)->estimated_self_stack_size;
+  gcc_checking_assert (inline_summary (e->callee)->estimated_self_stack_size != -1)
   peak = e->callee->global.stack_frame_offset
       + inline_summary (e->callee)->estimated_self_stack_size;
+  gcc_checking_assert (e->callee->global.inlined_to->global.estimated_stack_size != -1);
   if (e->callee->global.inlined_to->global.estimated_stack_size < peak)
     e->callee->global.inlined_to->global.estimated_stack_size = peak;
   cgraph_propagate_frequency (e->callee);
@@ -419,9 +422,11 @@ cgraph_check_inline_limits (struct cgrap
     }
 
   stack_size_limit = inline_summary (to)->estimated_self_stack_size;
+  gcc_checking_assert (stack_size_limit != -1);
 
   stack_size_limit += stack_size_limit * PARAM_VALUE (PARAM_STACK_FRAME_GROWTH) / 100;
 
+  gcc_checking_assert (what->global.estimated_stack_size != -1);
   inlined_stack = (to->global.stack_frame_offset
 		   + inline_summary (to)->estimated_self_stack_size
 		   + what->global.estimated_stack_size);
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c.orig	2011-02-04 05:26:40.137008650 -0200
+++ gcc/cfgexpand.c	2011-02-04 05:27:40.951213575 -0200
@@ -1311,6 +1311,29 @@ 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)
@@ -1354,7 +1377,10 @@ HOST_WIDE_INT
 estimated_stack_frame_size (tree decl)
 {
   HOST_WIDE_INT size = 0;
+  HOST_WIDE_INT osize = 0;
+  unsigned ix;
   size_t i;
+  tree outer_block = DECL_INITIAL (current_function_decl);
   tree var;
   tree old_cur_fun_decl = current_function_decl;
   referenced_var_iterator rvi;
@@ -1378,6 +1404,11 @@ estimated_stack_frame_size (tree decl)
       size += account_stack_vars ();
       fini_vars_expansion ();
     }
+
+  if (size != osize)
+    error ("est stack size changed from %li to %li",
+	   (long)osize, (long)size);
+
   pop_cfun ();
   current_function_decl = old_cur_fun_decl;
   return size;

-- 
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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]