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

login
register
mail settings
Submitter Alexandre Oliva
Date Feb. 3, 2011, 5:50 a.m.
Message ID <ory65xu0nd.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/81619/
State New
Headers show

Comments

Alexandre Oliva - Feb. 3, 2011, 5:50 a.m.
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.

Something very close to this patch (I'd accidentally left
init_vars_expansion() out) regstrapped without regressions on
x86_64-linux-gnu and i686-pc-linux-gnu.  I'm now retesting this.  Ok if
it passes?
Richard Guenther - Feb. 3, 2011, 10:19 a.m.
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.  It doesn't make much sense at its current
position.  Honza?

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

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

Thanks,
Richard.

> Something very close to this patch (I'd accidentally left
> init_vars_expansion() out) regstrapped without regressions on
> x86_64-linux-gnu and i686-pc-linux-gnu.  I'm now retesting this.  Ok if
> it passes?
>
>
>
> --
> 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
>
>
Jan Hubicka - Feb. 3, 2011, 1:24 p.m.
> 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.  It doesn't make much sense at its current
> position.  Honza?

Yes, it is there because we used to have early early inlining for profling.
I would suggest to move the first pass just before early inlining.  

I hope early inliner won't be confused by seeing functions with parameters
not computed yet, but those are not inlinable anyway since they are not yet
in SSA form.

Honza
> 
> 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).
> 
> Can you do a quick comparison between the old and the new
> stack frame estimations on some random files from gcc?
> 
> Thanks,
> Richard.
> 
> > Something very close to this patch (I'd accidentally left
> > init_vars_expansion() out) regstrapped without regressions on
> > x86_64-linux-gnu and i686-pc-linux-gnu.  I'm now retesting this.  Ok if
> > it passes?
> >
> >
> >
> > --
> > 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

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-02 19:58:10.013374804 -0200
+++ gcc/cfgexpand.c	2011-02-02 20:21:51.148296315 -0200
@@ -1325,8 +1325,7 @@  account_used_vars_for_block (tree block,
 
   /* 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);
+    size += expand_one_var (t, toplevel, false);
 
   /* Expand all variables at containing levels.  */
   for (t = BLOCK_SUBBLOCKS (block); t ; t = BLOCK_CHAIN (t))
@@ -1381,20 +1380,38 @@  estimated_stack_frame_size (tree decl)
   size_t i;
   tree var, outer_block = DECL_INITIAL (current_function_decl);
   unsigned ix;
+  referenced_var_iterator rvi;
   tree old_cur_fun_decl = current_function_decl;
+
   current_function_decl = decl;
   push_cfun (DECL_STRUCT_FUNCTION (decl));
 
-  init_vars_expansion ();
-
-  FOR_EACH_LOCAL_DECL (cfun, ix, var)
+  /* We want to count only referenced vars, but if asked to estimate
+     stack size before we compute them, guess based on local decls and
+     scopes.  We have to make sure we compute the same values for
+     debug and non-debug compilations, in spite of the removal of
+     unused variables from lexical blocks, but this removal never
+     occurs before referenced vars are computed.  Even when we perform
+     inlining and versioning, we register referenced vars, and can
+     thus use them for further stack size estimation.  */
+  if (gimple_in_ssa_p (cfun))
+    {
+      gcc_checking_assert (gimple_referenced_vars (cfun));
+      FOR_EACH_REFERENCED_VAR (var, rvi)
+	size += expand_one_var (var, true, false);
+    }
+  else
     {
-      /* 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);
+      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);
     }
-  size += account_used_vars_for_block (outer_block, true);
 
   if (stack_vars_num > 0)
     {
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h.orig	2011-02-02 17:31:40.430302181 -0200
+++ gcc/tree-flow.h	2011-02-02 20:05:17.169414197 -0200
@@ -319,6 +319,7 @@  typedef struct
        !end_referenced_vars_p (&(ITER)); \
        (VAR) = next_referenced_var (&(ITER)))
 
+extern tree referenced_var_lookup_in (htab_t, unsigned int);
 extern tree referenced_var_lookup (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-02 17:31:41.279276897 -0200
+++ gcc/tree-inline.c	2011-02-03 00:08:30.100962123 -0200
@@ -317,7 +317,12 @@  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_in (gimple_referenced_vars
+					   (DECL_STRUCT_FUNCTION (id->src_fn)),
+					   DECL_UID (decl)))
+	    add_referenced_var (t);
 	}
       return t;
     }
@@ -4753,6 +4758,14 @@  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_in (gimple_referenced_vars
+				   (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-02 16:21:12.934940723 -0200
+++ gcc/tree-dfa.c	2011-02-02 20:05:17.249411771 -0200
@@ -490,10 +490,18 @@  find_referenced_vars_in (gimple stmt)
 tree
 referenced_var_lookup (unsigned int uid)
 {
+  return referenced_var_lookup_in (gimple_referenced_vars (cfun), uid);
+}
+
+/* Lookup UID in REFVARS and return the associated variable.  */
+
+tree
+referenced_var_lookup_in (htab_t refvars, 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 (refvars, &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-02 20:18:16.826787838 -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 ());
+}