On Tue, 15 Apr 2014, Martin Jambor wrote:
> Hi,
>
> back in January in
> http://gcc.gnu.org/ml/gcc-patches/2014-01/msg00848.html Eric pointed
> out a testcase where the problem was SRA not scalarizing an aggregate
> because it was involved in a throwing statement. The reason is that
> SRA is likely to need to append new statements after each one where a
> replaced aggregate is present, but throwing statements must end their
> BBs. This patch comes up with a fix for most such situations by
> adding these new statements onto a single successor non-EH edge, if
> there is one and only one such edge.
>
> I have bootstrapped and tested a very similar version on x86_64-linux,
> bootstrap and testing of this exact one is currently underway. OK for
> trunk? Eric, if and once this gets in, can you please add the
> testcase from your original post to the suite?
>
> Thanks,
>
> Martin
>
>
> 2014-04-15 Martin Jambor <mjam...@suse.cz>
>
> * tree-sra.c (single_non_eh_succ): New function.
> (disqualify_ops_if_throwing_stmt): Renamed to
> disqualify_if_bad_bb_terminating_stmt. Allow throwing statements
> having one non-EH successor BB.
> (gsi_for_eh_followups): New function.
> (sra_modify_expr): If stmt ends bb, use single non-EH successor to
> generate loads into replacements.
> (sra_modify_assign): Likewise and and also use the simple path for
> such statements.
> (sra_modify_function_body): Iterate safely over BBs.
>
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index ffef13d..4fd0f5e 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -1142,17 +1142,41 @@ build_access_from_expr (tree expr, gimple stmt, bool
> write)
> return false;
> }
>
> -/* Disqualify LHS and RHS for scalarization if STMT must end its basic block
> in
> - modes in which it matters, return true iff they have been disqualified.
> RHS
> - may be NULL, in that case ignore it. If we scalarize an aggregate in
> - intra-SRA we may need to add statements after each statement. This is not
> - possible if a statement unconditionally has to end the basic block. */
> +/* Return the single non-EH successor edge of BB or NULL if there is none or
> + more than one. */
> +
> +static edge
> +single_non_eh_succ (basic_block bb)
> +{
> + edge e, res = NULL;
> + edge_iterator ei;
> +
> + FOR_EACH_EDGE (e, ei, bb->succs)
> + if (!(e->flags & EDGE_EH))
> + {
> + if (res)
> + return NULL;
> + res = e;
> + }
> +
> + return res;
> +}
> +
> +/* Disqualify LHS and RHS for scalarization if STMT has to terminate its BB
> and
> + there is no alternative spot where to put statements SRA might need to
> + generate after it. The spot we are looking for is an edge leading to a
> + single non-EH successor, if it exists and is indeed single. RHS may be
> + NULL, in that case ignore it. */
> +
> static bool
> -disqualify_ops_if_throwing_stmt (gimple stmt, tree lhs, tree rhs)
> +disqualify_if_bad_bb_terminating_stmt (gimple stmt, tree lhs, tree rhs)
> {
> if ((sra_mode == SRA_MODE_EARLY_INTRA || sra_mode == SRA_MODE_INTRA)
> - && (stmt_can_throw_internal (stmt) || stmt_ends_bb_p (stmt)))
> + && stmt_ends_bb_p (stmt))
> {
> + if (single_non_eh_succ (gimple_bb (stmt)))
> + return false;
> +
> disqualify_base_of_expr (lhs, "LHS of a throwing stmt.");
> if (rhs)
> disqualify_base_of_expr (rhs, "RHS of a throwing stmt.");
> @@ -1180,7 +1204,7 @@ build_accesses_from_assign (gimple stmt)
> lhs = gimple_assign_lhs (stmt);
> rhs = gimple_assign_rhs1 (stmt);
>
> - if (disqualify_ops_if_throwing_stmt (stmt, lhs, rhs))
> + if (disqualify_if_bad_bb_terminating_stmt (stmt, lhs, rhs))
> return false;
>
> racc = build_access_from_expr_1 (rhs, stmt, false);
> @@ -1319,7 +1343,7 @@ scan_function (void)
> }
>
> t = gimple_call_lhs (stmt);
> - if (t && !disqualify_ops_if_throwing_stmt (stmt, t, NULL))
> + if (t && !disqualify_if_bad_bb_terminating_stmt (stmt, t, NULL))
> ret |= build_access_from_expr (t, stmt, true);
> break;
>
> @@ -2734,6 +2758,19 @@ get_access_for_expr (tree expr)
> return get_var_base_offset_size_access (base, offset, max_size);
> }
>
> +/* Split the single non-EH successor edge from BB (there must be exactly one)
> + and return a gimple iterator to the new block. */
> +
> +static gimple_stmt_iterator
> +gsi_for_eh_followups (basic_block bb)
> +{
> + edge e = single_non_eh_succ (bb);
> + gcc_assert (e);
> +
> + basic_block new_bb = split_edge (e);
> + return gsi_start_bb (new_bb);
> +}
> +
> /* Replace the expression EXPR with a scalar replacement if there is one and
> generate other statements to do type conversion or subtree copying if
> necessary. GSI is used to place newly created statements, WRITE is true
> if
> @@ -2763,6 +2800,13 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator
> *gsi, bool write)
> type = TREE_TYPE (*expr);
>
> loc = gimple_location (gsi_stmt (*gsi));
> + gimple_stmt_iterator alt_gsi = gsi_none ();
> + if (write && stmt_ends_bb_p (gsi_stmt (*gsi)))
> + {
> + alt_gsi = gsi_for_eh_followups (gsi_bb (*gsi));
> + gsi = &alt_gsi;
I think you should try to either use gsi_insert_on_edge_immediate
(yeah, bad we can't build a gsi_for_edge_insert ()) or add
a gsi_for_edge_insert () building on gimple_find_edge_insert_loc
(note the before/after flag that returns - gsi_insert_* variants
that take a flag specifying after/before would come handy here).
You could also add a flag to gimple_find_edge_insert_loc whether
it always should be possible to use gsi_insert_after and split
the block in some more cases (or split it if both after and
before inserts should be valid, but that would not split in
the very rare case of an empty successor only).
Basically usually you can avoid splitting the edge.
> + }
> +
> if (access->grp_to_be_replaced)
> {
> tree repl = get_access_replacement (access);
> @@ -3226,14 +3270,24 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator
> *gsi)
> if (modify_this_stmt
> || gimple_has_volatile_ops (*stmt)
> || contains_vce_or_bfcref_p (rhs)
> - || contains_vce_or_bfcref_p (lhs))
> + || contains_vce_or_bfcref_p (lhs)
> + || stmt_can_throw_internal (*stmt)
> + || stmt_ends_bb_p (*stmt))
stmt_can_throw_internal is redundant.
> {
> if (access_has_children_p (racc))
> generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
> gsi, false, false, loc);
> if (access_has_children_p (lacc))
> - generate_subtree_copies (lacc->first_child, lacc->base, 0, 0, 0,
> - gsi, true, true, loc);
> + {
> + gimple_stmt_iterator alt_gsi = gsi_none ();
> + if (stmt_ends_bb_p (*stmt))
> + {
> + alt_gsi = gsi_for_eh_followups (gsi_bb (*gsi));
> + gsi = &alt_gsi;
> + }
> + generate_subtree_copies (lacc->first_child, lacc->base, 0, 0, 0,
> + gsi, true, true, loc);
> + }
> sra_stats.separate_lhs_rhs_handling++;
>
> /* This gimplification must be done after generate_subtree_copies,
> @@ -3327,8 +3381,13 @@ sra_modify_function_body (void)
> {
> bool cfg_changed = false;
> basic_block bb;
> + vec<basic_block> bb_list = vNULL;
>
auto_vec<basic_block> bb_list;
bb_list.create (n_basic_blocks_for_fn (cfun));
> FOR_EACH_BB_FN (bb, cfun)
> + bb_list.safe_push (bb);
bb_list.quick_push (bb);
> +
> + unsigned bb_vec_index;
> + FOR_EACH_VEC_ELT (bb_list, bb_vec_index, bb)
> {
> gimple_stmt_iterator gsi = gsi_start_bb (bb);
> while (!gsi_end_p (gsi))
> @@ -3397,6 +3456,7 @@ sra_modify_function_body (void)
> }
> }
>
> + bb_list.release ();
and with auto_vec you can avoid this (it has a destructor).
But is it really necessary to avoid scanning new blocks (which
would mean that inserting stmts in the successor instead of
always a newly created block would not work)?
Thanks,
Richard.
> return cfg_changed;
> }
