[RFC] Move ehcleanup pass to before early SRA

[RFC] Move ehcleanup pass to before early SRA

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

Hi,

we have run into optimization regressions in Ada caused by the interaction 
between the GIMPLE clobbers and SRA: on the one hand GIMPLE clobbers create 
artificial EH handlers for aggregate variables, on the other hand SRA refuses 
to scalarize objects that appear in a statement that can throw internally.  
The result is that GIMPLE clobbers block the scalarization of variables that 
used to be possible up to the 4.6 series.  Testcase attached, compile p.adb on 
x86-64 with -O2 -fdump-tree-ealias -fdump-tree-esra.

This can be solved by moving the ehcleanup pass to before early SRA in 
the pipeline.  It has a small but measurable positive effect on some of our 
benchmarks (with a 4.7-based compiler).  Tested on x86-64/Linux.

Thoughts?


2014-01-15  Eric Botcazou  <ebotcazou@adacore.com>

	* passes.def (pass_early_local_passes): Run first EH cleanup pass early.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1029 bytes --]

Index: passes.def
===================================================================
--- passes.def	(revision 206563)
+++ passes.def	(working copy)
@@ -71,6 +71,10 @@ along with GCC; see the file COPYING3.
 	  /* pass_build_ealias is a dummy pass that ensures that we
 	     execute TODO_rebuild_alias at this point.  */
 	  NEXT_PASS (pass_build_ealias);
+	  /* SRA refuses to scalarize objects that appear in a statement
+	     that can throw internally so we need to cleanup the EH tree
+	     early to remove handlers that contain only clobbers.  */
+	  NEXT_PASS (pass_cleanup_eh);
 	  NEXT_PASS (pass_sra_early);
 	  NEXT_PASS (pass_fre);
 	  NEXT_PASS (pass_copy_prop);
@@ -79,7 +83,6 @@ along with GCC; see the file COPYING3.
 	  NEXT_PASS (pass_early_ipa_sra);
 	  NEXT_PASS (pass_tail_recursion);
 	  NEXT_PASS (pass_convert_switch);
