This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Enable TBAA on anonymous types with LTO


> 
> Ok, as Jason explained this wouldn't work, so we indeed need to stream
> TYPE_CANONICAL in this case (and also properly SCC-walk it!)

Yep, my patch streams and SCC walks them when they are anonymous.
By incremental patch I plan to handle all ODR and component types,
so basically stream canonical types for all C++ types.
> 
> Huh?  The merging code has an early out on different SCC size.  And
> TYPE_CANONICAL

I was seeing ICE in the loop merging the SCC unions. Will try to reproduce
it and investigate further.

> is not set at this point (once you stream it you have to walk and
> compare it during
> comparison).

At the moment I probably don't need to compare it, because it is set only for
anonymous types that are never merged. Once ODR part is in, we will ideed need
to match them.  
I do not want to match when the TYPE_CANONICAL is set by canonical type hash,
but I suppose I only need to check if TYPE_CANONICAL of one of types is
anonymous and rely on canonical type hash never set TYPE_CANONICAL to anonymous
type (it should not)
> 
> What about other languages that match your anonymous-namespace check?
> This should really be thoroughly documented as a middle-end feature/requirement
> that languages should be aware of.

TREE_PUBLIC flag is already documented as such in tree.h
/* In a VAR_DECL, FUNCTION_DECL, NAMESPACE_DECL or TYPE_DECL,
   nonzero means name is to be accessible from outside this translation unit.
   In an IDENTIFIER_NODE, nonzero means an external declaration
   accessible from outside this translation unit was previously seen
   for this name in an inner scope.  */

But indeed the C frontend does not set it according to this in all cases (fixed
in my patch). I added Ada and Fortran maintainers to help me identifying if
these have notion of such local types.

Fixing FE's is forutnately easy - there are very few places TYPE_STUB is built
and all I need is to hack stream out to accept anonymous types only for C++
and LTO FE.
> 
> > This does not work for types build from ODR types that arenot ODR themselves.
> > My plan is:
> >   1) fork current canonical_type hash into two - structural_type_hash and
> >      canonical_type_hash
> 
> Ick, please no.
> 
> >   2) During the streaming in, I populate structural_type_hash and the existing
> >      odr_hash.  I record what structural type hash buckets contains non-ODR
> >      type
> >   3) During the existing loop that recomputes canonical type I start populating
> >      canonical type hash.  It works same way as structural except for ODR
> >      types that do not conflict. THose are considered by ODR equality.
> 
> What's the advantage over what your proposed patch did?  Apart from a lot
> of extra complexity?

I am shooting to handle all ODR types, tobasically to fully preserve TBAA for
C++ (modulo structural conficts with non-C++).  This patch is first step as I
tought it will be easier to first get anonymous types right and build
incrementally from this. I expected that for anonymous types I do not need
to solve the conflicts with non-C++ and also they never merge. Still they
need good part of the full infrastructure I want.

Obviously C++ programs tends to have a lot of different types that have
same layout.  Making them all alias does not make them easier to optimize.
> 
> How many types in anonymous namespaces do C++ programs usually have?
> That is, does it make a difference in real-world?  What's the number of
> extra disambiguations we get from it?

How do I measure the extra disambiguations?
> >> Not in this form, we have to discuss this further.  It's way too agressive
> >> in my view.
> >
> > Yep, I expected that, but we need to start somewhere ;))
> 
> Do we really?  I think that you eventually want something like a "type escape
> analysis" instead?  Or better said, we make use of TBAA only intra-procedural
> and thus after LTO IPA inlining we know exactly what types are refered to
> per function.  Which means if a function is composed only from bits of a
> single TU we can retain the original TYPE_CANONICALs for all memory
> accesses inside that function.

I do not think we have that many functions that would be composed only from
stuff from single TU.  This is becuase most of COMDAT code gets commonized
before inlining and true cross-module inlining is also very common thing.
On extension I was thinking about in longer term is to tag memory accesses
with original translation unit and make TBAA to be stronger for two
accesses within same unit.

> 
> >> I don't like changing the hashing/registering machinery this way.
> >>
> >> Can you please introduce a function that enters both hash and type
> >> into the tables (asserting they are not already there) and call that
> >> explicitely for anonymous types from lto_read_decls ...
> >
> > OK, so having something like register_odr_type doing both?
> 
> Well, you only register it in the hash-hash, no?

Yes, you are riht. I want to have it only in hash-hash to make types built
from it that are non-anonymous (ICK) not ICE.
> 
> > Of course multiple ODR types may have same canonical type hash, so I can not
> > just assert they are not already there.
> 
> As the ODR type should never be merged with anything (and thus does not
> need an entry in the canonical type hash) it doesn't matter.
> 
> > I believe the TYPE_CANONICAl check originally prevented ICE in the case
> > where strongly connected component is ordered in a way so registering earlier
> > type into canonical type hash makes it recursively regiser later type.
> > So it may happen that the type is already registered at a time lto_read_decls
> > walks acorss it.
> 
> I don't remember exactly, but the assert was more like a contract one on the
> existing API.

I was looking into it - as the TYPE_CANONICAL check looked bogues on to me
and the "wrongly" ordered SCC components seemed to be the reason. That is also
why hash-hash sometimes need to recurse.
> >
> > Yes, I had patches for that. You told you will think about best implementatoin
> > later.  Later did not seem to happen yet :)
> > https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00614.html
> > seems to be end of the thread.
> >
> > Yes, having that patch makes it possible to dropthese checks.
> 
> Ok, I'll try to revisit above somewhen next week.

Cool!
> 
> >>
> >> >      }
> >> >
> >> >    if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON))
> >> > @@ -1909,9 +1947,7 @@ lto_read_decls (struct lto_file_decl_dat
> >> >               num_prevailing_types++;
> >> >               lto_fixup_prevailing_type (t);
> >> >             }
> >> > -         /* Compute the canonical type of all types.
> >> > -            ???  Should be able to assert that !TYPE_CANONICAL.  */
> >> > -         if (TYPE_P (t) && !TYPE_CANONICAL (t))
> >> > +         if (TYPE_P (t))
> >>
> >> ... here as else { ...?
> >
> > I am not sure what you mean here?
> 
> A continued comment from one above, where to put the register_odr_type call.

Yep, I get what you mean and will give it a try.
I still wonder about wrongly ordered SCCs.  I proably can not simply try it,
because we have no non-anonymous type built from anonymous in testsuite. I noticed
those only in libreoffice where few anonymous enums are used in methods of
non-anonymous classes.  I suppose I can try to biuld artificial testcase,
but I am not that familiar with forcing SCC and their order.

Thanks for looking into this - indeed it is difficult area.
Honza


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]