Re: Enable pointer TBAA for LTO

[Jan Hubicka <hubicka at ucw dot cz>]

> > Index: tree.c
> > ===================================================================
> > --- tree.c	(revision 229968)
> > +++ tree.c	(working copy)
> > @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con
> >    /* If the types have been previously registered and found equal
> >       they still are.  */
> >    if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2)
> > +      && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)
> 
> But TYPE_CANONICAL (t1) should be NULL_TREE for POINTER_TYPE_P?

The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be
called before we finish all merging and then it is more fine grained than what
we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be
different, but here we want them to be equal so we can match:

struct aa { void *ptr;};
struct bb { int * ptr;};

Which is actually required for Fortran interoperability.

Removing this hunk triggers false type incompatibility warning in one of the
interoperability testcases I added.

Even if I drop the code bellow setting TYPE_CANOINCAL, I think I need to keep
this conditional: the types may be built in and those get TYPE_CANONICAL set as
they are constructed by build_pointer_type.  I can gcc_checking_assert for this
scenario and see.  Perhaps we never build LTO type from builtin type and this
won't happen. If we did, we would probably have a trouble with false negatives
in return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); on non-pointers anyway.
> 
> >        && trust_type_canonical)
> >      return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2);
> >  
> > Index: alias.c
> > ===================================================================
> > --- alias.c	(revision 229968)
> > +++ alias.c	(working copy)
> > @@ -869,13 +874,19 @@ get_alias_set (tree t)
> >        set = lang_hooks.get_alias_set (t);
> >        if (set != -1)
> >  	return set;
> > -      return 0;
> > +      /* LTO frontend does not assign canonical types to pointers (which we
> > +	 ignore anyway) and we compute them.  The following path may be
> > +	 probably enabled for non-LTO, too, and it may improve TBAA for
> > +	 pointers to types with structural equality.  */
> > +      if (!in_lto_p || !POINTER_TYPE_P (t))
> > +        return 0;
> 
> No new LTO paths please, do the suggested change immediately.

OK, I originally tested the patch without if and there was no problems.
Just chickened out before preparing final version of the patch.
> > +	      p = TYPE_MAIN_VARIANT (p);
> > +	      /* Normally all pointer types are built by
> > + 		 build_pointer_type_for_mode which ensures they have canonical
> > +		 type unless they point to type with structural equality.
> > +		 LTO frontend produce pointer types without TYPE_CANONICAL
> > +		 that are then added to TYPE_POINTER_TO lists and 
> > +		 build_pointer_type_for_mode will end up picking one for us.
> > +		 Declare it the canonical one.  This is the same as
> > +		 build_pointer_type_for_mode would do. */
> > +	      if (!TYPE_CANONICAL (p))
> > +		{
> > +		  TYPE_CANONICAL (p) = p;
> > +		  gcc_checking_assert (in_lto_p);
> > +		}
> > +	      else
> > +	        gcc_checking_assert (p == TYPE_CANONICAL (p));
> 
> The assert can trigger as
> build_pointer_type_for_mode builds SET_TYPE_STRUCTURAL_EQUALITY pointer
> types for SET_TYPE_STRUCTURAL_EQUALITY pointed-to types.  Ah,
> looking up more context reveals
> 
>       if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p))
>         set = get_alias_set (ptr_type_node);

Yep, we don't get here.
> 
> Not sure why you adjust TYPE_CANONICAL here at all either.

You are right, I may probably just drop all the code and just do:
gcc_checking_assert (!TYPE_CANONICAL || p == TYPE_CANONICAL (p));
I will test this and re-think the build_pointer_type code to be sure that we
won't get into a problem there.

As I recall, the original code
  p = TYPE_CANONICAL (p);
was there to permit frontends to glob two pointers by setting same canonical
type to them.  My original plan was to use this for LTO frotnend and make
gimple_compare_canonical_types to do the right thing for pointers and this would
follow gimple_compare_canonical_types globbing then.

This idea was wrong: since pointer rules are not transitive (i.e. void
* alias them all), we can't model that by an equivalence produced by
gimple_compare_canonical_types.

Since the assert does not trigger, seems no frontend is doing that and moreover
I do not see how that would be useful (well, perhaps for some kind of internal
bookeeping when build TYPE_CANONICAL of more complex types from pointer types,
like arrays, but for those we ignore TYPE_CANONICAL anyway).  Grepping over
TYPE_CANONICAL sets in frotneds, I see no code that I would suspect from doing
something like this.

Thank you!
Honza
> 
> Otherwise looks ok.
> 
> RIchard.
> 
> 
> >  	    }
> > -          gcc_checking_assert (TYPE_CANONICAL (p) == p);
> >  
> >  	  /* Assign the alias set to both p and t.
> >  	     We can not call get_alias_set (p) here as that would trigger
> > @@ -1015,11 +1043,6 @@ get_alias_set (tree t)
> >  	    }
> >  	}
> >      }
> > -  /* In LTO the rules above needs to be part of canonical type machinery.
> > -     For now just punt.  */
> > -  else if (POINTER_TYPE_P (t)
> > -	   && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p)
> > -    set = get_alias_set (TYPE_CANONICAL (ptr_type_node));
> >  
> >    /* Otherwise make a new alias set for this type.  */
> >    else
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)