Re: [PATCH] Allow MEM_REF lhs on gimple_clobber_p stmts (PR c++/34949)

[Richard Biener <rguenther at suse dot de>]

On Tue, 2 Apr 2013, Jakub Jelinek wrote:

> Hi!
> 
> Jason's http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34949#c12
> patch attempts to emit a clobber for *this at the end of the destructor,
> so that we can DSE stores into the object when the object is dead, but
> so far only VAR_DECLs have been allowed on the lhs of gimple_clobber_p
> stmts.
> This incremental patch allows there either a VAR_DECL, or MEM_REF.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Few comments below

> (Jason's patch can be just committed as the last thing in the series).
> 
> 2013-04-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/34949
> 	* tree-cfg.c (verify_gimple_assign_single): Allow lhs
> 	of gimple_clobber_p to be MEM_REF.
> 	* gimplify.c (gimplify_modify_expr): Gimplify *to_p of
> 	an assignment from TREE_CLOBBER_P.  Allow it to be MEM_REF
> 	after gimplification.
> 	* asan.c (get_mem_ref_of_assignment): Don't instrument
> 	gimple_clobber_p stmts.
> 	* tree-ssa-dse.c (dse_optimize_stmt): Allow DSE of
> 	gimple_clobber_p stmt if they have MEM_REF lhs and
> 	are dead because of another gimple_clobber_p stmt.
> 	* tree-ssa-live.c (clear_unused_block_pointer): Treat
> 	gimple_clobber_p stmts like debug stmts.
> 	(remove_unused_locals): Remove clobbers with MEM_REF lhs
> 	that refer to unused VAR_DECLs or uninitialized values.
> 	* tree-sra.c (sra_ipa_reset_debug_stmts): Also remove
> 	gimple_clobber_p stmts if they refer to removed parameters.
> 	(get_repl_default_def_ssa_name, sra_ipa_modify_expr): Fix up
> 	formatting.
> 
> --- gcc/tree-cfg.c.jj	2013-03-21 18:34:11.000000000 +0100
> +++ gcc/tree-cfg.c	2013-03-27 13:44:59.475007635 +0100
> @@ -3812,9 +3812,9 @@ verify_gimple_assign_single (gimple stmt
>      }
>  
>    if (gimple_clobber_p (stmt)
> -      && !DECL_P (lhs))
> +      && !(DECL_P (lhs) || TREE_CODE (lhs) == MEM_REF))
>      {
> -      error ("non-decl LHS in clobber statement");
> +      error ("non-decl/MEM_REF LHS in clobber statement");
>        debug_generic_expr (lhs);
>        return true;
>      }
> --- gcc/gimplify.c.jj	2013-03-21 18:34:11.000000000 +0100
> +++ gcc/gimplify.c	2013-03-27 13:38:30.988181267 +0100
> @@ -4840,7 +4840,12 @@ gimplify_modify_expr (tree *expr_p, gimp
>       so handle it here.  */
>    if (TREE_CLOBBER_P (*from_p))
>      {
> -      gcc_assert (!want_value && TREE_CODE (*to_p) == VAR_DECL);
> +      ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
> +      if (ret == GS_ERROR)
> +	return ret;
> +      gcc_assert (!want_value
> +		  && (TREE_CODE (*to_p) == VAR_DECL
> +		      || TREE_CODE (*to_p) == MEM_REF));
>        gimplify_seq_add_stmt (pre_p, gimple_build_assign (*to_p, *from_p));
>        *expr_p = NULL;
>        return GS_ALL_DONE;
> --- gcc/asan.c.jj	2013-02-28 22:22:03.000000000 +0100
> +++ gcc/asan.c	2013-03-27 14:29:54.531785577 +0100
> @@ -412,7 +412,8 @@ get_mem_ref_of_assignment (const gimple
>  {
>    gcc_assert (gimple_assign_single_p (assignment));
>  
> -  if (gimple_store_p (assignment))
> +  if (gimple_store_p (assignment)
> +      && !gimple_clobber_p (assignment))

hmm, maybe gimple_store_p should not consider a clobber a store?

>      {
>        ref->start = gimple_assign_lhs (assignment);
>        *ref_is_store = true;
> --- gcc/tree-ssa-dse.c.jj	2013-01-11 09:02:37.000000000 +0100
> +++ gcc/tree-ssa-dse.c	2013-03-27 17:14:27.424466023 +0100
> @@ -218,7 +218,10 @@ dse_optimize_stmt (gimple_stmt_iterator
>    if (is_gimple_call (stmt) && gimple_call_fndecl (stmt))
>      return;
>  
> -  if (gimple_has_volatile_ops (stmt))
> +  /* Don't return early on *this_2(D) ={v} {CLOBBER}.  */
> +  if (gimple_has_volatile_ops (stmt)
> +      && (!gimple_clobber_p (stmt)
> +	  || TREE_CODE (gimple_assign_lhs (stmt)) != MEM_REF))
>      return;

But this only means the clobber was not considered as "dead"
if there was a later store to the same location.  So, why would
that change be needed?

>    if (is_gimple_assign (stmt))
> @@ -228,6 +231,12 @@ dse_optimize_stmt (gimple_stmt_iterator
>        if (!dse_possible_dead_store_p (stmt, &use_stmt))
>  	return;
>  
> +      /* But only remove *this_2(D) ={v} {CLOBBER} if killed by
> +	 another clobber stmt.  */
> +      if (gimple_clobber_p (stmt)
> +	  && !gimple_clobber_p (use_stmt))
> +	return;

Ah.  Hmm, can that ever happen?  I presume the issue with non-decl
clobbers is that we will never remove them, even if the storage
becomes "dead"?

>        /* If we have precisely one immediate use at this point and the
>  	 stores are to the same memory location or there is a chain of
>  	 virtual uses from stmt and the stmt which stores to that same
> --- gcc/tree-ssa-live.c.jj	2013-03-21 18:34:11.000000000 +0100
> +++ gcc/tree-ssa-live.c	2013-03-28 11:10:15.889669796 +0100
> @@ -623,8 +623,8 @@ clear_unused_block_pointer_1 (tree *tp,
>    return NULL_TREE;
>  }
>  
> -/* Set all block pointer in debug stmt to NULL if the block is unused,
> -   so that they will not be streamed out.  */
> +/* Set all block pointer in debug or clobber stmt to NULL if the block
> +   is unused, so that they will not be streamed out.  */
>  
>  static void
>  clear_unused_block_pointer (void)
> @@ -639,7 +639,7 @@ clear_unused_block_pointer (void)
>  	tree b;
>  	gimple stmt = gsi_stmt (gsi);
>  
> -	if (!is_gimple_debug (stmt))
> +	if (!is_gimple_debug (stmt) && !gimple_clobber_p (stmt))
>  	  continue;
>  	b = gimple_block (stmt);
>  	if (b && !TREE_USED (b))
> @@ -827,7 +827,26 @@ remove_unused_locals (void)
>  	    if (gimple_clobber_p (stmt))
>  	      {
>  		tree lhs = gimple_assign_lhs (stmt);
> +		bool remove = false;
> +		/* Remove clobbers referencing unused vars, or clobbers
> +		   with MEM_REF lhs referencing unused vars or uninitialized
> +		   pointers.  */
>  		if (TREE_CODE (lhs) == VAR_DECL && !is_used_p (lhs))
> +		  remove = true;
> +		else if (TREE_CODE (lhs) == MEM_REF)
> +		  {
> +		    ssa_op_iter iter;
> +		    tree op;
> +		    FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)

The only SSA use on a clobber stmt can possibly be TREE_OPERAND (lhs, 0)
when lhs is a MEM_REF, no need to use FOR_EACH_SSA_TREE_OPERAND.

So just 

> +             else if (TREE_CODE (lhs) == MEM_REF
                         && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME)
...

> +		      if (SSA_NAME_VAR (op)
> +			  && TREE_CODE (SSA_NAME_VAR (op)) == VAR_DECL
> +			  && !is_used_p (SSA_NAME_VAR (op)))
> +			remove = true;

I'm not sure this is really desired ... isn't a better check to do
has_single_use (op)?  (which means it would be better suited to
be handled in DCE?)

Btw, due to propagation you can end up with MEM[&decl] = {CLOBBER};
which should be handled the same as the VAR_DECL case.  Eventually
just use lhs = get_base_address (gimple_assign_lhs (stmt)) here.

> +		      else if (SSA_NAME_IS_DEFAULT_DEF (op)
> +			       && TREE_CODE (SSA_NAME_VAR (op)) != PARM_DECL)
> +			remove = true;
> +		  }
> +		if (remove)
>  		  {
>  		    unlink_stmt_vdef (stmt);
>  		    gsi_remove (&gsi, true);
> --- gcc/tree-sra.c.jj	2013-03-21 18:34:11.000000000 +0100
> +++ gcc/tree-sra.c	2013-04-02 09:44:29.225462112 +0200
> @@ -2965,8 +2965,8 @@ sra_modify_constructor_assign (gimple *s
>  static tree
>  get_repl_default_def_ssa_name (struct access *racc)
>  {
> -  gcc_checking_assert (!racc->grp_to_be_replaced &&
> -		       !racc->grp_to_be_debug_replaced);
> +  gcc_checking_assert (!racc->grp_to_be_replaced
> +		       && !racc->grp_to_be_debug_replaced);
>    if (!racc->replacement_decl)
>      racc->replacement_decl = create_access_replacement (racc);
>    return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
> @@ -4462,8 +4462,8 @@ sra_ipa_modify_expr (tree *expr, bool co
>      {
>        adj = &adjustments[i];
>  
> -      if (adj->base == base &&
> -	  (adj->offset == offset || adj->remove_param))
> +      if (adj->base == base
> +	  && (adj->offset == offset || adj->remove_param))
>  	{
>  	  cand = adj;
>  	  break;
> @@ -4676,6 +4676,14 @@ sra_ipa_reset_debug_stmts (ipa_parm_adju
>        if (name)
>  	FOR_EACH_IMM_USE_STMT (stmt, ui, name)
>  	  {
> +	    if (gimple_clobber_p (stmt))
> +	      {
> +		gimple_stmt_iterator cgsi = gsi_for_stmt (stmt);
> +		unlink_stmt_vdef (stmt);
> +		gsi_remove (&cgsi, true);
> +		release_defs (stmt);
> +		continue;
> +	      }
>  	    /* All other users must have been removed by
>  	       ipa_sra_modify_function_body.  */
>  	    gcc_assert (is_gimple_debug (stmt));

Otherwise the patch looks fine to me.

Thanks,
Richard.