Re: Enable pointer TBAA for LTO

[Richard Biener <rguenther at suse dot de>]

On Mon, 23 Nov 2015, Jan Hubicka wrote:

> > 
> > Please in future leave patches for review again if you do such
> > big changes before committing...
> 
> Uhm, sorry, next time I will be more cureful.  It seemed rather easy after
> debugging it but indeed it is somewhat surprising issue.
> > 
> > I don't understand why we need this (testcase?) because get_alias_set ()
> > is supposed to do the alias-set of pointer globbing for us.
> 
> The situation is as follows.  You can have
> 
> struct a {
>   int *ptr;
> }
> 
> struct b {
>   float *ptr;
> }
> 
> Now, if becase type is ignored by TYPE_CANONICAL, we have
> 
>    TYPE_CANONICAL (sturct b) = struct a.
> 
> and while building alias set of "struct a" we record compoents as:
> 
>    struct a
>       ^
>       |
>     int *
> 
> Now do
> 
> struct b b = {NULL};
> float *p=&b->ptr;
> *p=nonnull;
> return b.ptr;
> 
> Now alias set of the initialization is alias set of "struct a". The alias set
> of of the pointer store is "float *".  We ask alias oracle if "struct a" can
> conflict with "float *" and answer is no, because "int *" (component of struct
> b) and "float *" are not conflicting.  With the change we record component
> alias as follows:

Ah, with my original pointer globbing I side-stepped this.  So are
you _really_ sure that we want to handle int * different from float *?
Because that makes the situation much more complicated in these cases.

> 
>    struct a
>       ^
>       |
>    void *
> 
> which makes the answer to be correct, because "int *" conflicts with all 
> pointers, so all such queries about a structure 
> gimple_canonical_types_compatible with "struct a" will be handled 
> correctly.
>
> I will add aritifical testcase for this after double checking that the code above
> miscompiles without that conditional.
> 
> I found that having warning about TBAA incompatibility when doing merigng in
> lto-symtab is great to catch issues like this.
> 
> I had this patch for quite some time, but it was producing obviously wrong
> positives (in Fortran, Ada and also sometimes in Firefox on array initializes
> of style int a[]={1,2,3}).  I wrongly assumed tha the bug is because we compute
> TYPE_CANONICAL sensitively to array size and there are permited transitions
> of array sizes between units.  THis is not the case.
> 
> Yesterday I found that the problem is with interaction of get_alias_set
> globbing and gimple_canonical_types_compatible globbing.  We can't have
> get_alias_set globbing more or less coarse than
> gimple_canonical_types_compatible does.
> 
> Here the situation is reverse to the case above : because array type is
> inherited from element type, we can't have TYPE_CANONICAL more globbed for
> array than we have get_alias_set.  In this case the problem appeared when
> TYPE_NONALIASED_COMPONENT array previaled in type merging other arrays.  The
> situation got worse with pointer, becuase array of pointers of one type could
> prevail array of pointers of other type and then the array type gets different
> alias sets in different units.  This seems to work for C programs, but is
> wrong.  I will send patch and separate explanation after re-testing final
> version shortly.

I feel we need to document all this somewhere.

Richard.
 
> Honza
> 
> 

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