This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix 44914 - add TODO_cleanup_cfg to IPA-SRA
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;
>
>
--
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex