diff mbox

[PR,43905] Make IPA-SRA clone functions it modifies

Message ID 20100615101256.GA12135@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor June 15, 2010, 10:12 a.m. UTC 
Hi,

On Thu, Jun 10, 2010 at 11:40:06PM +0200, Martin Jambor wrote:
> Hi,
> 
> the patch below is basically a different fix to PR 43141 which fixes
> the still-opened PR 43905 and a number of issues that that first
> attempt caused (e.g. wrong sharing of parameter chain list and some
> debug info problems).  After trying quite hard to move the body of a
> function from the old declaration to a new one and all necessary
> fixing up so that nothing breaks, I gave up and resorted to simple
> cloning.
> 
> Fortunately the pass management works in a way that setting the cfun
> and current_function_decl to the new function is enough to make the
> subsequent passes work on the correct function.  It is a bit of a
> hack, but I can't think of any substantial improvement.
> 

The previous patch did not test whether the current function was
versionable which lead to segfaults when I supplied a noclone
attribute to some simple testcases.  This is an updated patch that
does perform the necessary check (which unfortunately requires an
extra include), otherwise it is exactly the same.

Again, bootstrapped and tested on x86_64-linux without any issues.  OK
for trunk and later for 4.5 as well?  

Honza, it seems everybody is waiting for your assessment :-)

Thanks,

Martin


2010-06-14  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/43905
	* tree-sra.c: Include tree-inline.h.
	(create_abstract_origin): Removed.
	(modify_function): Version the call graph node instead of creating
	abstract origins and dealing with same_body aliases.
	* tree-sra.c (ipa_sra_preliminary_function_checks): Check whether the
	function is versionable.
	* Makefile.in (tree-sra.o): Add TREE_INLINE_H to dependencies.

	* testsuite/g++.dg/torture/pr43905.C

Comments

Jan Hubicka June 15, 2010, 4:17 p.m. UTC | #1 
> 
> The previous patch did not test whether the current function was
> versionable which lead to segfaults when I supplied a noclone
> attribute to some simple testcases.  This is an updated patch that
> does perform the necessary check (which unfortunately requires an
> extra include), otherwise it is exactly the same.
> 
> Again, bootstrapped and tested on x86_64-linux without any issues.  OK
> for trunk and later for 4.5 as well?  
> 
> Honza, it seems everybody is waiting for your assessment :-)

Sorry, i had talk today and thus didn't read mails much.  So my assesment is
needed WRT passmanager hack of setting cfun/current_function_decl?  Well, it is
sort of hack.  I think in longer term we will want to record state local passes
are in to be able to create function at specific place in queue, but it can be
done incrementally and thus patch makes is no harder, so I think it is OK.

Obviously the verionable_function_p check is not really needed as we are not
really going to version (we are killing the original), but same issues
are at other place (including my ipa-split pass), so this is something we could
improve later and it is not important at all in practice anyway.

honza
> 
> Thanks,
> 
> Martin
> 
> 
> 2010-06-14  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/43905
> 	* tree-sra.c: Include tree-inline.h.
> 	(create_abstract_origin): Removed.
> 	(modify_function): Version the call graph node instead of creating
> 	abstract origins and dealing with same_body aliases.
> 	* tree-sra.c (ipa_sra_preliminary_function_checks): Check whether the
> 	function is versionable.
> 	* Makefile.in (tree-sra.o): Add TREE_INLINE_H to dependencies.
> 
> 	* testsuite/g++.dg/torture/pr43905.C
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -89,6 +89,7 @@ along with GCC; see the file COPYING3.
>  #include "target.h"
>  #include "flags.h"
>  #include "dbgcnt.h"
> +#include "tree-inline.h"
>  
>  /* Enumeration of all aggregate reductions we can do.  */
>  enum sra_mode { SRA_MODE_EARLY_IPA,   /* early call regularization */
> @@ -4224,43 +4225,38 @@ convert_callers (struct cgraph_node *nod
>    return;
>  }
>  
> -/* Create an abstract origin declaration for OLD_DECL and make it an abstract
> -   origin of the provided decl so that there are preserved parameters for debug
> -   information.  */
> -
> -static void
> -create_abstract_origin (tree old_decl)
> -{
> -  if (!DECL_ABSTRACT_ORIGIN (old_decl))
> -    {
> -      tree new_decl = copy_node (old_decl);
> -
> -      DECL_ABSTRACT (new_decl) = 1;
> -      SET_DECL_ASSEMBLER_NAME (new_decl, NULL_TREE);
> -      SET_DECL_RTL (new_decl, NULL);
> -      DECL_STRUCT_FUNCTION (new_decl) = NULL;
> -      DECL_ARTIFICIAL (old_decl) = 1;
> -      DECL_ABSTRACT_ORIGIN (old_decl) = new_decl;
> -    }
> -}
> -
>  /* Perform all the modification required in IPA-SRA for NODE to have parameters
>     as given in ADJUSTMENTS.  */
>  
>  static void
>  modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
>  {
> -  struct cgraph_node *alias;
> -  for (alias = node->same_body; alias; alias = alias->next)
> -    ipa_modify_formal_parameters (alias->decl, adjustments, "ISRA");
> -  /* current_function_decl must be handled last, after same_body aliases,
> -     as following functions will use what it computed.  */
> -  create_abstract_origin (current_function_decl);
> +  struct cgraph_node *new_node;
> +  struct cgraph_edge *cs;
> +  VEC (cgraph_edge_p, heap) * redirect_callers;
> +  int node_callers;
> +
> +  node_callers = 0;
> +  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
> +    node_callers++;
> +  redirect_callers = VEC_alloc (cgraph_edge_p, heap, node_callers);
> +  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
> +    VEC_quick_push (cgraph_edge_p, redirect_callers, cs);
> +
> +  rebuild_cgraph_edges ();
> +  pop_cfun ();
> +  current_function_decl = NULL_TREE;
> +
> +  new_node = cgraph_function_versioning (node, redirect_callers, NULL, NULL,
> +					 NULL, NULL, "isra");
> +  current_function_decl = new_node->decl;
> +  push_cfun (DECL_STRUCT_FUNCTION (new_node->decl));
> +
>    ipa_modify_formal_parameters (current_function_decl, adjustments, "ISRA");
>    ipa_sra_modify_function_body (adjustments);
>    sra_ipa_reset_debug_stmts (adjustments);
> -  convert_callers (node, adjustments);
> -  cgraph_make_node_local (node);
> +  convert_callers (new_node, adjustments);
> +  cgraph_make_node_local (new_node);
>    return;
>  }
>  
> @@ -4275,6 +4271,13 @@ ipa_sra_preliminary_function_checks (str
>      {
>        if (dump_file)
>  	fprintf (dump_file, "Function not local to this compilation unit.\n");
> +      return false;
> +    }
> +
> +  if (!tree_versionable_function_p (node->decl))
> +    {
> +      if (dump_file)
> +	fprintf (dump_file, "Function not local to this compilation unit.\n");
>        return false;
>      }
>  
> Index: mine/gcc/testsuite/g++.dg/torture/pr43905.C
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/g++.dg/torture/pr43905.C
> @@ -0,0 +1,13 @@
> +extern void sf ( __const char *);
> +struct Matrix{
> +  int operator[](int n){
> +    sf ( __PRETTY_FUNCTION__);
> +  }
> +  int operator[](int n)const{
> +    sf ( __PRETTY_FUNCTION__);
> +  }
> +};
> +void calcmy(Matrix const &b, Matrix &c, int k){
> +  b[k];
> +  c[k];
> +}
> Index: mine/gcc/Makefile.in
> ===================================================================
> --- mine.orig/gcc/Makefile.in
> +++ mine/gcc/Makefile.in
> @@ -3118,7 +3118,8 @@ tree-ssa-ccp.o : tree-ssa-ccp.c $(TREE_F
>  tree-sra.o : tree-sra.c $(CONFIG_H) $(SYSTEM_H) coretypes.h alloc-pool.h \
>     $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) $(IPA_PROP_H) \
>     $(DIAGNOSTIC_H) statistics.h $(TREE_DUMP_H) $(TIMEVAR_H) $(PARAMS_H) \
> -   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H)
> +   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H) \
> +   $(TREE_INLINE_H)
>  tree-switch-conversion.o : tree-switch-conversion.c $(CONFIG_H) $(SYSTEM_H) \
>      $(TREE_H) $(TM_P_H) $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TREE_INLINE_H) \
>      $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \
Richard Biener June 16, 2010, 8:09 a.m. UTC | #2 
On Tue, 15 Jun 2010, Jan Hubicka wrote:

