Re: indirect_ref_may_alias_decl_p fix

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 7568 bytes --]

On Thu, 13 Jun 2019, Jan Hubicka wrote:

> Hi,
> after spending some time on the view converting MEM_REFs, I noticed that
> most of reasons we give up on view converts is not actually MEM_REF created
> but test
>  same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2))
> in indirect_ref_may_alias_decl_p
> 
> Here base2 is VAR_DECL while dbase2 is MEM_REF used to access it.
> 
> In the testcase:
> struct a {int a1; int a2;};
> struct b:a {};
> 
> struct b bvar,*bptr2;
> int
> test(void)
> {
>   struct a *bptr = &bvar;
>   bptr->a2=0;
>   bptr2->a1=1;
>   return bptr->a2;
> }
> 
> We have variable of type b, while we access it via its basetype.
> This mean that TREE_TYPE (base) is "struct b" while TREE_TYPE (dbase)
> is "struct a" which is perfectly normal and we could to the access path
> tests at least in the same strength as we would do if bptr=$bvar was 
> not visible to compiler (in that case we optimize things correctly).
> 
> Of course later in aliasing_component_refs_p we should not assume that
> "struct a" is the type of memory slot since the access path may contain
> b, but I think we can not assume that about "struct b" either, see below.
> 
> We should not give up on this case and just proceed the same way as
> indirect_refs_may_alias_p does.  In fact I would like to commonize the
> access path oracle of these functions incremetally but first I want to 
> drop main differences. In particular
> 
>  1) indirect_refs_may_alias_decl_p passing ref2_is_decl as true to 
>     aliasing_component_refs_p.
> 
>     This makes aliasing_component_refs_p to assume that all access paths
>     conflicting with REF2 must start by type of BASE2 or its subtype.
> 
>     IMO this is not quite right in gimple memory model where decls are just
>     untyped memory slots, since I can, for example, I can rewrite decl
>     of type a by a new data of type struct b {struct a a;};
>     which will confuse this logic.

The above check you complain about guards against this.

>     I will try to get rid of this incrementally - I would like to have it
>     logged how much optimization we lose here.
> 
>  2) indirect_refs_may_alias_decl_p does
> 
>   if ((TREE_CODE (base1) != TARGET_MEM_REF                                      
>        || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1)))                          
>       && same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (dbase2)) == 1        
>       && (TREE_CODE (TREE_TYPE (base1)) != ARRAY_TYPE                           
>           || (TYPE_SIZE (TREE_TYPE (base1))                                     
>               && TREE_CODE (TYPE_SIZE (TREE_TYPE (base1))) == INTEGER_CST)))    
>     return ranges_maybe_overlap_p (doffset1, max_size1, doffset2, max_size2);   
> 
>      while indirect_refs_may_alias_p does:
> 
>   if ((TREE_CODE (base1) != TARGET_MEM_REF                                      
>        || (!TMR_INDEX (base1) && !TMR_INDEX2 (base1)))                          
>       && (TREE_CODE (base2) != TARGET_MEM_REF                                   
>           || (!TMR_INDEX (base2) && !TMR_INDEX2 (base2)))                       
>       && same_type_for_tbaa (TREE_TYPE (ptrtype1),                              
>                              TREE_TYPE (ptrtype2)) == 1                         
>       /* But avoid treating arrays as "objects", instead assume they            
>          can overlap by an exact multiple of their element size.                
>          See gcc.dg/torture/alias-2.c.  */                                      
>       && TREE_CODE (TREE_TYPE (ptrtype1)) != ARRAY_TYPE)                        
>     return ranges_maybe_overlap_p (offset1, max_size1, offset2, max_size2);     
> 
>      Sincce we already checked that TREEE_TYPE (ptrtype) is same as TREE_TYPE (base1)
>      the same_type_for_tbaa check is equivalent in both.
> 
>      Notice however that first tests that array is VLA, while other
>      supports partial overlaps on all array types.  I suppose we want to
>      choose which way we support that and go with one or another.

Let's go with the stricter check for the purpose of unification and work
on this issue as followup.

>      Of course even in that case overlap check is not completely lost,
>      I attached testcase for that to
>      https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90869
>      All we need is to wrap the checks by array size.
> 
>   With these differences sorted out I think both functions may dispatch to
>   common access path oracle after doing the case specific work.
> 
> Bootstrapped/regtested x86_64-linux, makes sense?

Yes, the patch below makes sense.

Thanks,
Richard.

> 	PR tree-optimize/90869
> 	* tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Watch for view
> 	converts in MEM_REF referencing decl rather than view converts
> 	from decl type to MEM_REF type.
> 
> 	* g++.dg/tree-ssa/alias-access-path-1.C: New testcase.
> Index: tree-ssa-alias.c
> ===================================================================
> --- tree-ssa-alias.c	(revision 272037)
> +++ tree-ssa-alias.c	(working copy)
> @@ -1370,11 +1410,16 @@ indirect_ref_may_alias_decl_p (tree ref1
>    poly_offset_int doffset2 = offset2;
>    if (TREE_CODE (dbase2) == MEM_REF
>        || TREE_CODE (dbase2) == TARGET_MEM_REF)
> -    doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT;
> +    {
> +      doffset2 -= mem_ref_offset (dbase2) << LOG2_BITS_PER_UNIT;
> +      tree ptrtype2 = TREE_TYPE (TREE_OPERAND (dbase2, 1));
> +      /* If second reference is view-converted, give up now.  */
> +      if (same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (ptrtype2)) != 1)
> +	return true;
> +    }
>  
> -  /* If either reference is view-converted, give up now.  */
> -  if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1
> -      || same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) != 1)
> +  /* If first reference is view-converted, give up now.  */
> +  if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1)
>      return true;
>  
>    /* If both references are through the same type, they do not alias
> @@ -1408,7 +1457,13 @@ indirect_ref_may_alias_decl_p (tree ref1
>  				      offset1, max_size1,
>  				      ref2,
>  				      ref2_alias_set, base2_alias_set,
> -				      offset2, max_size2, true);
> +				      offset2, max_size2, 
> +				      /* Only if the other reference is actual
> +					 decl we can safely check only toplevel
> +					 part of access path 1.  */
> +				      same_type_for_tbaa (TREE_TYPE (dbase2),
> +					                  TREE_TYPE (base2))
> +				      == 1);
>  
>    return true;
>  }
> Index: testsuite/g++.dg/tree-ssa/alias-access-path-1.C
> ===================================================================
> --- testsuite/g++.dg/tree-ssa/alias-access-path-1.C	(nonexistent)
> +++ testsuite/g++.dg/tree-ssa/alias-access-path-1.C	(working copy)
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -fdump-tree-fre1" } */
> +
> +struct a {int a1; int a2;};
> +struct b:a {};
> +
> +struct b bvar,*bptr2;
> +int
> +test(void)
> +{
> +  struct a *bptr = &bvar;
> +  bptr->a2=0;
> +  bptr2->a1=1;
> +  return bptr->a2;
> +}
> +int
> +test2(void)
> +{
> +  struct b *bptr = &bvar;
> +  bptr->a2=0;
> +  bptr2->a1=1;
> +  return bptr->a2;
> +}
> +/* { dg-final { scan-tree-dump-times "return 0" 2 "fre1" } } */
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG NÌrnberg)