-          NEXT_PASS (pass_cleanup_eh);
           NEXT_PASS (pass_profile);
           NEXT_PASS (pass_local_pure_const);
 	  /* Split functions creates parts that are not run through

[-- Attachment #3: p.adb --]
[-- Type: text/x-adasrc, Size: 302 bytes --]

with Decls_Pack_2;  use Decls_Pack_2;
with Decls_Support; use Decls_Support;
with Support;       use Support;

procedure P is
   package Decls_Pack_Private is new Decls_Pack_Private_G;
   use Decls_Pack_Private;
begin
   Assert (Get_Integer (Decls_Pack_Private.Local_Fun (Get_Private (1))) = 100);
end;

[-- Attachment #4: decls_pack_2.adb --]
[-- Type: text/x-adasrc, Size: 1678 bytes --]

package body Decls_Pack_2 is

   procedure Local_Swap (V1, V2 : in out Access_Integer) is
      Tmp : Access_Integer := V1;
   begin
      V1  := V2;
      V2  := Tmp;
   end Local_Swap;

   function Local_Fun (I : Integer) return Access_Const_Integer is
      Result : Access_Const_Integer := new Integer'(I);
   begin

      if I < 0 then
        Result := null;
      end if;

      return Result;
   end Local_Fun;


   package body Decls_Pack_Derived_Records_G is

      procedure Local_Swap (V1, V2 : in out Derived_Coordinate) is
         Tmp : Derived_Coordinate;
      begin
         if V1 /= V2 then
            Tmp := V1;
            V1  := V2;
            V2  := Tmp;
         end if;
      end Local_Swap;

      function Local_Fun (C1, C2 : Float) return Derived_Coordinate is
         Result : Derived_Coordinate;
      begin

         if C1 > 0.0 and then C2 > 0.0 then
           Result.X := C1;
           Result.Y := C2;
         end if;

         return Result;
      end Local_Fun;

   end Decls_Pack_Derived_Records_G;

   package body Decls_Pack_Private_G is

      procedure Local_Swap (V1, V2 : in out T_Private) is
         Tmp : T_Private := V1;
      begin
         V1 := V2;
         V2 := Tmp;
      end Local_Swap;

      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;

   end Decls_Pack_Private_G;

end Decls_Pack_2;

[-- Attachment #5: decls_pack_2.ads --]
[-- Type: text/x-adasrc, Size: 894 bytes --]

with Decls_Support; use Decls_Support;
with Support;       use Support;
package Decls_Pack_2 is

   Access_All_Integer_Var : Access_All_Integer := Integer_Var'Access;

   I : Integer := Identity (1);

   procedure Local_Swap (V1, V2 : in out Access_Integer);

   function Local_Fun (I : Integer) return Access_Const_Integer;

   generic
   package Decls_Pack_Derived_Records_G is

      Derived_Discrete_Coordinate_V : Derived_Discrete_Coordinate;

      procedure Local_Swap (V1, V2 : in out Derived_Coordinate);

      function Local_Fun (C1, C2 : Float) return Derived_Coordinate;
   end Decls_Pack_Derived_Records_G;

   generic
   package Decls_Pack_Private_G is

      T_Private_C : constant T_Private := T_Private_Zero;

      procedure Local_Swap (V1, V2 : in out T_Private);

      function Local_Fun (Arg : T_Private) return T_Private;
   end Decls_Pack_Private_G;

end Decls_Pack_2;

[-- Attachment #6: decls_support.ads --]
[-- Type: text/x-adasrc, Size: 1377 bytes --]

with Support; use Support;
package Decls_Support is

   type Week_Day is (Mon, Tue, Wen, Thu, Fri, Sat, Sun);

   subtype Index is Natural range 0 .. 100;

   type Var_String (Len : Index := 0) is record
      Data : String (1 .. Len);
   end record;

   type Coordinate is record
      X, Y : Float := 0.0;
   end record;

   Coordinate_Zero : constant Coordinate := (X => 0.0, Y => 0.0);

   type Discrete_Coordinate is record
      X : Integer := Identity (0);
      Y : Integer := Identity (0);
   end record;

   type Vector is array (Integer range <>) of Integer;

   type Matrix is array (Integer range <>, Integer range <>) of Integer;
   subtype Small_Matrix is Matrix (1 .. 2, 1 .. 2);

   type T_Private is private;
   T_Private_Zero : constant T_Private;

   function Get_Private (I : Integer) return T_Private;
   function Get_Integer (X : T_Private) return Integer;
   type Access_Integer is access Integer;
   type Access_All_Integer is access all Integer;
   type Access_Const_Integer is access constant Integer;
   type Access_Coordinate is access Coordinate;

   type Derived_Discrete_Coordinate is new Discrete_Coordinate;
   type Derived_Coordinate is new Coordinate;

   Integer_Var : aliased Integer := 1;

private
   type T_Private is record
      I : Integer := 0;
   end record;

   T_Private_Zero : constant T_Private := (I => 0);

end Decls_Support;

[-- Attachment #7: support.ads --]
[-- Type: text/x-adasrc, Size: 295 bytes --]

package Support is
   procedure Assert (Cond : Boolean);
   function Identity (X : Integer) return Integer;
   function Identity (B : Boolean) return Boolean;

   function Value (X : Integer) return Integer renames Identity;
   function Value (B : Boolean) return Boolean renames Identity;
end;

Re: [RFC] Move ehcleanup pass to before early SRA

[Richard Biener]

On Wed, Jan 15, 2014 at 10:24 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> we have run into optimization regressions in Ada caused by the interaction
> between the GIMPLE clobbers and SRA: on the one hand GIMPLE clobbers create
> artificial EH handlers for aggregate variables, on the other hand SRA refuses
> to scalarize objects that appear in a statement that can throw internally.
> The result is that GIMPLE clobbers block the scalarization of variables that
> used to be possible up to the 4.6 series.  Testcase attached, compile p.adb on
> x86-64 with -O2 -fdump-tree-ealias -fdump-tree-esra.
>
> This can be solved by moving the ehcleanup pass to before early SRA in
> the pipeline.  It has a small but measurable positive effect on some of our
> benchmarks (with a 4.7-based compiler).  Tested on x86-64/Linux.
>
> Thoughts?

What you want is tree-eh.c:optimize_clobbers, right?  Can't we
do this optimization during EH refactoring / lowering as well?

Also why does SRA refuse to scalarize here?  It will end up
scalarizing during regular opts as

<L1>:
  result = decls_support.get_private (100);
  result$i_12 = MEM[(struct decls_support__t_private *)&result];

where it can do the scalarization just fine if it inserts on all
non-EH successor edges (so not doing the scalarization only
if there is more than one non-EH successor edge would make
sense).

The current placement of EH cleanup is to make it catch
all opportunities before early inlining sees the function as
callee.  And I guess both FRE and DCE will expose quite
some EH cleanup opportunities especially with non-call-exceptions.

Richard.

>
> 2014-01-15  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * passes.def (pass_early_local_passes): Run first EH cleanup pass early.
>
>
> --
> Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Eric Botcazou]

> What you want is tree-eh.c:optimize_clobbers, right?  Can't we
> do this optimization during EH refactoring / lowering as well?

We need the SSA form for sink_clobbers.

> Also why does SRA refuse to scalarize here?

Because of the EH handler and the following predicate:

/* 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.  */
static bool
disqualify_ops_if_throwing_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)))
    {
      disqualify_base_of_expr (lhs, "LHS of a throwing stmt.");
      if (rhs)
	disqualify_base_of_expr (rhs, "RHS of a throwing stmt.");
      return true;
    }
  return false;
}

> It will end up scalarizing during regular opts as
> 
> <L1>:
>   result = decls_support.get_private (100);
>   result$i_12 = MEM[(struct decls_support__t_private *)&result];

Yes, it's scalarized during SRA but not ESRA because there is an ehcleanup 
pass in-between.  It used to be scalarized during ESRA up to 4.6.x.

> The current placement of EH cleanup is to make it catch
> all opportunities before early inlining sees the function as
> callee.  And I guess both FRE and DCE will expose quite
> some EH cleanup opportunities especially with non-call-exceptions.

Early inlining or regular inlining (I'm always slightly confused here)?

p.adb.003t.original
p.adb.004t.gimple
p.adb.005t.nested
p.adb.006t.omplower
p.adb.007t.lower
p.adb.009t.ehopt
p.adb.010t.eh
p.adb.011t.cfg
p.adb.015t.ssa
p.adb.017t.inline_param1
p.adb.018t.einline
p.adb.019t.early_optimizations
p.adb.020t.copyrename1
p.adb.021t.ccp1
p.adb.022t.forwprop1
p.adb.023t.ealias
p.adb.024t.esra
p.adb.025t.fre1
p.adb.026t.copyprop1
p.adb.027t.mergephi1
p.adb.028t.cddce1
p.adb.029t.eipa_sra
p.adb.030t.tailr1
p.adb.031t.switchconv
p.adb.032t.ehcleanup1
p.adb.033t.profile_estimate
p.adb.034t.local-pure-const1
p.adb.035t.fnsplit
p.adb.036t.release_ssa
p.adb.037t.inline_param2
p.adb.054t.copyrename2

Then we could add another ehcleanup pass before early inlining, i.e. between 
015t.ssa and 017t.inline_param1, to achieve the same goal.

-- 
Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Richard Biener]

On Wed, Jan 15, 2014 at 1:06 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> What you want is tree-eh.c:optimize_clobbers, right?  Can't we
>> do this optimization during EH refactoring / lowering as well?
>
> We need the SSA form for sink_clobbers.

I know, I'm saying it may be possible to implement an equivalent
optimization without SSA form.

>> Also why does SRA refuse to scalarize here?
>
> Because of the EH handler and the following predicate:
>
> /* 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.  */
> static bool
> disqualify_ops_if_throwing_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)))
>     {
>       disqualify_base_of_expr (lhs, "LHS of a throwing stmt.");
>       if (rhs)
>         disqualify_base_of_expr (rhs, "RHS of a throwing stmt.");
>       return true;
>     }
>   return false;
> }
>
>> It will end up scalarizing during regular opts as
>>
>> <L1>:
>>   result = decls_support.get_private (100);
>>   result$i_12 = MEM[(struct decls_support__t_private *)&result];
>
> Yes, it's scalarized during SRA but not ESRA because there is an ehcleanup
> pass in-between.  It used to be scalarized during ESRA up to 4.6.x.

I'm saying even ESRA should be able to scalarize it just fine.  It just
needs to be careful where to insert the loads from the aggregate result
(on the single outgoing non-EH edge).

Wouldn't that fix your issue?

>> The current placement of EH cleanup is to make it catch
>> all opportunities before early inlining sees the function as
>> callee.  And I guess both FRE and DCE will expose quite
>> some EH cleanup opportunities especially with non-call-exceptions.
>
> Early inlining or regular inlining (I'm always slightly confused here)?

Early inlining.

> p.adb.003t.original
> p.adb.004t.gimple
> p.adb.005t.nested
> p.adb.006t.omplower
> p.adb.007t.lower
> p.adb.009t.ehopt
> p.adb.010t.eh
> p.adb.011t.cfg
> p.adb.015t.ssa
> p.adb.017t.inline_param1
> p.adb.018t.einline
> p.adb.019t.early_optimizations
> p.adb.020t.copyrename1
> p.adb.021t.ccp1
> p.adb.022t.forwprop1
> p.adb.023t.ealias
> p.adb.024t.esra
> p.adb.025t.fre1
> p.adb.026t.copyprop1
> p.adb.027t.mergephi1
> p.adb.028t.cddce1
> p.adb.029t.eipa_sra
> p.adb.030t.tailr1
> p.adb.031t.switchconv
> p.adb.032t.ehcleanup1
> p.adb.033t.profile_estimate
> p.adb.034t.local-pure-const1
> p.adb.035t.fnsplit
> p.adb.036t.release_ssa
> p.adb.037t.inline_param2
> p.adb.054t.copyrename2
>
> Then we could add another ehcleanup pass before early inlining, i.e. between
> 015t.ssa and 017t.inline_param1, to achieve the same goal.
>
> --
> Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Martin Jambor]

Hi,

On Wed, Jan 15, 2014 at 03:39:09PM +0100, Richard Biener wrote:
> On Wed, Jan 15, 2014 at 1:06 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > Yes, it's scalarized during SRA but not ESRA because there is an ehcleanup
> > pass in-between.  It used to be scalarized during ESRA up to 4.6.x.
> 
> I'm saying even ESRA should be able to scalarize it just fine.  It just
> needs to be careful where to insert the loads from the aggregate result
> (on the single outgoing non-EH edge).
> 
> Wouldn't that fix your issue?

Eric, more specifically, would the (untested proof of concept) patch
below help?

Martin


diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 4992b4c..a4c0e21 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -1142,6 +1142,26 @@ build_access_from_expr (tree expr, gimple stmt, bool write)
   return false;
 }
 
+/* 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 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
@@ -1153,6 +1173,9 @@ disqualify_ops_if_throwing_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)))
     {
+      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.");
@@ -2720,6 +2743,19 @@ get_access_for_expr (tree expr)
   return get_var_base_offset_size_access (base, offset, max_size);
 }
 
+/* Aplit the sinle non-EH successor edge from BB (there must be exactly one)
+   and return a gsi 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
@@ -2749,6 +2785,14 @@ 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_can_throw_internal (gsi_stmt (*gsi))
+		|| stmt_ends_bb_p (gsi_stmt (*gsi))))
+    {
+      alt_gsi = gsi_for_eh_followups (gsi_bb (*gsi));
+      gsi = &alt_gsi;
+    }
+
   if (access->grp_to_be_replaced)
     {
       tree repl = get_access_replacement (access);
@@ -3208,14 +3252,25 @@ 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))
     {
       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_can_throw_internal (*stmt)
+	      || 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,
@@ -3309,8 +3364,13 @@ sra_modify_function_body (void)
 {
   bool cfg_changed = false;
   basic_block bb;
+  vec<basic_block> bb_list = vNULL;
 
   FOR_EACH_BB_FN (bb, cfun)
+    bb_list.safe_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))
@@ -3379,6 +3439,7 @@ sra_modify_function_body (void)
 	}
     }
 
+  bb_list.release ();
   return cfg_changed;
 }
 

Re: [RFC] Move ehcleanup pass to before early SRA

[Eric Botcazou]

> I know, I'm saying it may be possible to implement an equivalent
> optimization without SSA form.

Sure, but what would be the point of this duplication exactly?

> I'm saying even ESRA should be able to scalarize it just fine.  It just
> needs to be careful where to insert the loads from the aggregate result
> (on the single outgoing non-EH edge).
> 
> Wouldn't that fix your issue?

Very likely, yes, but there might be other passes with the same problem before 
the first EH cleanup pass (e.g. FRE, DCE, etc).

> Early inlining.

Are you sure?  I don't see how 032t.ehcleanup1 can have any effects on 
p.adb.018t.einline (unless something mysterious happens here).

> > p.adb.003t.original
> > p.adb.004t.gimple
> > p.adb.005t.nested
> > p.adb.006t.omplower
> > p.adb.007t.lower
> > p.adb.009t.ehopt
> > p.adb.010t.eh
> > p.adb.011t.cfg
> > p.adb.015t.ssa
> > p.adb.017t.inline_param1
> > p.adb.018t.einline
> > p.adb.019t.early_optimizations
> > p.adb.020t.copyrename1
> > p.adb.021t.ccp1
> > p.adb.022t.forwprop1
> > p.adb.023t.ealias
> > p.adb.024t.esra
> > p.adb.025t.fre1
> > p.adb.026t.copyprop1
> > p.adb.027t.mergephi1
> > p.adb.028t.cddce1
> > p.adb.029t.eipa_sra
> > p.adb.030t.tailr1
> > p.adb.031t.switchconv
> > p.adb.032t.ehcleanup1
> > p.adb.033t.profile_estimate
> > p.adb.034t.local-pure-const1
> > p.adb.035t.fnsplit
> > p.adb.036t.release_ssa
> > p.adb.037t.inline_param2
> > p.adb.054t.copyrename2

-- 
Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Martin Jambor]

Hi,

On Wed, Jan 15, 2014 at 06:37:30PM +0100, Eric Botcazou wrote:
> > I know, I'm saying it may be possible to implement an equivalent
> > optimization without SSA form.
> 
> Sure, but what would be the point of this duplication exactly?
> 
> > I'm saying even ESRA should be able to scalarize it just fine.  It just
> > needs to be careful where to insert the loads from the aggregate result
> > (on the single outgoing non-EH edge).
> > 
> > Wouldn't that fix your issue?
> 
> Very likely, yes, but there might be other passes with the same problem before 
> the first EH cleanup pass (e.g. FRE, DCE, etc).
> 
> > Early inlining.
> 
> Are you sure?  I don't see how 032t.ehcleanup1 can have any effects on 
> p.adb.018t.einline (unless something mysterious happens here).

We early-optimize functions in topological order so generally (except
for SCCs), so when 018t.einline runs all the callees have already been
early-optimized, and it is really the callees it looks at most.

(But it of course prepares the function for ipa inlining analysis too
since the standalone functions will be untouched.)

Martin

> 
> > > p.adb.003t.original
> > > p.adb.004t.gimple
> > > p.adb.005t.nested
> > > p.adb.006t.omplower
> > > p.adb.007t.lower
> > > p.adb.009t.ehopt
> > > p.adb.010t.eh
> > > p.adb.011t.cfg
> > > p.adb.015t.ssa
> > > p.adb.017t.inline_param1
> > > p.adb.018t.einline
> > > p.adb.019t.early_optimizations
> > > p.adb.020t.copyrename1
> > > p.adb.021t.ccp1
> > > p.adb.022t.forwprop1
> > > p.adb.023t.ealias
> > > p.adb.024t.esra
> > > p.adb.025t.fre1
> > > p.adb.026t.copyprop1
> > > p.adb.027t.mergephi1
> > > p.adb.028t.cddce1
> > > p.adb.029t.eipa_sra
> > > p.adb.030t.tailr1
> > > p.adb.031t.switchconv
> > > p.adb.032t.ehcleanup1
> > > p.adb.033t.profile_estimate
> > > p.adb.034t.local-pure-const1
> > > p.adb.035t.fnsplit
> > > p.adb.036t.release_ssa
> > > p.adb.037t.inline_param2
> > > p.adb.054t.copyrename2
> 
> -- 
> Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Richard Biener]

On Wed, Jan 15, 2014 at 6:37 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I know, I'm saying it may be possible to implement an equivalent
>> optimization without SSA form.
>
> Sure, but what would be the point of this duplication exactly?

To avoid ...

>> I'm saying even ESRA should be able to scalarize it just fine.  It just
>> needs to be careful where to insert the loads from the aggregate result
>> (on the single outgoing non-EH edge).
>>
>> Wouldn't that fix your issue?
>
> Very likely, yes, but there might be other passes with the same problem before
> the first EH cleanup pass (e.g. FRE, DCE, etc).

... these and avoid adding another EH cleanup pass.

>> Early inlining.
>
> Are you sure?  I don't see how 032t.ehcleanup1 can have any effects on
> p.adb.018t.einline (unless something mysterious happens here).
>
>> > p.adb.003t.original
>> > p.adb.004t.gimple
>> > p.adb.005t.nested
>> > p.adb.006t.omplower
>> > p.adb.007t.lower
>> > p.adb.009t.ehopt
>> > p.adb.010t.eh
>> > p.adb.011t.cfg
>> > p.adb.015t.ssa
>> > p.adb.017t.inline_param1
>> > p.adb.018t.einline
>> > p.adb.019t.early_optimizations
>> > p.adb.020t.copyrename1
>> > p.adb.021t.ccp1
>> > p.adb.022t.forwprop1
>> > p.adb.023t.ealias
>> > p.adb.024t.esra
>> > p.adb.025t.fre1
>> > p.adb.026t.copyprop1
>> > p.adb.027t.mergephi1
>> > p.adb.028t.cddce1
>> > p.adb.029t.eipa_sra
>> > p.adb.030t.tailr1
>> > p.adb.031t.switchconv
>> > p.adb.032t.ehcleanup1
>> > p.adb.033t.profile_estimate
>> > p.adb.034t.local-pure-const1
>> > p.adb.035t.fnsplit
>> > p.adb.036t.release_ssa
>> > p.adb.037t.inline_param2
>> > p.adb.054t.copyrename2
>
> --
> Eric Botcazou

Re: [RFC] Move ehcleanup pass to before early SRA

[Richard Guenther]

On Sun, Sep 23, 2012 at 10:46 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> we have run into optimization regressions in Ada caused by the interaction
> between the new GIMPLE clobbers and SRA: on the one hand GIMPLE clobbers
> create artificial EH handlers for aggregate variables, on the other hand SRA
> refuses to scalarize objects that appear in a statement that can throw
> internally.  The result is that GIMPLE clobbers block the scalarization of
> variables that used to be possible up to the 4.6 series.
>
> Therefore the attached patch moves the ehcleanup pass to before early SRA in
> the pipeline.  It has a small but measurable positive effect on some of our
> benchmarks (with a 4.7-based compiler).  Tested on x86-64/Linux.

Hmm.  I think cleanup EH is scheduled late to help early inlining.  Do you
have a testcase where I can quickly look at the issue you describe?

Thanks,
Richard.

>
> 2012-09-23  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * passes.c (init_optimization_passes): Run first EH cleanup pass early.
>
>
> --
> Eric Botcazou

[RFC] Move ehcleanup pass to before early SRA

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Hi,

we have run into optimization regressions in Ada caused by the interaction 
between the new GIMPLE clobbers and SRA: on the one hand GIMPLE clobbers 
create artificial EH handlers for aggregate variables, on the other hand SRA 
refuses to scalarize objects that appear in a statement that can throw 
internally.  The result is that GIMPLE clobbers block the scalarization of 
variables that used to be possible up to the 4.6 series.

Therefore the attached patch moves the ehcleanup pass to before early SRA in 
the pipeline.  It has a small but measurable positive effect on some of our 
benchmarks (with a 4.7-based compiler).  Tested on x86-64/Linux.


2012-09-23  Eric Botcazou  <ebotcazou@adacore.com>

	* passes.c (init_optimization_passes): Run first EH cleanup pass early.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1018 bytes --]

Index: passes.c
===================================================================
--- passes.c	(revision 191365)
+++ passes.c	(working copy)
@@ -1303,6 +1303,10 @@ init_optimization_passes (void)
 	  /* pass_build_ealias is a dummy pass that ensures that we
 	     execute TODO_rebuild_alias at this point.  */
 	  NEXT_PASS (pass_build_ealias);
+	  /* SRA refuses to scalarize objects that appear in a statement
+	     that can throw internally so we need to cleanup the EH tree
+	     early to remove handlers that contain only clobbers.  */
+	  NEXT_PASS (pass_cleanup_eh);
 	  NEXT_PASS (pass_sra_early);
 	  NEXT_PASS (pass_fre);
 	  NEXT_PASS (pass_copy_prop);
@@ -1311,7 +1315,6 @@ init_optimization_passes (void)
 	  NEXT_PASS (pass_early_ipa_sra);
 	  NEXT_PASS (pass_tail_recursion);
 	  NEXT_PASS (pass_convert_switch);
-          NEXT_PASS (pass_cleanup_eh);
           NEXT_PASS (pass_profile);
           NEXT_PASS (pass_local_pure_const);
 	  /* Split functions creates parts that are not run through