Re: [PATCH] Fix wrong-code aggregate propagate_with_phi bug (PR tree-optimization/81365)

[Richard Biener <rguenther at suse dot de>]

On Thu, 13 Jul 2017, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, for aggregate copies we fail to verify there
> are no loads in between the PHI and the aggregate copy that could alias with the
> lhs of the copy, which is needed, because we want to hoist the aggregate
> copy onto predecessor edges of the bb with the PHI.
> 
> The following patch implements that, bootstrapped/regtested on x86_64-linux
> and i686-linux, ok for trunk and after a while 7.x?
> 
> 2017-07-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/81365
> 	* tree-ssa-phiprop.c (propagate_with_phi): When considering hoisting
> 	aggregate moves onto bb predecessor edges, make sure there are no
> 	loads that could alias the lhs in between the start of bb and the
> 	loads from *phi.  If there are any debug stmts that could alias,
> 	reset them.
> 
> 	* g++.dg/torture/pr81365.C: New test.
> 
> --- gcc/tree-ssa-phiprop.c.jj	2017-05-22 10:50:11.000000000 +0200
> +++ gcc/tree-ssa-phiprop.c	2017-07-11 16:52:41.012340615 +0200
> @@ -327,7 +327,7 @@ propagate_with_phi (basic_block bb, gphi
>        if (!dominated_by_p (CDI_POST_DOMINATORS,
>  			   bb, gimple_bb (use_stmt)))
>  	continue;
> -         
> +
>        /* Check whether this is a load of *ptr.  */
>        if (!(is_gimple_assign (use_stmt)
>  	    && gimple_assign_rhs_code (use_stmt) == MEM_REF
> @@ -356,6 +356,9 @@ propagate_with_phi (basic_block bb, gphi
>           insert aggregate copies on the edges instead.  */
>        if (!is_gimple_reg_type (TREE_TYPE (TREE_TYPE (ptr))))
>  	{
> +	  if (!gimple_vdef (use_stmt))
> +	    goto next;
> +
>  	  /* As we replicate the lhs on each incoming edge all
>  	     used SSA names have to be available there.  */
>  	  if (! for_each_index (gimple_assign_lhs_ptr (use_stmt),
> @@ -363,6 +366,51 @@ propagate_with_phi (basic_block bb, gphi
>  				get_immediate_dominator (CDI_DOMINATORS,
>  							 gimple_bb (phi))))
>  	    goto next;
> +
> +	  gimple *vuse_stmt;
> +	  imm_use_iterator vui;
> +	  use_operand_p vuse_p;
> +	  bool debug_use_seen = false;
> +	  /* In order to move the aggregate copies earlier, make sure
> +	     there are no statements that could read from memory
> +	     aliasing the lhs in between the start of bb and use_stmt.
> +	     As we require use_stmt to have a VDEF above, loads after
> +	     use_stmt will use a different virtual SSA_NAME.  */
> +	  FOR_EACH_IMM_USE_FAST (vuse_p, vui, vuse)
> +	    {
> +	      vuse_stmt = USE_STMT (vuse_p);
> +	      if (vuse_stmt == use_stmt)
> +		continue;
> +	      if (!dominated_by_p (CDI_DOMINATORS,
> +				   gimple_bb (vuse_stmt), bb))
> +		continue;
> +	      if (ref_maybe_used_by_stmt_p (vuse_stmt,
> +					    gimple_assign_lhs (use_stmt)))
> +		{
> +		  if (is_gimple_debug (vuse_stmt))

debug stmts do not have virtual operands so their handling is not
necessary here.

Ok with that change.

Thanks,
Richard.


> +		    debug_use_seen = true;
> +		  else
> +		    goto next;
> +		}
> +	    }
> +	  /* Debug stmt uses should not prevent the transformation, but
> +	     if we saw any, reset those debug stmts.  */
> +	  if (debug_use_seen)
> +	    FOR_EACH_IMM_USE_STMT (vuse_stmt, vui, vuse)
> +	      {
> +		if (!is_gimple_debug (vuse_stmt))
> +		  continue;
> +		if (!dominated_by_p (CDI_DOMINATORS,
> +				     gimple_bb (vuse_stmt), bb))
> +		  continue;
> +		if (ref_maybe_used_by_stmt_p (vuse_stmt,
> +					      gimple_assign_lhs (use_stmt)))
> +		  {
> +		    gimple_debug_bind_reset_value (vuse_stmt);
> +		    update_stmt (vuse_stmt);
> +		  }
> +	      }
> +
>  	  phiprop_insert_phi (bb, phi, use_stmt, phivn, n);
>  
>  	  /* Remove old stmt.  The phi is taken care of by DCE.  */
> --- gcc/testsuite/g++.dg/torture/pr81365.C.jj	2017-07-11 17:07:11.107130111 +0200
> +++ gcc/testsuite/g++.dg/torture/pr81365.C	2017-07-11 17:06:52.000000000 +0200
> @@ -0,0 +1,39 @@
> +// PR tree-optimization/81365
> +// { dg-do run }
> +
> +struct A { unsigned a; };
> +
> +struct B {
> +  B (const A *x)
> +  {
> +    __builtin_memcpy (b, x, 3 * sizeof (A));
> +    __builtin_memcpy (c, x + 3, sizeof (A));
> +    __builtin_memset (c + 1, 0, sizeof (A));
> +  }
> +  bool
> +  foo (unsigned x)
> +  {
> +    A *it = c;
> +    if (it->a == x || (++it)->a == x)
> +      {
> +	A t(b[0]);
> +	b[0] = *it;
> +	*it = t;
> +	return true;
> +      }
> +    return false;
> +  }
> +  A b[3];
> +  A c[2];
> +};
> +
> +int
> +main ()
> +{
> +  A x[] = { 4, 8, 12, 18 };
> +  B y(x);
> +  if (!y.foo (18))
> +    __builtin_abort ();
> +  if (!y.foo (4))
> +    __builtin_abort ();
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)