> > 
> > The previous patch did not test whether the current function was
> > versionable which lead to segfaults when I supplied a noclone
> > attribute to some simple testcases.  This is an updated patch that
> > does perform the necessary check (which unfortunately requires an
> > extra include), otherwise it is exactly the same.
> > 
> > Again, bootstrapped and tested on x86_64-linux without any issues.  OK
> > for trunk and later for 4.5 as well?  
> > 
> > Honza, it seems everybody is waiting for your assessment :-)
> 
> Sorry, i had talk today and thus didn't read mails much.  So my assesment is
> needed WRT passmanager hack of setting cfun/current_function_decl?  Well, it is
> sort of hack.  I think in longer term we will want to record state local passes
> are in to be able to create function at specific place in queue, but it can be
> done incrementally and thus patch makes is no harder, so I think it is OK.
> 
> Obviously the verionable_function_p check is not really needed as we are not
> really going to version (we are killing the original), but same issues
> are at other place (including my ipa-split pass), so this is something we could
> improve later and it is not important at all in practice anyway.

The rest of the patch is ok then.  Please give it some time on trunk
before backporting.

Thanks,
Richard.

> honza
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2010-06-14  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR tree-optimization/43905
> > 	* tree-sra.c: Include tree-inline.h.
> > 	(create_abstract_origin): Removed.
> > 	(modify_function): Version the call graph node instead of creating
> > 	abstract origins and dealing with same_body aliases.
> > 	* tree-sra.c (ipa_sra_preliminary_function_checks): Check whether the
> > 	function is versionable.
> > 	* Makefile.in (tree-sra.o): Add TREE_INLINE_H to dependencies.
> > 
> > 	* testsuite/g++.dg/torture/pr43905.C
> > 
> > Index: mine/gcc/tree-sra.c
> > ===================================================================
> > --- mine.orig/gcc/tree-sra.c
> > +++ mine/gcc/tree-sra.c
> > @@ -89,6 +89,7 @@ along with GCC; see the file COPYING3.
> >  #include "target.h"
> >  #include "flags.h"
> >  #include "dbgcnt.h"
> > +#include "tree-inline.h"
> >  
> >  /* Enumeration of all aggregate reductions we can do.  */
> >  enum sra_mode { SRA_MODE_EARLY_IPA,   /* early call regularization */
> > @@ -4224,43 +4225,38 @@ convert_callers (struct cgraph_node *nod
> >    return;
> >  }
> >  
> > -/* Create an abstract origin declaration for OLD_DECL and make it an abstract
> > -   origin of the provided decl so that there are preserved parameters for debug
> > -   information.  */
> > -
> > -static void
> > -create_abstract_origin (tree old_decl)
> > -{
> > -  if (!DECL_ABSTRACT_ORIGIN (old_decl))
> > -    {
> > -      tree new_decl = copy_node (old_decl);
> > -
> > -      DECL_ABSTRACT (new_decl) = 1;
> > -      SET_DECL_ASSEMBLER_NAME (new_decl, NULL_TREE);
> > -      SET_DECL_RTL (new_decl, NULL);
> > -      DECL_STRUCT_FUNCTION (new_decl) = NULL;
> > -      DECL_ARTIFICIAL (old_decl) = 1;
> > -      DECL_ABSTRACT_ORIGIN (old_decl) = new_decl;
> > -    }
> > -}
> > -
> >  /* Perform all the modification required in IPA-SRA for NODE to have parameters
> >     as given in ADJUSTMENTS.  */
> >  
> >  static void
> >  modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
> >  {
> > -  struct cgraph_node *alias;
> > -  for (alias = node->same_body; alias; alias = alias->next)
> > -    ipa_modify_formal_parameters (alias->decl, adjustments, "ISRA");
> > -  /* current_function_decl must be handled last, after same_body aliases,
> > -     as following functions will use what it computed.  */
> > -  create_abstract_origin (current_function_decl);
> > +  struct cgraph_node *new_node;
> > +  struct cgraph_edge *cs;
> > +  VEC (cgraph_edge_p, heap) * redirect_callers;
> > +  int node_callers;
> > +
> > +  node_callers = 0;
> > +  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
> > +    node_callers++;
> > +  redirect_callers = VEC_alloc (cgraph_edge_p, heap, node_callers);
> > +  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
> > +    VEC_quick_push (cgraph_edge_p, redirect_callers, cs);
> > +
> > +  rebuild_cgraph_edges ();
> > +  pop_cfun ();
> > +  current_function_decl = NULL_TREE;
> > +
> > +  new_node = cgraph_function_versioning (node, redirect_callers, NULL, NULL,
> > +					 NULL, NULL, "isra");
> > +  current_function_decl = new_node->decl;
> > +  push_cfun (DECL_STRUCT_FUNCTION (new_node->decl));
> > +
> >    ipa_modify_formal_parameters (current_function_decl, adjustments, "ISRA");
> >    ipa_sra_modify_function_body (adjustments);
> >    sra_ipa_reset_debug_stmts (adjustments);
> > -  convert_callers (node, adjustments);
> > -  cgraph_make_node_local (node);
> > +  convert_callers (new_node, adjustments);
> > +  cgraph_make_node_local (new_node);
> >    return;
> >  }
> >  
> > @@ -4275,6 +4271,13 @@ ipa_sra_preliminary_function_checks (str
> >      {
> >        if (dump_file)
> >  	fprintf (dump_file, "Function not local to this compilation unit.\n");
> > +      return false;
> > +    }
> > +
> > +  if (!tree_versionable_function_p (node->decl))
> > +    {
> > +      if (dump_file)
> > +	fprintf (dump_file, "Function not local to this compilation unit.\n");
> >        return false;
> >      }
> >  
> > Index: mine/gcc/testsuite/g++.dg/torture/pr43905.C
> > ===================================================================
> > --- /dev/null
> > +++ mine/gcc/testsuite/g++.dg/torture/pr43905.C
> > @@ -0,0 +1,13 @@
> > +extern void sf ( __const char *);
> > +struct Matrix{
> > +  int operator[](int n){
> > +    sf ( __PRETTY_FUNCTION__);
> > +  }
> > +  int operator[](int n)const{
> > +    sf ( __PRETTY_FUNCTION__);
> > +  }
> > +};
> > +void calcmy(Matrix const &b, Matrix &c, int k){
> > +  b[k];
> > +  c[k];
> > +}
> > Index: mine/gcc/Makefile.in
> > ===================================================================
> > --- mine.orig/gcc/Makefile.in
> > +++ mine/gcc/Makefile.in
> > @@ -3118,7 +3118,8 @@ tree-ssa-ccp.o : tree-ssa-ccp.c $(TREE_F
> >  tree-sra.o : tree-sra.c $(CONFIG_H) $(SYSTEM_H) coretypes.h alloc-pool.h \
> >     $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) $(IPA_PROP_H) \
> >     $(DIAGNOSTIC_H) statistics.h $(TREE_DUMP_H) $(TIMEVAR_H) $(PARAMS_H) \
> > -   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H)
> > +   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H) \
> > +   $(TREE_INLINE_H)
> >  tree-switch-conversion.o : tree-switch-conversion.c $(CONFIG_H) $(SYSTEM_H) \
> >      $(TREE_H) $(TM_P_H) $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TREE_INLINE_H) \
> >      $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \
> 
>
diff mbox

