Re: [PATCH] Add a new type attribute always_alias (PR79671)

[Jason Merrill <jason at redhat dot com>]

On Tue, Apr 11, 2017 at 9:35 AM, Richard Biener <rguenther@suse.de> wrote:
> On Tue, 11 Apr 2017, Richard Biener wrote:
>
>> On Tue, 11 Apr 2017, Richard Biener wrote:
>>
>> > On Mon, 10 Apr 2017, Jason Merrill wrote:
>> >
>> > > On Mon, Apr 10, 2017 at 11:30 AM, Richard Biener <rguenther@suse.de> wrote:
>> > > > On Mon, 10 Apr 2017, Jason Merrill wrote:
>> > > >> On Mon, Apr 10, 2017 at 8:50 AM, Richard Biener <rguenther@suse.de> wrote:
>> > > >> >         * tree.c (build_cplus_array_type): Set TYPE_TYPELESS_STORAGE
>> > > >> >         for arrays of unsigned char or std::byte.
>> > > >>
>> > > >> I think it would be good to have a flag to select whether these
>> > > >> semantics apply to any char variant and std::byte, only unsigned char
>> > > >> and std::byte, or only std::byte.
>> > > >
>> > > > Any suggestion?  Not sure we really need it (I'm hesitant to write
>> > > > all the testcases to verify it actually works).
>> > >
>> > > Well, there's existing code that uses plain char (e.g. boost) that I
>> > > want to support.  If there's a significant optimization penalty for
>> > > allowing that, we probably also want to be able to limit the impact.
>> > > If there isn't much penalty, let's just support all char variants.
>> >
>> > I haven't assessed the penalty involved but it can't be worse than
>> > the situation we had in GCC 6.  So I think it's reasonable to support
>> > all char variants for now.  One could add some instrumenting to
>> > alias_set_subset_of / alias_sets_conflict_p but it would only yield
>> > an upper bound on the number of failed queries (TBAA is a quite early
>> > out before PTA info is used for example).
>> >
>> > The following variant -- added missing
>> >
>> > Index: gcc/cp/tree.c
>> > ===================================================================
>> > --- gcc/cp/tree.c       (revision 246832)
>> > +++ gcc/cp/tree.c       (working copy)
>> > @@ -972,6 +979,7 @@ build_cplus_array_type (tree elt_type, t
>> >                  as it will overwrite alignment etc. of all variants.  */
>> >               TYPE_SIZE (t) = TYPE_SIZE (m);
>> >               TYPE_SIZE_UNIT (t) = TYPE_SIZE_UNIT (m);
>> > +             TYPE_TYPELESS_STORAGE (t) = TYPE_TYPELESS_STORAGE (m);
>> >             }
>> >
>> >           TYPE_MAIN_VARIANT (t) = m;
>> >
>> > that caused LTO bootstrap to fail and removed the tree-ssa-structalias.c
>> > change (committed separately) [LTO] bootstrapped and tested ok on
>> > x86_64-unknown-linux-gnu.
>> >
>> > I've tested some template examples and they seem to work fine.
>> >
>> > Ok for trunk?
>> >
>> > Disclaimer: there might still be an issue with cross-language LTO
>> > support, but it might at most result in TYPE_TYPELESS_STORAGE
>> > getting lost.  Trying to craft a testcase to verify my theory.
>>
>> Not too difficult in the end (see below).  A fix (below) is to
>> not treat arrays with differing TYPE_TYPELESS_STORAGE as
>> compatible for the purpose of computing TYPE_CANONICAL (and
>> thus recursively structures containing them).  While they'd
>> still alias each other (because currently a TYPE_TYPELESS_STORAGE
>> member makes the whole thing effectively alias anything) this
>> results in warnings in case such a type is used in the interface
>> between C and C++ (it's also considered a ODR type).
>>
>> t.C:17:17: warning: type of ‘bar’ does not match original declaration
>> [-Wlto-type-mismatch]
>>  extern "C" void bar (X *);
>>                  ^
>> t2.c:5:6: note: ‘bar’ was previously declared here
>>  void bar (struct X *p) {}
>>       ^
>>
>> if you add a struct X * parameter to bar().
>>
>> So it's not the optimal solution here.  Another fix would be to
>> somehow make TYPE_TYPELESS_STORAGE "sticky" when merging canonical
>> types but there's not precedent in doing this kind of thing and
>> I'm not sure we'll get everything merged before computing alias
>> sets.
>>
>> CCing Honza.
>
> So after discussion and some more thinking we don't really benefit
> (and really can't) from having different aggregates with such
> members distignuishable via their alias set.  So the following
> simplifies the approach and makes the above LTO bits trivial
> by more following Bernds approach of assigning the types alias
> set zero and instead of in the alias machinery propagate the
> flag on the types.
>
> It should also make reference_alias_ptr_type correct and it
> does the flag propagation in type layout.
>
> [LTO] bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
> The C++ bits are unchanged.

The C++ bits are OK.

Jason