Patchwork MEM_REF clobber handling fixes/improvements (PR c++/34949, PR c++/50243)

login
register
mail settings
Submitter Jakub Jelinek
Date April 4, 2013, 6:15 p.m.
Message ID <20130404181532.GU4201@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/233892/
State New
Headers show

Comments

Jakub Jelinek - April 4, 2013, 6:15 p.m.
Hi!

The vt3.C testcase (from PR34949) ICEd, because sink_clobbers sunk a
MEM_REF[SSA_NAME] clobber from a landing pad which that SSA_NAME definition
dominated to an outer one which wasn't dominated by the definition.
As the ehcleanup nor ehdisp passes keep dominance info current and perform
changes that invalidate it, I can't unfortunately do cheaply a
dominated_by_p check, so the patch just throws away all MEM_REF[SSA_NAME]
clobbers if SSA_NAME isn't a default def which is valid everywhere.  As
sink_clobbers is only done on otherwise empty bb's and typically the
clobbers are preceeded by some stores which are to be DSEd if unneeded
and after DSEing aren't really needed anymore, this doesn't seem to hurt
much.
The patch also improves optimize_clobbers, so that it only removes any
clobbers if the bb is actually empty (except for clobbers, resx, maybe debug
stmts or __builtin_stack_restore), that way needed clobbers are kept around
until they are used by DSE.
Also, MEM_REF[SSA_NAME] clobbers aren't useful very late in the optimization
pipeline, but could cause some SSA_NAMEs to be considered unnecessarily live
(especially if they are considered live across EH edges it is undesirable),
such clobbers are mainly useful during DSE1/DSE2, but at expansion are
completely ignored (unlike VAR_DECL clobbers, which are also used for the
stack layout decisions), so the patch removes all those MEM_REF[SSA_NAME]
clobbers shortly after dse2 (in fab pass).

Bootstrapped/regtested on x86_64-linux and i686-linux, on libstdc++ I saw
some code size reduction with the patch.

Ok for trunk?

2013-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR c++/34949
	PR c++/50243
	* tree-eh.c (optimize_clobbers): Only remove clobbers if bb doesn't
	contain anything but clobbers, at most one __builtin_stack_restore,
	optionally debug stmts and final resx, and if it has at least one
	incoming EH edge.  Don't check for SSA_NAME on LHS of a clobber.
	(sink_clobbers): Don't check for SSA_NAME on LHS of a clobber.
	Instead of moving clobbers with MEM_REF LHS with SSA_NAME address
	which isn't defaut definition, remove them.
	(unsplit_eh, cleanup_empty_eh): Use single_{pred,succ}_{p,edge}
	instead of EDGE_COUNT comparisons or EDGE_{PRED,SUCC}.
	* tree-ssa-ccp.c (execute_fold_all_builtins): Remove clobbers
	with MEM_REF LHS with SSA_NAME address.

	* g++.dg/opt/vt3.C: New test.
	* g++.dg/opt/vt4.C: New test.


	Jakub
Richard Guenther - April 8, 2013, 8:44 a.m.
On Thu, 4 Apr 2013, Jakub Jelinek wrote:

> Hi!
> 
> The vt3.C testcase (from PR34949) ICEd, because sink_clobbers sunk a
> MEM_REF[SSA_NAME] clobber from a landing pad which that SSA_NAME definition
> dominated to an outer one which wasn't dominated by the definition.
> As the ehcleanup nor ehdisp passes keep dominance info current and perform
> changes that invalidate it, I can't unfortunately do cheaply a
> dominated_by_p check, so the patch just throws away all MEM_REF[SSA_NAME]
> clobbers if SSA_NAME isn't a default def which is valid everywhere.  As
> sink_clobbers is only done on otherwise empty bb's and typically the
> clobbers are preceeded by some stores which are to be DSEd if unneeded
> and after DSEing aren't really needed anymore, this doesn't seem to hurt
> much.
> The patch also improves optimize_clobbers, so that it only removes any
> clobbers if the bb is actually empty (except for clobbers, resx, maybe debug
> stmts or __builtin_stack_restore), that way needed clobbers are kept around
> until they are used by DSE.
> Also, MEM_REF[SSA_NAME] clobbers aren't useful very late in the optimization
> pipeline, but could cause some SSA_NAMEs to be considered unnecessarily live
> (especially if they are considered live across EH edges it is undesirable),
> such clobbers are mainly useful during DSE1/DSE2, but at expansion are
> completely ignored (unlike VAR_DECL clobbers, which are also used for the
> stack layout decisions), so the patch removes all those MEM_REF[SSA_NAME]
> clobbers shortly after dse2 (in fab pass).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, on libstdc++ I saw
> some code size reduction with the patch.
> 
> Ok for trunk?

Ok.

Thanks,
Richard.

> 2013-04-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/34949
> 	PR c++/50243
> 	* tree-eh.c (optimize_clobbers): Only remove clobbers if bb doesn't
> 	contain anything but clobbers, at most one __builtin_stack_restore,
> 	optionally debug stmts and final resx, and if it has at least one
> 	incoming EH edge.  Don't check for SSA_NAME on LHS of a clobber.
> 	(sink_clobbers): Don't check for SSA_NAME on LHS of a clobber.
> 	Instead of moving clobbers with MEM_REF LHS with SSA_NAME address
> 	which isn't defaut definition, remove them.
> 	(unsplit_eh, cleanup_empty_eh): Use single_{pred,succ}_{p,edge}
> 	instead of EDGE_COUNT comparisons or EDGE_{PRED,SUCC}.
> 	* tree-ssa-ccp.c (execute_fold_all_builtins): Remove clobbers
> 	with MEM_REF LHS with SSA_NAME address.
> 
> 	* g++.dg/opt/vt3.C: New test.
> 	* g++.dg/opt/vt4.C: New test.
> 
> --- gcc/tree-eh.c.jj	2013-03-26 10:03:55.000000000 +0100
> +++ gcc/tree-eh.c	2013-04-04 13:44:27.718982776 +0200
> @@ -3230,14 +3230,48 @@ static void
>  optimize_clobbers (basic_block bb)
>  {
>    gimple_stmt_iterator gsi = gsi_last_bb (bb);
> +  bool any_clobbers = false;
> +  bool seen_stack_restore = false;
> +  edge_iterator ei;
> +  edge e;
> +
> +  /* Only optimize anything if the bb contains at least one clobber,
> +     ends with resx (checked by caller), optionally contains some
> +     debug stmts or labels, or at most one __builtin_stack_restore
> +     call, and has an incoming EH edge.  */
>    for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
>      {
>        gimple stmt = gsi_stmt (gsi);
>        if (is_gimple_debug (stmt))
>  	continue;
> -      if (!gimple_clobber_p (stmt)
> -	  || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
> -	return;
> +      if (gimple_clobber_p (stmt))
> +	{
> +	  any_clobbers = true;
> +	  continue;
> +	}
> +      if (!seen_stack_restore
> +	  && gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE))
> +	{
> +	  seen_stack_restore = true;
> +	  continue;
> +	}
> +      if (gimple_code (stmt) == GIMPLE_LABEL)
> +	break;
> +      return;
> +    }
> +  if (!any_clobbers)
> +    return;
> +  FOR_EACH_EDGE (e, ei, bb->preds)
> +    if (e->flags & EDGE_EH)
> +      break;
> +  if (e == NULL)
> +    return;
> +  gsi = gsi_last_bb (bb);
> +  for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
> +    {
> +      gimple stmt = gsi_stmt (gsi);
> +      if (!gimple_clobber_p (stmt))
> +	continue;
>        unlink_stmt_vdef (stmt);
>        gsi_remove (&gsi, true);
>        release_defs (stmt);
> @@ -3278,8 +3312,7 @@ sink_clobbers (basic_block bb)
>  	continue;
>        if (gimple_code (stmt) == GIMPLE_LABEL)
>  	break;
> -      if (!gimple_clobber_p (stmt)
> -	  || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
> +      if (!gimple_clobber_p (stmt))
>  	return 0;
>        any_clobbers = true;
>      }
> @@ -3292,11 +3325,27 @@ sink_clobbers (basic_block bb)
>    for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
>      {
>        gimple stmt = gsi_stmt (gsi);
> +      tree lhs;
>        if (is_gimple_debug (stmt))
>  	continue;
>        if (gimple_code (stmt) == GIMPLE_LABEL)
>  	break;
>        unlink_stmt_vdef (stmt);
> +      lhs = gimple_assign_lhs (stmt);
> +      /* Unfortunately we don't have dominance info updated at this
> +	 point, so checking if
> +	 dominated_by_p (CDI_DOMINATORS, succbb,
> +			 gimple_bb (SSA_NAME_DEF_STMT (TREE_OPERAND (lhs, 0)))
> +	 would be too costly.  Thus, avoid sinking any clobbers that
> +	 refer to non-(D) SSA_NAMEs.  */
> +      if (TREE_CODE (lhs) == MEM_REF
> +	  && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME
> +	  && !SSA_NAME_IS_DEFAULT_DEF (TREE_OPERAND (lhs, 0)))
> +	{
> +	  gsi_remove (&gsi, true);
> +	  release_defs (stmt);
> +	  continue;
> +	}
>        gsi_remove (&gsi, false);
>        /* Trigger the operand scanner to cause renaming for virtual
>           operands for this statement.
> @@ -3737,10 +3786,10 @@ unsplit_eh (eh_landing_pad lp)
>    edge e_in, e_out;
>  
>    /* Quickly check the edge counts on BB for singularity.  */
> -  if (EDGE_COUNT (bb->preds) != 1 || EDGE_COUNT (bb->succs) != 1)
> +  if (!single_pred_p (bb) || !single_succ_p (bb))
>      return false;
> -  e_in = EDGE_PRED (bb, 0);
> -  e_out = EDGE_SUCC (bb, 0);
> +  e_in = single_pred_edge (bb);
> +  e_out = single_succ_edge (bb);
>  
>    /* Input edge must be EH and output edge must be normal.  */
>    if ((e_in->flags & EDGE_EH) == 0 || (e_out->flags & EDGE_EH) != 0)
> @@ -4142,7 +4191,7 @@ cleanup_empty_eh (eh_landing_pad lp)
>        e_out = NULL;
>        break;
>      case 1:
> -      e_out = EDGE_SUCC (bb, 0);
> +      e_out = single_succ_edge (bb);
>        break;
>      default:
>        return false;
> --- gcc/tree-ssa-ccp.c.jj	2013-02-20 18:40:49.000000000 +0100
> +++ gcc/tree-ssa-ccp.c	2013-04-04 14:01:08.926335910 +0200
> @@ -2396,6 +2396,21 @@ execute_fold_all_builtins (void)
>  
>            if (gimple_code (stmt) != GIMPLE_CALL)
>  	    {
> +	      /* Remove all *ssaname_N ={v} {CLOBBER}; stmts,
> +		 after the last GIMPLE DSE they aren't needed and might
> +		 unnecessarily keep the SSA_NAMEs live.  */
> +	      if (gimple_clobber_p (stmt))
> +		{
> +		  tree lhs = gimple_assign_lhs (stmt);
> +		  if (TREE_CODE (lhs) == MEM_REF
> +		      && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME)
> +		    {
> +		      unlink_stmt_vdef (stmt);
> +		      gsi_remove (&i, true);
> +		      release_defs (stmt);
> +		      continue;
> +		    }
> +		}
>  	      gsi_next (&i);
>  	      continue;
>  	    }
> --- gcc/testsuite/g++.dg/opt/vt3.C.jj	2013-04-04 14:33:12.190484331 +0200
> +++ gcc/testsuite/g++.dg/opt/vt3.C	2013-04-04 14:32:19.000000000 +0200
> @@ -0,0 +1,43 @@
> +// PR c++/34949
> +// { dg-do compile }
> +// { dg-options "-O3" }
> +
> +struct E {};
> +struct A
> +{
> +  virtual void a (void *) = 0;
> +};
> +struct B
> +{
> +  virtual ~B () {};
> +  unsigned int b1;
> +  E **b2;
> +  A *b3;
> +};
> +struct C : public B
> +{
> +  ~C ();
> +};
> +C::~C ()
> +{
> +  for (unsigned int i = 0; i < b1; i++)
> +    b3->a (b2);
> +}
> +struct D
> +{
> +  ~D () {}
> +  C d;
> +};
> +struct F { virtual ~F () {}; };
> +struct G { void g (); };
> +struct H : public F
> +{
> +  virtual ~H ();
> +  D *h1;
> +  G *h2;
> +};
> +H::~H ()
> +{
> +  h2->g ();
> +  delete h1;
> +}
> --- gcc/testsuite/g++.dg/opt/vt4.C.jj	2013-04-04 14:33:35.684354321 +0200
> +++ gcc/testsuite/g++.dg/opt/vt4.C	2013-04-04 14:33:44.245304179 +0200
> @@ -0,0 +1,31 @@
> +// PR c++/50243
> +// { dg-do compile }
> +// { dg-options "-O" }
> +// { dg-final { scan-assembler-not "_ZTV.A" } }
> +
> +void foo ();
> +
> +struct A
> +{
> +  ~A () { }
> +  virtual void a () = 0;
> +  virtual void b () = 0;
> +  virtual void c () = 0;
> +};
> +
> +struct B : public A
> +{
> +  ~B () { foo (); }
> +  void a () { foo (); }
> +  void b () { foo (); }
> +  void c () { delete this; }
> +};
> +
> +void
> +test ()
> +{
> +  A *y = new B ();
> +  y->a ();
> +  y->b ();
> +  y->c ();
> +}
> 
> 	Jakub
> 
>

Patch

--- gcc/tree-eh.c.jj	2013-03-26 10:03:55.000000000 +0100
+++ gcc/tree-eh.c	2013-04-04 13:44:27.718982776 +0200
@@ -3230,14 +3230,48 @@  static void
 optimize_clobbers (basic_block bb)
 {
   gimple_stmt_iterator gsi = gsi_last_bb (bb);
+  bool any_clobbers = false;
+  bool seen_stack_restore = false;
+  edge_iterator ei;
+  edge e;
+
+  /* Only optimize anything if the bb contains at least one clobber,
+     ends with resx (checked by caller), optionally contains some
+     debug stmts or labels, or at most one __builtin_stack_restore
+     call, and has an incoming EH edge.  */
   for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       gimple stmt = gsi_stmt (gsi);
       if (is_gimple_debug (stmt))
 	continue;
-      if (!gimple_clobber_p (stmt)
-	  || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
-	return;
+      if (gimple_clobber_p (stmt))
+	{
+	  any_clobbers = true;
+	  continue;
+	}
+      if (!seen_stack_restore
+	  && gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE))
+	{
+	  seen_stack_restore = true;
+	  continue;
+	}
+      if (gimple_code (stmt) == GIMPLE_LABEL)
+	break;
+      return;
+    }
+  if (!any_clobbers)
+    return;
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    if (e->flags & EDGE_EH)
+      break;
+  if (e == NULL)
+    return;
+  gsi = gsi_last_bb (bb);
+  for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
+    {
+      gimple stmt = gsi_stmt (gsi);
+      if (!gimple_clobber_p (stmt))
+	continue;
       unlink_stmt_vdef (stmt);
       gsi_remove (&gsi, true);
       release_defs (stmt);
@@ -3278,8 +3312,7 @@  sink_clobbers (basic_block bb)
 	continue;
       if (gimple_code (stmt) == GIMPLE_LABEL)
 	break;
-      if (!gimple_clobber_p (stmt)
-	  || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
+      if (!gimple_clobber_p (stmt))
 	return 0;
       any_clobbers = true;
     }
@@ -3292,11 +3325,27 @@  sink_clobbers (basic_block bb)
   for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       gimple stmt = gsi_stmt (gsi);
+      tree lhs;
       if (is_gimple_debug (stmt))
 	continue;
       if (gimple_code (stmt) == GIMPLE_LABEL)
 	break;
       unlink_stmt_vdef (stmt);
+      lhs = gimple_assign_lhs (stmt);
+      /* Unfortunately we don't have dominance info updated at this
+	 point, so checking if
+	 dominated_by_p (CDI_DOMINATORS, succbb,
+			 gimple_bb (SSA_NAME_DEF_STMT (TREE_OPERAND (lhs, 0)))
+	 would be too costly.  Thus, avoid sinking any clobbers that
+	 refer to non-(D) SSA_NAMEs.  */
+      if (TREE_CODE (lhs) == MEM_REF
+	  && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME
+	  && !SSA_NAME_IS_DEFAULT_DEF (TREE_OPERAND (lhs, 0)))
+	{
+	  gsi_remove (&gsi, true);
+	  release_defs (stmt);
+	  continue;
+	}
       gsi_remove (&gsi, false);
       /* Trigger the operand scanner to cause renaming for virtual
          operands for this statement.
@@ -3737,10 +3786,10 @@  unsplit_eh (eh_landing_pad lp)
   edge e_in, e_out;
 
   /* Quickly check the edge counts on BB for singularity.  */
-  if (EDGE_COUNT (bb->preds) != 1 || EDGE_COUNT (bb->succs) != 1)
+  if (!single_pred_p (bb) || !single_succ_p (bb))
     return false;
-  e_in = EDGE_PRED (bb, 0);
-  e_out = EDGE_SUCC (bb, 0);
+  e_in = single_pred_edge (bb);
+  e_out = single_succ_edge (bb);
 
   /* Input edge must be EH and output edge must be normal.  */
   if ((e_in->flags & EDGE_EH) == 0 || (e_out->flags & EDGE_EH) != 0)
@@ -4142,7 +4191,7 @@  cleanup_empty_eh (eh_landing_pad lp)
       e_out = NULL;
       break;
     case 1:
-      e_out = EDGE_SUCC (bb, 0);
+      e_out = single_succ_edge (bb);
       break;
     default:
       return false;
--- gcc/tree-ssa-ccp.c.jj	2013-02-20 18:40:49.000000000 +0100
+++ gcc/tree-ssa-ccp.c	2013-04-04 14:01:08.926335910 +0200
@@ -2396,6 +2396,21 @@  execute_fold_all_builtins (void)
 
           if (gimple_code (stmt) != GIMPLE_CALL)
 	    {
+	      /* Remove all *ssaname_N ={v} {CLOBBER}; stmts,
+		 after the last GIMPLE DSE they aren't needed and might
+		 unnecessarily keep the SSA_NAMEs live.  */
+	      if (gimple_clobber_p (stmt))
+		{
+		  tree lhs = gimple_assign_lhs (stmt);
+		  if (TREE_CODE (lhs) == MEM_REF
+		      && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME)
+		    {
+		      unlink_stmt_vdef (stmt);
+		      gsi_remove (&i, true);
+		      release_defs (stmt);
+		      continue;
+		    }
+		}
 	      gsi_next (&i);
 	      continue;
 	    }
--- gcc/testsuite/g++.dg/opt/vt3.C.jj	2013-04-04 14:33:12.190484331 +0200
+++ gcc/testsuite/g++.dg/opt/vt3.C	2013-04-04 14:32:19.000000000 +0200
@@ -0,0 +1,43 @@ 
+// PR c++/34949
+// { dg-do compile }
+// { dg-options "-O3" }
+
+struct E {};
+struct A
+{
+  virtual void a (void *) = 0;
+};
+struct B
+{
+  virtual ~B () {};
+  unsigned int b1;
+  E **b2;
+  A *b3;
+};
+struct C : public B
+{
+  ~C ();
+};
+C::~C ()
+{
+  for (unsigned int i = 0; i < b1; i++)
+    b3->a (b2);
+}
+struct D
+{
+  ~D () {}
+  C d;
+};
+struct F { virtual ~F () {}; };
+struct G { void g (); };
+struct H : public F
+{
+  virtual ~H ();
+  D *h1;
+  G *h2;
+};
+H::~H ()
+{
+  h2->g ();
+  delete h1;
+}
--- gcc/testsuite/g++.dg/opt/vt4.C.jj	2013-04-04 14:33:35.684354321 +0200
+++ gcc/testsuite/g++.dg/opt/vt4.C	2013-04-04 14:33:44.245304179 +0200
@@ -0,0 +1,31 @@ 
+// PR c++/50243
+// { dg-do compile }
+// { dg-options "-O" }
+// { dg-final { scan-assembler-not "_ZTV.A" } }
+
+void foo ();
+
+struct A
+{
+  ~A () { }
+  virtual void a () = 0;
+  virtual void b () = 0;
+  virtual void c () = 0;
+};
+
+struct B : public A
+{
+  ~B () { foo (); }
+  void a () { foo (); }
+  void b () { foo (); }
+  void c () { delete this; }
+};
+
+void
+test ()
+{
+  A *y = new B ();
+  y->a ();
+  y->b ();
+  y->c ();
+}