Re: [PATCH] MEM_REF clobber handling fixes/improvements (PR c++/34949, PR c++/50243)

[Richard Biener <rguenther at suse dot de>]

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
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend