From patchwork Fri Jul 23 12:28:06 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 59778 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 3B325B70A4 for ; Fri, 23 Jul 2010 22:28:24 +1000 (EST) Received: (qmail 20212 invoked by alias); 23 Jul 2010 12:28:19 -0000 Received: (qmail 20198 invoked by uid 22791); 23 Jul 2010 12:28:15 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, TW_CF, TW_TM X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 23 Jul 2010 12:28:09 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id 0ECBB9417A for ; Fri, 23 Jul 2010 14:28:07 +0200 (CEST) Date: Fri, 23 Jul 2010 14:28:06 +0200 From: Martin Jambor To: Richard Guenther Cc: GCC Patches Subject: Re: [PATCH] Fix 44914 - add TODO_cleanup_cfg to IPA-SRA Message-ID: <20100723122805.GA28683@virgil.arch.suse.de> Mail-Followup-To: Richard Guenther , GCC Patches References: <20100722180636.GE8513@virgil.arch.suse.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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 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;