Re: [PATCH] Fix profiledbootstrap - store-merging aliasing issue (PR bootstrap/82916)

[Richard Biener <rguenther at suse dot de>]

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

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