> > 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
