Re: Enable pointer TBAA for LTO

[Richard Biener <rguenther@suse.de>]

On Tue, 24 Nov 2015, Jan Hubicka wrote:

> > On November 23, 2015 5:50:25 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote:
> > >> 
> > >> I think it also causes the following and one related ICE
> > >> 
> > >> FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal
> > >compiler 
> > >> error)
> > >> 
> > >>
> > >/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1:
> > >
> > >> internal compiler error: in get_alias_set, at alias.c:880^M
> > >> 0x7528a7 get_alias_set(tree_node*)^M
> > >>         /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M
> > >
> > >Does this one reproduce with mainline?
> > 
> > Yes, testsuite run on x86_64
> > 
> >  All thee ICEs are on the sanity
> > >check:
> > >gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t));
> > >which check that in LTO all types that ought to have CANONICAL_TYPE
> > >gets CANONICAL_TYPE
> > >computed.  ICE here probalby means that the type somehow bypassed LTO
> > >canonical type merging
> > >as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c
> 
> Hi,
> all the ICE seems to be caused by fact that the simd streaming code creates
> array of pointers at ltrans time. Because pointers are now
> TYPE_STRUCTURAL_EQUALITY_P, we get array with TYPE_STRUCTURAL_EQUALITY_P
> and that sanity check is there to verify that this doesn't happen (because
> we lose optimization then)
> 
> This patch makes arrays and vectors to be handled same way as pointers:
> the canonical type is not computed because it is unused by get_alias_set
> anyway.
> 
> I implemented it for different reason - my TBAA checking in lto-symtab got
> false positives on arrays during the LTO bootstrap.  THe problem was again
> the overly generous globbing of TYPE_CANONICAL.
> 
> get_alias_set first does
> 
>  t = TYPE_CANONICAL (t)
> 
> and then for arrays recurses
> 
>  set = get_alias_set (TREE_TYPE (t));
> 
> Now we can have type
> 
>  int *[10];
> 
> with TYPE_CANONICAL
> 
>  float *[10];
> 
> and then get_alias_set (int *[10]) will return get_alias_set (float *)
> which is wrong, because then the array does not conflict with its elements.
> I did not managed to get any wrong code out of this, but it is an obvious bug.
> This problem reproduced prior the pointer patch, too, on other types which are
> globed at TYPE_CANONICAL computation.
> 
> This also fixeds semi-latent bug with Ada where TYPE_STRUCTURAL_EQUALITY_P type
> may win the merging and turn all interoperable arrays into
> TYPE_STRUCTURAL_EQUALITY_P.
> 
> For next stage1 I think I can move frontends away from computing TYPE_CANONICAL
> for those types. At least my initial test for C and C++ seemed to work just fine.
> 
> Bootstrapped/regtested x86_64-linux, OK?

I don't like the in_lto_p checks in tree.c but as you say they'll be 
removed next stage1 ok.

Thanks,
Richard.

> 	* lto-streamer-in.c (lto_read_body_or_constructor): Set TYPE_CANONICAL
> 	only for types where LTO sets them.
> 	* tree.c (build_array_type_1): Do ont set TYPE_CANONICAL for LTO.
> 	(make_vector_type): Likewise.
> 	(gimple_canonical_types_compatible_p): Use canonical_type_used_p.
> 	* tree.h (canonical_type_used_p): New inline.
> 	* alias.c (get_alias_set): Handle structural equality for all
> 	types that pass canonical_type_used_p.
> 	(record_component_aliases): Look through all types with
> 	record_component_aliases for possible pointers; sanity check that
> 	the alias sets match.
> 
> 	* lto.c (iterative_hash_canonical_type): Recruse for all types
> 	which pass !canonical_type_used_p.
> 	(gimple_register_canonical_type_1): Sanity check we do not compute
> 	canonical type of anything with !canonical_type_used_p.
> 	(gimple_register_canonical_type): Skip all types that are
> 	!canonical_type_used_p
> 
> Index: lto-streamer-in.c
> ===================================================================
> --- lto-streamer-in.c	(revision 230718)
> +++ lto-streamer-in.c	(working copy)
> @@ -1231,7 +1231,9 @@ lto_read_body_or_constructor (struct lto
>  	      if (TYPE_P (t))
>  		{
>  		  gcc_assert (TYPE_CANONICAL (t) == NULL_TREE);
> -		  TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t);
> +		  if (type_with_alias_set_p (t)
> +		      && canonical_type_used_p (t))
> +		    TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t);
>  		  if (TYPE_MAIN_VARIANT (t) != t)
>  		    {
>  		      gcc_assert (TYPE_NEXT_VARIANT (t) == NULL_TREE);
> Index: tree.c
> ===================================================================
> --- tree.c	(revision 230718)
> +++ tree.c	(working copy)
> @@ -8236,7 +8243,8 @@ build_array_type_1 (tree elt_type, tree
>    if (TYPE_CANONICAL (t) == t)
>      {
>        if (TYPE_STRUCTURAL_EQUALITY_P (elt_type)
> -	  || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)))
> +	  || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))
> +	  || in_lto_p)
>  	SET_TYPE_STRUCTURAL_EQUALITY (t);
>        else if (TYPE_CANONICAL (elt_type) != elt_type
>  	       || (index_type && TYPE_CANONICAL (index_type) != index_type))
> @@ -9849,13 +9857,13 @@ make_vector_type (tree innertype, int nu
>    SET_TYPE_VECTOR_SUBPARTS (t, nunits);
>    SET_TYPE_MODE (t, mode);
>  
> -  if (TYPE_STRUCTURAL_EQUALITY_P (innertype))
> +  if (TYPE_STRUCTURAL_EQUALITY_P (innertype) || in_lto_p)
>      SET_TYPE_STRUCTURAL_EQUALITY (t);
>    else if ((TYPE_CANONICAL (innertype) != innertype
>  	    || mode != VOIDmode)
>  	   && !VECTOR_BOOLEAN_TYPE_P (t))
>      TYPE_CANONICAL (t)
> -      = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode);
> +      = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode);
>  
>    layout_type (t);
>  
> @@ -13279,7 +13292,8 @@ gimple_canonical_types_compatible_p (con
>  	 TYPE_CANONICAL is more fine grained than the equivalnce we test (where
>  	 all pointers are considered equal.  Be sure to not return false
>  	 negatives.  */
> -      gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2));
> +      gcc_checking_assert (canonical_type_used_p (t1)
> +			   && canonical_type_used_p (t2));
>        return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2);
>      }
>  
> Index: tree.h
> ===================================================================
> --- tree.h	(revision 230718)
> +++ tree.h	(working copy)
> @@ -4807,7 +4807,9 @@ extern void DEBUG_FUNCTION verify_type (
>  extern bool gimple_canonical_types_compatible_p (const_tree, const_tree,
>  						 bool trust_type_canonical = true);
>  extern bool type_with_interoperable_signedness (const_tree);
> -/* Return simplified tree code of type that is used for canonical type merging.  */
> +
> +/* Return simplified tree code of type that is used for canonical type
> +   merging.  */
>  inline enum tree_code
>  tree_code_for_canonical_type_merging (enum tree_code code)
>  {
> @@ -4829,6 +4831,23 @@ tree_code_for_canonical_type_merging (en
>    return code;
>  }
>  
> +/* Return ture if get_alias_set care about TYPE_CANONICAL of given type.
> +   We don't define the types for pointers, arrays and vectors.  The reason is
> +   that pointers are handled specially: ptr_type_node accesses conflict with
> +   accesses to all other pointers.  This is done by alias.c.
> +   Because alias sets of arrays and vectors are the same as types of their
> +   elements, we can't compute canonical type either.  Otherwise we could go
> +   form void *[10] to int *[10] (because they are equivalent for canonical type
> +   machinery) and get wrong TBAA.  */
> +
> +inline bool
> +canonical_type_used_p (const_tree t)
> +{
> +  return !(POINTER_TYPE_P (t)
> +	   || TREE_CODE (t) == ARRAY_TYPE
> +	   || TREE_CODE (t) == VECTOR_TYPE);
> +}
> +
>  #define tree_map_eq tree_map_base_eq
>  extern unsigned int tree_map_hash (const void *);
>  #define tree_map_marked_p tree_map_base_marked_p
> Index: alias.c
> ===================================================================
> --- alias.c	(revision 230718)
> +++ alias.c	(working copy)
> @@ -869,11 +870,11 @@ get_alias_set (tree t)
>        set = lang_hooks.get_alias_set (t);
>        if (set != -1)
>  	return set;
> -      /* Handle structure type equality for pointer types.  This is easy
> -	 to do, because the code bellow ignore canonical types on these anyway.
> -	 This is important for LTO, where TYPE_CANONICAL for pointers can not
> -	 be meaningfuly computed by the frotnend.  */
> -      if (!POINTER_TYPE_P (t))
> +      /* Handle structure type equality for pointer types, arrays and vectors.
> +	 This is easy to do, because the code bellow ignore canonical types on
> +	 these anyway.  This is important for LTO, where TYPE_CANONICAL for
> +	 pointers can not be meaningfuly computed by the frotnend.  */
> +      if (canonical_type_used_p (t))
>  	{
>  	  /* In LTO we set canonical types for all types where it makes
>  	     sense to do so.  Double check we did not miss some type.  */
> @@ -929,7 +931,9 @@ get_alias_set (tree t)
>       integer(kind=4)[4] the same alias set or not.
>       Just be pragmatic here and make sure the array and its element
>       type get the same alias set assigned.  */
> -  else if (TREE_CODE (t) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (t))
> +  else if (TREE_CODE (t) == ARRAY_TYPE
> +	   && (!TYPE_NONALIASED_COMPONENT (t)
> +	       || TYPE_STRUCTURAL_EQUALITY_P (t)))
>      set = get_alias_set (TREE_TYPE (t));
>  
>    /* From the former common C and C++ langhook implementation:
> @@ -971,7 +975,10 @@ get_alias_set (tree t)
>  	 We also want to make pointer to array/vector equivalent to pointer to
>  	 its element (see the reasoning above). Skip all those types, too.  */
>        for (p = t; POINTER_TYPE_P (p)
> -	   || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p))
> +	   || (TREE_CODE (p) == ARRAY_TYPE
> +	       && (!TYPE_NONALIASED_COMPONENT (p)
> +		   || !COMPLETE_TYPE_P (p)
> +		   || TYPE_STRUCTURAL_EQUALITY_P (p)))
>  	   || TREE_CODE (p) == VECTOR_TYPE;
>  	   p = TREE_TYPE (p))
>  	{
> @@ -1195,20 +1203,23 @@ record_component_aliases (tree type)
>  	       Accesses to it conflicts with accesses to any other pointer
>  	       type.  */
>  	    tree t = TREE_TYPE (field);
> -	    if (in_lto_p)
> +	    if (in_lto_p)
>  	      {
>  		/* VECTOR_TYPE and ARRAY_TYPE share the alias set with their
>  		   element type and that type has to be normalized to void *,
>  		   too, in the case it is a pointer. */
> -		while ((TREE_CODE (t) == ARRAY_TYPE
> -			&& (!COMPLETE_TYPE_P (t)
> -			    || TYPE_NONALIASED_COMPONENT (t)))
> -		       || TREE_CODE (t) == VECTOR_TYPE)
> -		  t = TREE_TYPE (t);
> +		while (!canonical_type_used_p (t) && !POINTER_TYPE_P (t))
> +		  {
> +		    gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t));
> +		    t = TREE_TYPE (t);
> +		  }
>  		if (POINTER_TYPE_P (t))
>  		  t = ptr_type_node;
> +		else if (flag_checking)
> +		  gcc_checking_assert (get_alias_set (t)
> +				       == get_alias_set (TREE_TYPE (field)));
>  	      }
> -	   
> +
>  	    record_alias_subset (superset, get_alias_set (t));
>  	  }
>        break;
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 230718)
> +++ lto/lto.c	(working copy)
> @@ -389,9 +389,7 @@ iterative_hash_canonical_type (tree type
>    /* All type variants have same TYPE_CANONICAL.  */
>    type = TYPE_MAIN_VARIANT (type);
>  
> -  /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing
> -     code never use it anyway.  */
> -  if (POINTER_TYPE_P (type))
> +  if (!canonical_type_used_p (type))
>      v = hash_canonical_type (type);
>    /* An already processed type.  */
>    else if (TYPE_CANONICAL (type))
> @@ -444,7 +442,7 @@ gimple_register_canonical_type_1 (tree t
>  
>    gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t)
>  		       && type_with_alias_set_p (t)
> -		       && !POINTER_TYPE_P (t));
> +		       && canonical_type_used_p (t));
>  
>    slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT);
>    if (*slot)
> @@ -477,7 +475,8 @@ gimple_register_canonical_type_1 (tree t
>  static void
>  gimple_register_canonical_type (tree t)
>  {
> -  if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t))
> +  if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t)
> +      || !canonical_type_used_p (t))
>      return;
>  
>    /* Canonical types are same among all complete variants.  */
> 
> 

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