Patchwork Fix 44914 - add TODO_cleanup_cfg to IPA-SRA

login
register
mail settings
Submitter Martin Jambor
Date July 23, 2010, 12:28 p.m.
Message ID <20100723122805.GA28683@virgil.arch.suse.de>
Download mbox | patch
Permalink /patch/59778/
State New
Headers show

Comments

Martin Jambor - July 23, 2010, 12:28 p.m.
On Fri, Jul 23, 2010 at 11:02:17AM +0200, Richard Guenther wrote:
> On Thu, 22 Jul 2010, Martin Jambor wrote:
> 
> > Hi,
> > 
> > this simple patch fixes PR 44914.  IPA-SRA might have left some
> > unreachable BBs in the CFG.
> 
> Hm.  The usual pattern for eh cleanup is
> 
>   if (maybe_clean_eh_stmt (stmt)
>       && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
>     cfg_changed = true;
> 
> and if the cfg changed add TODO_cleanup_cfg to the TODO.  I see
> that tree-sra misses gimple_purge_dead_eh_edges on a path where
> it does maybe_clean_eh_stmt and it doesn't call it conditionally.
> 
> (note that if maybe_clean_eh_stmt returns true we know it is the
> last stmt in the block, so the bb_changed flag isn't necessary in
> ipa_sra_modify_function_body).  Both sra_modify_function_body and
> ipa_sra_modify_function_body should return whether they changed
> the CFG and propagate that flag so TODO_cleanup_cfg is added.
> 
> Can you rework the patch this way?

Like this?

I've bootstrapped and tested an almost identical patch on the trunk
without any problems, I'm doing the same for exactly this one now.
Please also let me know when to test and commit it to the 4.5 branch.

Thanks,

Martin

2010-07-23  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/44914
	* tree-sra.c (sra_modify_function_body): Return true if CFG was
	changed, add purging dead eh edges.
	(ipa_sra_modify_function_body): Return true if CFG was changed,
	simplify purging dead eh edges.
	(modify_function): Return true if CFG was changed.
	(perform_intra_sra): Add TODO_cleanup_cfg to the return value if CFG
	was changed.
	(ipa_early_sra): Likewise.

	* testsuite/g++.dg/tree-ssa/pr44914.C:  New test.
Richard Guenther - July 23, 2010, 12:38 p.m.
On Fri, 23 Jul 2010, Martin Jambor wrote:

> On Fri, Jul 23, 2010 at 11:02:17AM +0200, Richard Guenther wrote:
> > On Thu, 22 Jul 2010, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > this simple patch fixes PR 44914.  IPA-SRA might have left some
> > > unreachable BBs in the CFG.
> > 
> > Hm.  The usual pattern for eh cleanup is
> > 
> >   if (maybe_clean_eh_stmt (stmt)
> >       && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
> >     cfg_changed = true;
> > 
> > and if the cfg changed add TODO_cleanup_cfg to the TODO.  I see
> > that tree-sra misses gimple_purge_dead_eh_edges on a path where
> > it does maybe_clean_eh_stmt and it doesn't call it conditionally.
> > 
> > (note that if maybe_clean_eh_stmt returns true we know it is the
> > last stmt in the block, so the bb_changed flag isn't necessary in
> > ipa_sra_modify_function_body).  Both sra_modify_function_body and
> > ipa_sra_modify_function_body should return whether they changed
> > the CFG and propagate that flag so TODO_cleanup_cfg is added.
> > 
> > Can you rework the patch this way?
> 
> Like this?
> 
> I've bootstrapped and tested an almost identical patch on the trunk
> without any problems, I'm doing the same for exactly this one now.
> Please also let me know when to test and commit it to the 4.5 branch.

This is ok for trunk.  Please wait until after 4.5.1 was released
for the backport (which is ok then).

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 2010-07-23  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/44914
> 	* tree-sra.c (sra_modify_function_body): Return true if CFG was
> 	changed, add purging dead eh edges.
> 	(ipa_sra_modify_function_body): Return true if CFG was changed,
> 	simplify purging dead eh edges.
> 	(modify_function): Return true if CFG was changed.
> 	(perform_intra_sra): Add TODO_cleanup_cfg to the return value if CFG
> 	was changed.
> 	(ipa_early_sra): Likewise.
> 
> 	* testsuite/g++.dg/tree-ssa/pr44914.C:  New test.
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -2791,11 +2791,13 @@ sra_modify_assign (gimple *stmt, gimple_
>  }
>  
>  /* Traverse the function body and all modifications as decided in
> -   analyze_all_variable_accesses.  */
> +   analyze_all_variable_accesses.  Return true iff the CFG has been
> +   changed.  */
>  
> -static void
> +static bool
>  sra_modify_function_body (void)
>  {
> +  bool cfg_changed = false;
>    basic_block bb;
>  
>    FOR_EACH_BB (bb)
> @@ -2858,12 +2860,16 @@ sra_modify_function_body (void)
>  	  if (modified)
>  	    {
>  	      update_stmt (stmt);
> -	      maybe_clean_eh_stmt (stmt);
> +	      if (maybe_clean_eh_stmt (stmt)
> +		  && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
> +		cfg_changed = true;
>  	    }
>  	  if (!deleted)
>  	    gsi_next (&gsi);
>  	}
>      }
> +
> +  return cfg_changed;
>  }
>  
>  /* Generate statements initializing scalar replacements of parts of function
> @@ -2923,7 +2929,10 @@ perform_intra_sra (void)
>    if (!analyze_all_variable_accesses ())
>      goto out;
>  
> -  sra_modify_function_body ();
> +  if (sra_modify_function_body ())
> +    ret = TODO_update_ssa | TODO_cleanup_cfg;
> +  else
> +    ret = TODO_update_ssa;
>    initialize_parameter_reductions ();
>  
>    statistics_counter_event (cfun, "Scalar replacements created",
> @@ -2937,8 +2946,6 @@ perform_intra_sra (void)
>    statistics_counter_event (cfun, "Separate LHS and RHS handling",
>  			    sra_stats.separate_lhs_rhs_handling);
>  
> -  ret = TODO_update_ssa;
> -
>   out:
>    sra_deinitialize ();
>    return ret;
> @@ -4064,17 +4071,17 @@ sra_ipa_modify_assign (gimple *stmt_ptr,
>  }
>  
>  /* Traverse the function body and all modifications as described in
> -   ADJUSTMENTS.  */
> +   ADJUSTMENTS.  Return true iff the CFG has been changed.  */
>  
> -static void
> +static bool
>  ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>  {
> +  bool cfg_changed = false;
>    basic_block bb;
>  
>    FOR_EACH_BB (bb)
>      {
>        gimple_stmt_iterator gsi;
> -      bool bb_changed = false;
>  
>        for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>  	replace_removed_params_ssa_names (gsi_stmt (gsi), adjustments);
> @@ -4136,15 +4143,16 @@ ipa_sra_modify_function_body (ipa_parm_a
>  
>  	  if (modified)
>  	    {
> -	      bb_changed = true;
>  	      update_stmt (stmt);
> -	      maybe_clean_eh_stmt (stmt);
> +	      if (maybe_clean_eh_stmt (stmt)
> +		  && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
> +		cfg_changed = true;
>  	    }
>  	  gsi_next (&gsi);
>  	}
> -      if (bb_changed)
> -	gimple_purge_dead_eh_edges (bb);
>      }
> +
> +  return cfg_changed;
>  }
>  
>  /* Call gimple_debug_bind_reset_value on all debug statements describing
> @@ -4260,13 +4268,14 @@ convert_callers (struct cgraph_node *nod
>  }
>  
>  /* Perform all the modification required in IPA-SRA for NODE to have parameters
> -   as given in ADJUSTMENTS.  */
> +   as given in ADJUSTMENTS.  Return true iff the CFG has been changed.  */
>  
> -static void
> +static bool
>  modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
>  {
>    struct cgraph_node *new_node;
>    struct cgraph_edge *cs;
> +  bool cfg_changed;
>    VEC (cgraph_edge_p, heap) * redirect_callers;
>    int node_callers;
>  
> @@ -4287,11 +4296,11 @@ modify_function (struct cgraph_node *nod
>    push_cfun (DECL_STRUCT_FUNCTION (new_node->decl));
>  
>    ipa_modify_formal_parameters (current_function_decl, adjustments, "ISRA");
> -  ipa_sra_modify_function_body (adjustments);
> +  cfg_changed = ipa_sra_modify_function_body (adjustments);
>    sra_ipa_reset_debug_stmts (adjustments);
>    convert_callers (new_node, node->decl, adjustments);
>    cgraph_make_node_local (new_node);
> -  return;
> +  return cfg_changed;
>  }
>  
>  /* Return false the function is apparently unsuitable for IPA-SRA based on it's
> @@ -4408,9 +4417,11 @@ ipa_early_sra (void)
>    if (dump_file)
>      ipa_dump_param_adjustments (dump_file, adjustments, current_function_decl);
>  
> -  modify_function (node, adjustments);
> +  if (modify_function (node, adjustments))
> +    ret = TODO_update_ssa | TODO_cleanup_cfg;
> +  else
> +    ret = TODO_update_ssa;
>    VEC_free (ipa_parm_adjustment_t, heap, adjustments);
> -  ret = TODO_update_ssa;
>  
>    statistics_counter_event (cfun, "Unused parameters deleted",
>  			    sra_stats.deleted_unused_parameters);
> Index: mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
> @@ -0,0 +1,20 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fipa-sra -fnon-call-exceptions" } */
> +
> +struct A
> +{
> +  ~A () { }
> +};
> +
> +struct B
> +{
> +  A a;
> +  int i;
> +  void f (int) { }
> +  B ()
> +  {
> +    f (i);
> +  }
> +};
> +
> +B b;
> 
>

Patch

Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -2791,11 +2791,13 @@  sra_modify_assign (gimple *stmt, gimple_
 }
 
 /* Traverse the function body and all modifications as decided in
-   analyze_all_variable_accesses.  */
+   analyze_all_variable_accesses.  Return true iff the CFG has been
+   changed.  */
 
-static void
+static bool
 sra_modify_function_body (void)
 {
+  bool cfg_changed = false;
   basic_block bb;
 
   FOR_EACH_BB (bb)
@@ -2858,12 +2860,16 @@  sra_modify_function_body (void)
 	  if (modified)
 	    {
 	      update_stmt (stmt);
-	      maybe_clean_eh_stmt (stmt);
+	      if (maybe_clean_eh_stmt (stmt)
+		  && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
+		cfg_changed = true;
 	    }
 	  if (!deleted)
 	    gsi_next (&gsi);
 	}
     }
+
+  return cfg_changed;
 }
 
 /* Generate statements initializing scalar replacements of parts of function
@@ -2923,7 +2929,10 @@  perform_intra_sra (void)
   if (!analyze_all_variable_accesses ())
     goto out;
 
-  sra_modify_function_body ();
+  if (sra_modify_function_body ())
+    ret = TODO_update_ssa | TODO_cleanup_cfg;
+  else
+    ret = TODO_update_ssa;
   initialize_parameter_reductions ();
 
   statistics_counter_event (cfun, "Scalar replacements created",
@@ -2937,8 +2946,6 @@  perform_intra_sra (void)
   statistics_counter_event (cfun, "Separate LHS and RHS handling",
 			    sra_stats.separate_lhs_rhs_handling);
 
-  ret = TODO_update_ssa;
-
  out:
   sra_deinitialize ();
   return ret;
@@ -4064,17 +4071,17 @@  sra_ipa_modify_assign (gimple *stmt_ptr,
 }
 
 /* Traverse the function body and all modifications as described in
-   ADJUSTMENTS.  */
+   ADJUSTMENTS.  Return true iff the CFG has been changed.  */
 
-static void
+static bool
 ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
 {
+  bool cfg_changed = false;
   basic_block bb;
 
   FOR_EACH_BB (bb)
     {
       gimple_stmt_iterator gsi;
-      bool bb_changed = false;
 
       for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	replace_removed_params_ssa_names (gsi_stmt (gsi), adjustments);
@@ -4136,15 +4143,16 @@  ipa_sra_modify_function_body (ipa_parm_a
 
 	  if (modified)
 	    {
-	      bb_changed = true;
 	      update_stmt (stmt);
-	      maybe_clean_eh_stmt (stmt);
+	      if (maybe_clean_eh_stmt (stmt)
+		  && gimple_purge_dead_eh_edges (gimple_bb (stmt)))
+		cfg_changed = true;
 	    }
 	  gsi_next (&gsi);
 	}
-      if (bb_changed)
-	gimple_purge_dead_eh_edges (bb);
     }
+
+  return cfg_changed;
 }
 
 /* Call gimple_debug_bind_reset_value on all debug statements describing
@@ -4260,13 +4268,14 @@  convert_callers (struct cgraph_node *nod
 }
 
 /* Perform all the modification required in IPA-SRA for NODE to have parameters
-   as given in ADJUSTMENTS.  */
+   as given in ADJUSTMENTS.  Return true iff the CFG has been changed.  */
 
-static void
+static bool
 modify_function (struct cgraph_node *node, ipa_parm_adjustment_vec adjustments)
 {
   struct cgraph_node *new_node;
   struct cgraph_edge *cs;
+  bool cfg_changed;
   VEC (cgraph_edge_p, heap) * redirect_callers;
   int node_callers;
 
@@ -4287,11 +4296,11 @@  modify_function (struct cgraph_node *nod
   push_cfun (DECL_STRUCT_FUNCTION (new_node->decl));
 
   ipa_modify_formal_parameters (current_function_decl, adjustments, "ISRA");
-  ipa_sra_modify_function_body (adjustments);
+  cfg_changed = ipa_sra_modify_function_body (adjustments);
   sra_ipa_reset_debug_stmts (adjustments);
   convert_callers (new_node, node->decl, adjustments);
   cgraph_make_node_local (new_node);
-  return;
+  return cfg_changed;
 }
 
 /* Return false the function is apparently unsuitable for IPA-SRA based on it's
@@ -4408,9 +4417,11 @@  ipa_early_sra (void)
   if (dump_file)
     ipa_dump_param_adjustments (dump_file, adjustments, current_function_decl);
 
-  modify_function (node, adjustments);
+  if (modify_function (node, adjustments))
+    ret = TODO_update_ssa | TODO_cleanup_cfg;
+  else
+    ret = TODO_update_ssa;
   VEC_free (ipa_parm_adjustment_t, heap, adjustments);
-  ret = TODO_update_ssa;
 
   statistics_counter_event (cfun, "Unused parameters deleted",
 			    sra_stats.deleted_unused_parameters);
Index: mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/g++.dg/tree-ssa/pr44914.C
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1 -fipa-sra -fnon-call-exceptions" } */
+
+struct A
+{
+  ~A () { }
+};
+
+struct B
+{
+  A a;
+  int i;
+  void f (int) { }
+  B ()
+  {
+    f (i);
+  }
+};
+
+B b;