On Tue, 10 Nov 2015, Jan Hubicka wrote:
> > > 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.
Ok, I see.
> 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.
Hmm, indeed. The various type builders might end up setting
TYPE_CANONICAL if you ever run into a pre-defined pointer type
(ptr_type_node for example).
> >
> > > && 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.
Yes.
> 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.
Ok. Let's see what your experiment shows, otherwise the original patch
is ok.
Thanks,
Richard.
> 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 <rguent...@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
> > 21284 (AG Nuernberg)
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