Patch

Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -89,6 +89,7 @@  along with GCC; see the file COPYING3.
 #include "target.h"
 #include "flags.h"
 #include "dbgcnt.h"
+#include "tree-inline.h"
 
 /* Enumeration of all aggregate reductions we can do.  */
 enum sra_mode { SRA_MODE_EARLY_IPA,   /* early call regularization */
@@ -4224,43 +4225,38 @@  convert_callers (struct cgraph_node *nod
   return;
 }
 
-/* Create an abstract origin declaration for OLD_DECL and make it an abstract
-   origin of the provided decl so that there are preserved parameters for debug
-   information.  */
-
-static void
-create_abstract_origin (tree old_decl)
-{
-  if (!DECL_ABSTRACT_ORIGIN (old_decl))
-    {
-      tree new_decl = copy_node (old_decl);
-
-      DECL_ABSTRACT (new_decl) = 1;
-      SET_DECL_ASSEMBLER_NAME (new_decl, NULL_TREE);
-      SET_DECL_RTL (new_decl, NULL);
-      DECL_STRUCT_FUNCTION (new_decl) = NULL;
-      DECL_ARTIFICIAL (old_decl) = 1;
-      DECL_ABSTRACT_ORIGIN (old_decl) = new_decl;
-    }
-}
-
 /* Perform all the modification required in IPA-SRA for NODE to have parameters
    as given in ADJUSTMENTS.  */
 
 static void
 modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
 {
-  struct cgraph_node *alias;
-  for (alias = node->same_body; alias; alias = alias->next)
-    ipa_modify_formal_parameters (alias->decl, adjustments, "ISRA");
-  /* current_function_decl must be handled last, after same_body aliases,
-     as following functions will use what it computed.  */
-  create_abstract_origin (current_function_decl);
+  struct cgraph_node *new_node;
+  struct cgraph_edge *cs;
+  VEC (cgraph_edge_p, heap) * redirect_callers;
+  int node_callers;
+
+  node_callers = 0;
+  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
+    node_callers++;
+  redirect_callers = VEC_alloc (cgraph_edge_p, heap, node_callers);
+  for (cs = node->callers; cs != NULL; cs = cs->next_caller)
+    VEC_quick_push (cgraph_edge_p, redirect_callers, cs);
+
+  rebuild_cgraph_edges ();
+  pop_cfun ();
+  current_function_decl = NULL_TREE;
+
+  new_node = cgraph_function_versioning (node, redirect_callers, NULL, NULL,
+					 NULL, NULL, "isra");
+  current_function_decl = new_node->decl;
+  push_cfun (DECL_STRUCT_FUNCTION (new_node->decl));
+
   ipa_modify_formal_parameters (current_function_decl, adjustments, "ISRA");
   ipa_sra_modify_function_body (adjustments);
   sra_ipa_reset_debug_stmts (adjustments);
-  convert_callers (node, adjustments);
-  cgraph_make_node_local (node);
+  convert_callers (new_node, adjustments);
+  cgraph_make_node_local (new_node);
   return;
 }
 
@@ -4275,6 +4271,13 @@  ipa_sra_preliminary_function_checks (str
     {
       if (dump_file)
 	fprintf (dump_file, "Function not local to this compilation unit.\n");
+      return false;
+    }
+
+  if (!tree_versionable_function_p (node->decl))
+    {
+      if (dump_file)
+	fprintf (dump_file, "Function not local to this compilation unit.\n");
       return false;
     }
 
Index: mine/gcc/testsuite/g++.dg/torture/pr43905.C
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/g++.dg/torture/pr43905.C
@@ -0,0 +1,13 @@ 
+extern void sf ( __const char *);
+struct Matrix{
+  int operator[](int n){
+    sf ( __PRETTY_FUNCTION__);
+  }
+  int operator[](int n)const{
+    sf ( __PRETTY_FUNCTION__);
+  }
+};
+void calcmy(Matrix const &b, Matrix &c, int k){
+  b[k];
+  c[k];
+}
Index: mine/gcc/Makefile.in
===================================================================
--- mine.orig/gcc/Makefile.in
+++ mine/gcc/Makefile.in
@@ -3118,7 +3118,8 @@  tree-ssa-ccp.o : tree-ssa-ccp.c $(TREE_F
 tree-sra.o : tree-sra.c $(CONFIG_H) $(SYSTEM_H) coretypes.h alloc-pool.h \
    $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) $(IPA_PROP_H) \
    $(DIAGNOSTIC_H) statistics.h $(TREE_DUMP_H) $(TIMEVAR_H) $(PARAMS_H) \
-   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H)
+   $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H) \
+   $(TREE_INLINE_H)
 tree-switch-conversion.o : tree-switch-conversion.c $(CONFIG_H) $(SYSTEM_H) \
     $(TREE_H) $(TM_P_H) $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TREE_INLINE_H) \
     $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \