Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)

Hi!

We want to terminate a chain if a chain with different base (or insn
outside of any chain) has a store that the current stmt might use, or
overwrite.  The functions it used didn't cover the store after store
case which in the middle-end aliasing model needs to avoid tbaa, because
the latter store might be after a placement new or might otherwise change
the dynamic object type of the object.

The following patch does that.  Bootstrapped/regtested on x86_64-linux and
i686-linux, additionally profiledbootstrapped with the configure options
Markus mentioned in the PR.  Ok for trunk?

2017-11-09  Jakub Jelinek  <jakub@redhat.com>

	PR bootstrap/82916
	* gimple-ssa-store-merging.c
	(pass_store_merging::terminate_all_aliasing_chains): For
	gimple_store_p stmts also call refs_output_dependent_p.

	* gcc.dg/store_merging_2.c: Only expect 2 successful mergings instead
	of 3.
	* gcc.dg/pr82916.c: New test.


	Jakub

On Thu, 9 Nov 2017, Jakub Jelinek wrote:

> Hi!
> 
> We want to terminate a chain if a chain with different base (or insn
> outside of any chain) has a store that the current stmt might use, or
> overwrite.  The functions it used didn't cover the store after store
> case which in the middle-end aliasing model needs to avoid tbaa, because
> the latter store might be after a placement new or might otherwise change
> the dynamic object type of the object.
> 
> The following patch does that.  Bootstrapped/regtested on x86_64-linux and
> i686-linux, additionally profiledbootstrapped with the configure options
> Markus mentioned in the PR.  Ok for trunk?
> 
> 2017-11-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR bootstrap/82916
> 	* gimple-ssa-store-merging.c
> 	(pass_store_merging::terminate_all_aliasing_chains): For
> 	gimple_store_p stmts also call refs_output_dependent_p.
> 
> 	* gcc.dg/store_merging_2.c: Only expect 2 successful mergings instead
> 	of 3.
> 	* gcc.dg/pr82916.c: New test.
> 
> --- gcc/gimple-ssa-store-merging.c.jj	2017-11-09 15:51:08.000000000 +0100
> +++ gcc/gimple-ssa-store-merging.c	2017-11-09 18:01:20.789236951 +0100
> @@ -945,6 +945,7 @@ pass_store_merging::terminate_all_aliasi
>    if (!gimple_vuse (stmt))
>      return false;
>  
> +  tree store_lhs = gimple_store_p (stmt) ? gimple_get_lhs (stmt) : NULL_TREE;
>    for (imm_store_chain_info *next = m_stores_head, *cur = next; cur; cur = next)
>      {
>        next = cur->next;
> @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
>        unsigned int i;
>        FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
>  	{
> -	  if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> -	      || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> +	  tree lhs = gimple_assign_lhs (info->stmt);
> +	  if (ref_maybe_used_by_stmt_p (stmt, lhs)
> +	      || stmt_may_clobber_ref_p (stmt, lhs)
> +	      || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))

Looks good but may do redundant work for store_lhs?  So rather

   || (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
   || (store_lhs && refs_output_dependent_p (store_lhs, lhs)

?  Fails to handle storing calls (in case those can appear in the chains).
Looks like we miss some convenient stmt_output/anti_dependent_p (you can
follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).

That said - the patch is ok, any improvements can be done as followup.

Thanks,
Richard.

>  	    {
>  	      if (dump_file && (dump_flags & TDF_DETAILS))
>  		{
> --- gcc/testsuite/gcc.dg/store_merging_2.c.jj	2017-11-08 16:46:19.000000000 +0100
> +++ gcc/testsuite/gcc.dg/store_merging_2.c	2017-11-09 18:16:33.482344795 +0100
> @@ -77,4 +77,4 @@ main (void)
>    return 0;
>  }
>  
> -/* { dg-final { scan-tree-dump-times "Merging successful" 3 "store-merging" } } */
> +/* { dg-final { scan-tree-dump-times "Merging successful" 2 "store-merging" } } */
> --- gcc/testsuite/gcc.dg/pr82916.c.jj	2017-11-09 18:12:40.707128841 +0100
> +++ gcc/testsuite/gcc.dg/pr82916.c	2017-11-09 18:12:19.000000000 +0100
> @@ -0,0 +1,47 @@
> +/* PR bootstrap/82916 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fno-tree-dse" } */
> +
> +struct A { struct A *next; };
> +struct C
> +{
> +  int *of;
> +  struct C *parent, *prev, *next;
> +  int depth;
> +  int min;
> +  struct C *min_occ;
> +};
> +
> +__attribute__((noipa)) struct C *
> +foo (int *node)
> +{
> +  struct A *p = __builtin_malloc (sizeof (struct C));
> +  if (!p)
> +    return 0;
> +  p->next = 0;
> +  /* Originally placement new.  */
> +  struct C *nw = (struct C *)(void *)p;
> +  nw->of = node;
> +  nw->parent = 0;
> +  nw->prev = 0;
> +  nw->next = 0;
> +  nw->depth = 0;
> +  nw->min_occ = nw;
> +  nw->min = 0;
> +  return nw;
> +}
> +
> +int
> +main ()
> +{
> +  int o;
> +  struct C *p = foo (&o);
> +  if (p)
> +    {
> +      if (p->of != &o || p->parent || p->prev || p->next || p->depth
> +	  || p->min || p->min_occ != p)
> +	__builtin_abort ();
> +    }
> +  __builtin_free (p);
> +  return 0;
> +}
> 
> 	Jakub
> 
>

On Fri, Nov 10, 2017 at 08:52:16AM +0100, Richard Biener wrote:
> > @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
> >        unsigned int i;
> >        FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
> >  	{
> > -	  if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> > -	      || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> > +	  tree lhs = gimple_assign_lhs (info->stmt);
> > +	  if (ref_maybe_used_by_stmt_p (stmt, lhs)
> > +	      || stmt_may_clobber_ref_p (stmt, lhs)
> > +	      || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
> 
> Looks good but may do redundant work for store_lhs?  So rather
> 
>    || (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
>    || (store_lhs && refs_output_dependent_p (store_lhs, lhs)
> 
> ?  Fails to handle storing calls (in case those can appear in the chains).

info->stmt is known to be a store, but stmt is not, it can be any other
stmt, including calls, so the above would miss the calls handling.

> Looks like we miss some convenient stmt_output/anti_dependent_p (you can
> follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
> bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).

So perhaps bool tbaa = true argument to both stmt_may_clobber_ref_p_1
and stmt_may_clobber_ref_p, or just stmt_may_clobber_ref_p_1 and
add some differently named alternative to stmt_may_clobber_ref_p
(in that case, any suggestions on a good name?)?

> That said - the patch is ok, any improvements can be done as followup.

	Jakub

On Fri, 10 Nov 2017, Jakub Jelinek wrote:

> On Fri, Nov 10, 2017 at 08:52:16AM +0100, Richard Biener wrote:
> > > @@ -958,8 +959,10 @@ pass_store_merging::terminate_all_aliasi
> > >        unsigned int i;
> > >        FOR_EACH_VEC_ELT (cur->m_store_info, i, info)
> > >  	{
> > > -	  if (ref_maybe_used_by_stmt_p (stmt, gimple_assign_lhs (info->stmt))
> > > -	      || stmt_may_clobber_ref_p (stmt, gimple_assign_lhs (info->stmt)))
> > > +	  tree lhs = gimple_assign_lhs (info->stmt);
> > > +	  if (ref_maybe_used_by_stmt_p (stmt, lhs)
> > > +	      || stmt_may_clobber_ref_p (stmt, lhs)
> > > +	      || (store_lhs && refs_output_dependent_p (store_lhs, lhs)))
> > 
> > Looks good but may do redundant work for store_lhs?  So rather
> > 
> >    || (! store_lhs && stmt_may_clobber_ref_p (stmt, lhs)
> >    || (store_lhs && refs_output_dependent_p (store_lhs, lhs)
> > 
> > ?  Fails to handle storing calls (in case those can appear in the chains).
> 
> info->stmt is known to be a store, but stmt is not, it can be any other
> stmt, including calls, so the above would miss the calls handling.
> 
> > Looks like we miss some convenient stmt_output/anti_dependent_p (you can
> > follow stmt_may_clobbers_ref_p[_1] for cut&pasting and/or add a
> > bool tbaa flag we can pass down to stmt_may_clobber_ref_p_1).
> 
> So perhaps bool tbaa = true argument to both stmt_may_clobber_ref_p_1
> and stmt_may_clobber_ref_p, or just stmt_may_clobber_ref_p_1 and
> add some differently named alternative to stmt_may_clobber_ref_p
> (in that case, any suggestions on a good name?)?

Internally (aka static fn) I'd just add a bool param w/o default.

The external API should be stmt_output/anti_dependent_p and I think
these days with C++ we could make stmt_may_clobber_ref_p_1 taking ao_ref
and stmt_may_clobber_ref_p taking a tree overloads of
stmt_may_clobber_ref_p.  We'd then have _1 being static and having the
extra arg.

Richard.

> > That said - the patch is ok, any improvements can be done as followup.
> 
> 	Jakub
> 
>