On Wed, Apr 16, 2014 at 11:22:28AM +0200, Richard Biener wrote:
> 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.
> >
...
> > @@ -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.
The following patch adds gsi_start_edge for that purpose and uses it
together with gsi_commit_edge_inserts from within SRA.
I did not make it an inline static function in the header like the
other gsi initializing functions because that would make
gimple-iterator.h depend on tree-cfg.h and with our current flat
includes that triggered changes of includes in half a gazillion
unrelated c files (I have that patch too because I was apparently too
lazy to think before the third coffee yesterday but I do not think it
is worth it).
Bootstrapped and tested on x86_64-linux, this time it also includes
Eric's testcase. OK for trunk?
Thanks,
Martin
2014-04-16 Martin Jambor <mjam...@suse.cz>
* gimple-iterator.c (gsi_start_edge): New function.
* gimple-iterator.h (gsi_start_edge): Declare.
* 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.
(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): Commit statements on edges.
testsuite/
* gnat.dg/opt34.adb: New.
* gnat.dg/opt34_pkg.ads: Likewise.
diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c
index 1cfeb73..8a1ec53 100644
--- a/gcc/gimple-iterator.c
+++ b/gcc/gimple-iterator.c
@@ -689,6 +689,15 @@ gsi_insert_seq_on_edge (edge e, gimple_seq seq)
gimple_seq_add_seq (&PENDING_STMT (e), seq);
}
+/* Return a new iterator pointing to the first statement in sequence of
+ statements on edge E. Such statements need to be subsequently moved into a
+ basic block by calling gsi_commit_edge_inserts. */
+
+gimple_stmt_iterator
+gsi_start_edge (edge e)
+{
+ return gsi_start (PENDING_STMT (e));
+}
/* Insert the statement pointed-to by GSI into edge E. Every attempt
is made to place the statement in an existing basic block, but
diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index c35dc81..909d58b 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -123,6 +123,8 @@ gsi_start_bb (basic_block bb)
return i;
}
+gimple_stmt_iterator gsi_start_edge (edge e);
+
/* Return a new iterator initially pointing to GIMPLE_SEQ's last statement. */
static inline gimple_stmt_iterator
diff --git a/gcc/testsuite/gnat.dg/opt34.adb b/gcc/testsuite/gnat.dg/opt34.adb
new file mode 100644
index 0000000..a5d0ab6
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/opt34.adb
@@ -0,0 +1,29 @@
+-- { dg-do compile }
+-- { dg-options "-O -fdump-tree-esra" }
+
+with Opt34_Pkg; use Opt34_Pkg;
+
+procedure Opt34 is
+
+ function Local_Fun (Arg : T_Private) return T_Private is
+ Result : T_Private;
+ begin
+
+ case Get_Integer (Arg) is
+ when 1 =>
+ Result := Get_Private (100);
+ when 2 =>
+ Result := T_Private_Zero;
+ when others =>
+ null;
+ end case;
+
+ return Result;
+ end Local_Fun;
+
+begin
+ Assert (Get_Integer (Local_Fun (Get_Private (1))) = 100);
+end;
+
+-- { dg-final { scan-tree-dump "Created a replacement for result" "esra" } }
+-- { dg-final { cleanup-tree-dump "esra" } }
diff --git a/gcc/testsuite/gnat.dg/opt34_pkg.ads
b/gcc/testsuite/gnat.dg/opt34_pkg.ads
new file mode 100644
index 0000000..83ecb49
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/opt34_pkg.ads
@@ -0,0 +1,14 @@
+package Opt34_Pkg is
+
+ type T_Private is record
+ I : Integer := 0;
+ end record;
+
+ T_Private_Zero : constant T_Private := (I => 0);
+
+ function Get_Private (I : Integer) return T_Private;
+ function Get_Integer (X : T_Private) return Integer;
+
+ procedure Assert (Cond : Boolean);
+
+end Opt34_Pkg;
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index ffef13d..73e681d 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;
@@ -2763,6 +2787,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_start_edge (single_non_eh_succ (gsi_bb (*gsi)));
+ gsi = &alt_gsi;
+ }
+
if (access->grp_to_be_replaced)
{
tree repl = get_access_replacement (access);
@@ -3226,14 +3257,23 @@ 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_ends_bb_p (*stmt))
{
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_start_edge (single_non_eh_succ (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,
@@ -3397,6 +3437,7 @@ sra_modify_function_body (void)
}
}
+ gsi_commit_edge_inserts ();
return cfg_changed;
}
