| Message ID | 20190603132957.w2vh5z3zu3h6z2ib@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Use ODR for canonical types construction in LTO | expand |
> * alias.c (record_component_aliases): Honor odr_based_tbaa_p. > * ipa-devirt.c (odr_type_d): Add tbaa_enabled field. > (get_odr_type): Return NULL when odr_type_hash is NULL. > (enable_odr_based_tbaa): New function. > (odr_based_tbaa_p): New function. > (set_type_canonical_for_odr_type): New function. > * ipa-utils.h (enable_odr_based_tbaa, odr_based_tbaa_p, > set_type_canonical_for_odr_type): Declare. > * lto-common.c: Update copyright; include tree-pretty-print.h. > (types_to_register): New static var. > (iterative_hash_canonical_type): Add new parameter INSERT. > (hash_canonical_type): Likewise. > (lookup_canonical_type): New function. > (lto_register_canonical_types_for_odr_types): New. > (odr_or_derived_type_p): New function. > (lto_read_decls): Deffer ODR and ODR derived types > to later canonical type calculation. > * tree.c (gimple_canonical_types_compatible_p): Honnor > odr_based_tbaa_p. Ping. This patch improves TBAA for C++ code bases relatively noticeably... Honza
On Mon, 3 Jun 2019, Jan Hubicka wrote: > Hi, > this patch makes LTO to use ODR names when building canonical types. > Theoretically this is easy task because every ODR type is unique and > it is enough to fill in the hash with the ODR names and compare them. > > In reality we want to be more careful and detect situation when non-ODR > type is structurally equivalent to ODR type so C++ code and non-C++ code > can co-exist. > > I have implemented it as follows > 1) at stream in time populate canonical type hash by non-ODR types > only. Also register all ODR types into so we detect ODR violations. > 2) once all streaming is done we process all ODR types, lookup each > of them in the canonical type hash and if structurally compatible > type is present default to normal canonical type calculation. > Otherwise canonical type of ODR type is the prevailing ODR variant. This sounds sensible, while ... > Now the problem is that in C++ some structures can be non-ODR. For > example > > struct A {int a;} a; > struct {int b;} b; > > Here the second structure has no name and we have no way to tell if it > is C++ or non-C++ code at LTO time. I tought that perhaps such conflicts > would be rare, but in reality most of types conflicts this way > (C++ FE seems to generate non-ODR types of same strucutre as ODR > types and they do get streamed, I am looking into this). > So more careful approach is needed. > > I extende extend step 1 to also postpone types that reffers to ODR types (those > are known to originate from C++ translation units and do not need to be unified > by structurally equivalent ODR types). > > This is tested by odr_or_derived_type_p which is kind of hack and perhaps we > should get around streaming this info somehow (but i would preffer to do that > incrementally) > > Step 2 remain same. > Additional step 3 registers all odr derived types into canonical type hash. > This requires canonical type hash to play well with ODR types (i.e. not > consider them equivalent based on structural equivalety). > > The decision on whether given type has ODR based canonical > type is stored in odr_based_tbaa_p and is used > 1) by gimple_canonical_types_compatible_p so the comparsion is > behaving well after ODR types are inserted into the hash > 2) by alias.c to be build more precise alias sets - for ODR types > it is not necessary to glob pointers into void *. > There is no need to update canonical type hash since I simply insert > hash codes of ODR names into canonical type hash cache. ICK. Can you please solve the C++ issue differently? The patch also seems to do many other things ... More comments inline. > > LTO linking cc1plus there are 3317 ODR types that gets their own > canonical type and 876 which have conflict. For example: > > ODR and non-ODR type conflict: union ssa_name_info_type and union > { > const void * cv; > void * v; > } > ODR and non-ODR type conflict: struct ht_identifier and struct > { > union byte_fail_stack_elt_t * stack; > unsigned int size; > unsigned int avail; > } > ODR and non-ODR type conflict: struct lang_hooks_for_tree_dump and struct > { > struct demangle_component * left; > struct demangle_component * right; > } > ODR and non-ODR type conflict: struct call_site_record_d and struct > { > struct demangle_component * sub; > int num; > } > > etc. > > So mostly random conflict induced by the C compiled liberty and other bits. > Clearly most common issue is globing of pointers and lack of field names > which is needed primarily for Fortran and is easy to disable for translation units > not having Fortran modules in them. > > Canonical type hash stats for cc1plus build: > > [WPA] GIMPLE canonical type table: size 16381, 1590 elements, 6797 searches, 88 collisions (ratio: 0.012947) > [WPA] GIMPLE canonical type pointer-map: 1590 elements, 15494 searches > > to: > > [WPA] GIMPLE canonical type table: size 16381, 755 elements, 6311 searches, 57 collisions (ratio: 0.009032) > [WPA] GIMPLE canonical type pointer-map: 755 elements, 15553 searches > > So without patch there are 1590 distinct canonical types, with the patch there > are 755 non-ODR and 3317 odr types, about 2.5 times more. > > And alias oracle stats for -flto-partition=none cc1plus build: > > Alias oracle query stats: > refs_may_alias_p: 39227004 disambiguations, 47344870 queries > ref_maybe_used_by_call_p: 57815 disambiguations, 39808081 queries > call_may_clobber_ref_p: 5511 disambiguations, 8287 queries > aliasing_component_ref_p: 90654 disambiguations, 269895 queries > TBAA oracle: 11130153 disambiguations 34368560 queries > 14199938 are in alias set 0 > 5193428 queries asked about the same object > 147 queries asked about the same alias set > 0 access volatile > 1665979 are dependent in the DAG > 2178915 are aritificially in conflict with void * > > PTA query stats: > pt_solution_includes: 464074 disambiguations, 7105649 queries > pt_solutions_intersect: 355139 disambiguations, 6952492 queries > > > Alias oracle query stats: > refs_may_alias_p: 39682205 disambiguations, 47774305 queries > ref_maybe_used_by_call_p: 58390 disambiguations, 40265537 queries > call_may_clobber_ref_p: 5619 disambiguations, 8397 queries > aliasing_component_ref_p: 61523 disambiguations, 240208 queries > TBAA oracle: 11691755 disambiguations 34552651 queries > 14233568 are in alias set 0 > 5230754 queries asked about the same object > 147 queries asked about the same alias set > 0 access volatile > 2433081 are dependent in the DAG > 963346 are aritificially in conflict with void * > > PTA query stats: > pt_solution_includes: 457871 disambiguations, 7044729 queries > pt_solutions_intersect: 367701 disambiguations, 6859208 queries > > So about 455k new disambiguations or 5% more TBAA ones. > It is about half of what PTA oracle stats shows, so it seems > reasibale improvement to me. > > The patch results in about 1% cc1plus code size increase. > (mostly because of less ICF) > > For tramp3d the stats are: > > refs_may_alias_p: 3252729 disambiguations, 3587196 queries > ref_maybe_used_by_call_p: 7018 disambiguations, 3280028 queries > call_may_clobber_ref_p: 883 disambiguations, 883 queries > aliasing_component_ref_p: 549 disambiguations, 17885 queries > TBAA oracle: 1644568 disambiguations 3261681 queries > 555812 are in alias set 0 > 680883 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 248396 are dependent in the DAG > 132022 are aritificially in conflict with void * > > PTA query stats: > pt_solution_includes: 641563 disambiguations, 921034 queries > pt_solutions_intersect: 115861 disambiguations, 501205 queries > > Alias oracle query stats: > refs_may_alias_p: 3016277 disambiguations, 3316301 queries > ref_maybe_used_by_call_p: 7134 disambiguations, 3042055 queries > call_may_clobber_ref_p: 817 disambiguations, 817 queries > aliasing_component_ref_p: 495 disambiguations, 14565 queries > TBAA oracle: 1416803 disambiguations 2911585 queries > 552489 are in alias set 0 > 567158 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 259923 are dependent in the DAG > 115212 are aritificially in conflict with void * > > PTA query stats: > pt_solution_includes: 668779 disambiguations, 949986 queries > pt_solutions_intersect: 96232 disambiguations, 436617 queries > > About 7% difference. > > I did Firefox stats with earlier variant of this patch and difference was about 12%. > It takes about a day for lto1-ltrans with one partition to converge (and two > days for for gas) so I did not re-run it with final version. Code size > difference is about 4%. > > Notice also that access path oracle now suceeds less often since > we less often mix up unrealted types. In general with more precist > TBAA lookups the oracle should be bit cheaper by avoiding more complex > tests. > > lto-bootstrapped/regtested x86_64-linux with all languages and also > tested on few extra C++ apps. > > * alias.c (record_component_aliases): Honor odr_based_tbaa_p. > * ipa-devirt.c (odr_type_d): Add tbaa_enabled field. > (get_odr_type): Return NULL when odr_type_hash is NULL. > (enable_odr_based_tbaa): New function. > (odr_based_tbaa_p): New function. > (set_type_canonical_for_odr_type): New function. > * ipa-utils.h (enable_odr_based_tbaa, odr_based_tbaa_p, > set_type_canonical_for_odr_type): Declare. > * lto-common.c: Update copyright; include tree-pretty-print.h. > (types_to_register): New static var. > (iterative_hash_canonical_type): Add new parameter INSERT. > (hash_canonical_type): Likewise. > (lookup_canonical_type): New function. > (lto_register_canonical_types_for_odr_types): New. > (odr_or_derived_type_p): New function. > (lto_read_decls): Deffer ODR and ODR derived types > to later canonical type calculation. > * tree.c (gimple_canonical_types_compatible_p): Honnor > odr_based_tbaa_p. > Index: alias.c > =================================================================== > --- alias.c (revision 271843) > +++ alias.c (working copy) > @@ -1202,47 +1202,52 @@ record_component_aliases (tree type) > case RECORD_TYPE: > case UNION_TYPE: > case QUAL_UNION_TYPE: > - for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field)) > - if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field)) > - { > - /* LTO type merging does not make any difference between > - component pointer types. We may have > - > - struct foo {int *a;}; > - > - as TYPE_CANONICAL of > - > - struct bar {float *a;}; > - > - Because accesses to int * and float * do not alias, we would get > - false negative when accessing the same memory location by > - float ** and bar *. We thus record the canonical type as: > - > - struct {void *a;}; > - > - void * is special cased and works as a universal pointer type. > - Accesses to it conflicts with accesses to any other pointer > - type. */ > - tree t = TREE_TYPE (field); > - if (in_lto_p) > - { > - /* VECTOR_TYPE and ARRAY_TYPE share the alias set with their > - element type and that type has to be normalized to void *, > - too, in the case it is a pointer. */ > - while (!canonical_type_used_p (t) && !POINTER_TYPE_P (t)) > - { > - gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t)); > - t = TREE_TYPE (t); > - } > - if (POINTER_TYPE_P (t)) > - t = ptr_type_node; > - else if (flag_checking) > - gcc_checking_assert (get_alias_set (t) > - == get_alias_set (TREE_TYPE (field))); > - } > - > - record_alias_subset (superset, get_alias_set (t)); > - } > + { > + /* LTO non-ODR type merging does not make any difference between > + component pointer types. We may have > + > + struct foo {int *a;}; > + > + as TYPE_CANONICAL of > + > + struct bar {float *a;}; > + > + Because accesses to int * and float * do not alias, we would get > + false negative when accessing the same memory location by > + float ** and bar *. We thus record the canonical type as: > + > + struct {void *a;}; > + > + void * is special cased and works as a universal pointer type. > + Accesses to it conflicts with accesses to any other pointer > + type. */ > + bool void_pointers = in_lto_p > + && (!odr_type_p (type) > + || !odr_based_tbaa_p (type)); This seems like an independent improvement to me. > + for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field)) > + if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field)) > + { > + tree t = TREE_TYPE (field); > + if (void_pointers) > + { > + /* VECTOR_TYPE and ARRAY_TYPE share the alias set with their > + element type and that type has to be normalized to void *, > + too, in the case it is a pointer. */ > + while (!canonical_type_used_p (t) && !POINTER_TYPE_P (t)) > + { > + gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t)); > + t = TREE_TYPE (t); > + } > + if (POINTER_TYPE_P (t)) > + t = ptr_type_node; > + else if (flag_checking) > + gcc_checking_assert (get_alias_set (t) > + == get_alias_set (TREE_TYPE (field))); > + } > + > + record_alias_subset (superset, get_alias_set (t)); > + } > + } > break; > > case COMPLEX_TYPE: > Index: ipa-devirt.c > =================================================================== > --- ipa-devirt.c (revision 271843) > +++ ipa-devirt.c (working copy) > @@ -213,6 +213,8 @@ struct GTY(()) odr_type_d > bool odr_violated; > /* Set when virtual table without RTTI previaled table with. */ > bool rtti_broken; > + /* Set when we need to give up on ODR based TBAA info. */ > + bool tbaa_enabled; ? > }; > > /* Return TRUE if all derived types of T are known and thus > @@ -2045,6 +2047,9 @@ get_odr_type (tree type, bool insert) > if (!in_lto_p && !TYPE_STRUCTURAL_EQUALITY_P (type)) > type = TYPE_CANONICAL (type); > > + if (!odr_hash) > + return NULL; > + > gcc_checking_assert (can_be_name_hashed_p (type) > || can_be_vtable_hashed_p (type)); > > @@ -2182,6 +2187,45 @@ prevailing_odr_type (tree type) > return t->type; > } > > +/* Set tbaa_enabled flag for TYPE. */ > + > +void > +enable_odr_based_tbaa (tree type) > +{ > + odr_type t = get_odr_type (type, true); > + t->tbaa_enabled = true; > +} > + > +/* Return type that in ODR type hash prevailed TYPE. Be careful and punt > + on ODR violations. */ > + > +bool > +odr_based_tbaa_p (const_tree type) > +{ > + odr_type t = get_odr_type (const_cast <tree> (type), false); > + if (!t || !t->tbaa_enabled) > + return false; > + return true; > +} > + > +/* Set TYPE_CANONICAL of type and all its variants and duplicates > + to CANONICAL. */ > + > +void > +set_type_canonical_for_odr_type (tree type, tree canonical) > +{ > + odr_type t = get_odr_type (type, false); > + unsigned int i; > + tree tt; > + > + for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > + if (t->types) > + FOR_EACH_VEC_ELT (*t->types, i, tt) > + for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > +} > + > /* Return true if we reported some ODR violation on TYPE. */ > > bool > Index: ipa-utils.h > =================================================================== > --- ipa-utils.h (revision 271843) > +++ ipa-utils.h (working copy) > @@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t > bool odr_types_equivalent_p (tree type1, tree type2); > bool odr_type_violation_reported_p (tree type); > tree prevailing_odr_type (tree type); > +void enable_odr_based_tbaa (tree type); > +bool odr_based_tbaa_p (const_tree type); > +void set_type_canonical_for_odr_type (tree type, tree canonical); > > /* Return vector containing possible targets of polymorphic call E. > If COMPLETEP is non-NULL, store true if the list is complete. > Index: lto/lto-common.c > =================================================================== > --- lto/lto-common.c (revision 271843) > +++ lto/lto-common.c (working copy) > @@ -1,5 +1,5 @@ > /* Top-level LTO routines. > - Copyright (C) 2009-2018 Free Software Foundation, Inc. > + Copyright (C) 2009-2019 Free Software Foundation, Inc. > Contributed by CodeSourcery, Inc. > > This file is part of GCC. > @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. > #include "attribs.h" > #include "builtins.h" > #include "lto-common.h" > +#include "tree-pretty-print.h" > > GTY(()) tree first_personality_decl; > > @@ -212,22 +213,26 @@ lto_read_in_decl_state (struct data_in * > > > /* Global canonical type table. */ > +static GTY(()) vec<tree, va_gc> *types_to_register = NULL; > static htab_t gimple_canonical_types; > static hash_map<const_tree, hashval_t> *canonical_type_hash_cache; > static unsigned long num_canonical_type_hash_entries; > static unsigned long num_canonical_type_hash_queries; > > -static void iterative_hash_canonical_type (tree type, inchash::hash &hstate); > +static void iterative_hash_canonical_type (tree type, inchash::hash &hstate, bool); > static hashval_t gimple_canonical_type_hash (const void *p); > static void gimple_register_canonical_type_1 (tree t, hashval_t hash); > > /* Returning a hash value for gimple type TYPE. > > The hash value returned is equal for types considered compatible > - by gimple_canonical_types_compatible_p. */ > + by gimple_canonical_types_compatible_p. > + > + If INSERT is true, also populate canonical type hash by all > + types TYPE is derived from. */ > > static hashval_t > -hash_canonical_type (tree type) > +hash_canonical_type (tree type, bool insert) > { > inchash::hash hstate; > enum tree_code code; > @@ -293,7 +298,7 @@ hash_canonical_type (tree type) > if (TREE_CODE (type) == ARRAY_TYPE > || TREE_CODE (type) == COMPLEX_TYPE > || TREE_CODE (type) == VECTOR_TYPE) > - iterative_hash_canonical_type (TREE_TYPE (type), hstate); > + iterative_hash_canonical_type (TREE_TYPE (type), hstate, insert); > > /* Incorporate function return and argument types. */ > if (TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE) > @@ -301,11 +306,11 @@ hash_canonical_type (tree type) > unsigned na; > tree p; > > - iterative_hash_canonical_type (TREE_TYPE (type), hstate); > + iterative_hash_canonical_type (TREE_TYPE (type), hstate, insert); > > for (p = TYPE_ARG_TYPES (type), na = 0; p; p = TREE_CHAIN (p)) > { > - iterative_hash_canonical_type (TREE_VALUE (p), hstate); > + iterative_hash_canonical_type (TREE_VALUE (p), hstate, insert); > na++; > } > > @@ -322,7 +327,7 @@ hash_canonical_type (tree type) > && (! DECL_SIZE (f) > || ! integer_zerop (DECL_SIZE (f)))) > { > - iterative_hash_canonical_type (TREE_TYPE (f), hstate); > + iterative_hash_canonical_type (TREE_TYPE (f), hstate, insert); > nf++; > } > > @@ -332,10 +337,11 @@ hash_canonical_type (tree type) > return hstate.end(); > } > > -/* Returning a hash value for gimple type TYPE combined with VAL. */ > +/* Returning a hash value for gimple type TYPE combined with VAL. > + If INSERT is true possibly insert TYPE to canonical type hash. */ > > static void > -iterative_hash_canonical_type (tree type, inchash::hash &hstate) > +iterative_hash_canonical_type (tree type, inchash::hash &hstate, bool insert) > { > hashval_t v; > > @@ -343,7 +349,7 @@ iterative_hash_canonical_type (tree type > type = TYPE_MAIN_VARIANT (type); > > if (!canonical_type_used_p (type)) > - v = hash_canonical_type (type); > + v = hash_canonical_type (type, insert); > /* An already processed type. */ > else if (TYPE_CANONICAL (type)) > { > @@ -355,9 +361,11 @@ iterative_hash_canonical_type (tree type > /* Canonical types should not be able to form SCCs by design, this > recursion is just because we do not register canonical types in > optimal order. To avoid quadratic behavior also register the > - type here. */ > - v = hash_canonical_type (type); > - gimple_register_canonical_type_1 (type, v); > + type here. Do not assign canonical types to ODR types - this > + is done later using ODR name hash. */ > + v = hash_canonical_type (type, insert); > + if (insert) > + gimple_register_canonical_type_1 (type, v); So the comment explains that gimple_register_canonical_type_1 is to avoid quadratic behavior for types referenced multiple times. You remove this and I see why but I fear this will cause issues. > } > hstate.add_int (v); > } > @@ -437,12 +445,26 @@ gimple_register_canonical_type (tree t) > TYPE_CANONICAL (t) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); > else > { > - hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t)); > + hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t), true); > gimple_register_canonical_type_1 (TYPE_MAIN_VARIANT (t), h); > TYPE_CANONICAL (t) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); > } > } > > +/* See if structurally equivalent type to T is in the canonical type hash and > + return it. Return NULL otherwise. > + No changes to canonical hash are made during the lookup. */ > + > +static tree > +lookup_canonical_type (tree t) > +{ > + hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t), false); > + void **slot = htab_find_slot_with_hash (gimple_canonical_types, t, h, NO_INSERT); > + if (!slot) > + return NULL; > + return (tree)(*slot); > +} > + > /* Re-compute TYPE_CANONICAL for NODE and related types. */ > > static void > @@ -464,6 +486,107 @@ lto_register_canonical_types (tree node, > gimple_register_canonical_type (node); > } > > +/* Finish canonical type calculation: after all units has been streamed in we > + can check if given ODR type structurally conflicts with a non-ODR type. In > + the first case we set type canonical according to the canonical type hash. > + In the second case we use type names. */ > + > +static void > +lto_register_canonical_types_for_odr_types () > +{ > + tree t; > + unsigned int i; > + > + if (!types_to_register) > + return; > + > + /* Be sure that no types derived from ODR types was > + not inserted into the hash table. */ > + if (flag_checking) > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + gcc_assert (!TYPE_CANONICAL (t)); > + > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + if (odr_type_p (t) && !TYPE_CANONICAL (t)) > + { > + /* Punt on ODR violations - if there are structurally different > + types of the same name we are better to not try too hard to > + use TBAA. */ > + if (odr_type_violation_reported_p (t)) > + { > + if (symtab->dump_file) > + { > + fprintf (symtab->dump_file, > + "Disabling ODR TBAA because of ODR violation: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, "\n"); > + } But you don't actually do anything to the type? > + } > + else > + { > + tree nonodr = type_in_anonymous_namespace_p (t) > + ? NULL : lookup_canonical_type (t); > + > + /* If there is non-ODR type matching T, use its canonical > + type. We can still propagate it to all variants of T > + including incomplete ones. */ > + if (nonodr) > + { > + gcc_checking_assert (!odr_type_p (nonodr)); > + if (symtab->dump_file) > + { > + fprintf (symtab->dump_file, > + "ODR and non-ODR type conflict: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, " and "); > + print_generic_expr (symtab->dump_file, nonodr); > + fprintf (symtab->dump_file, "\n"); > + } > + set_type_canonical_for_odr_type (t, nonodr); > + } > + else > + { > + if (symtab->dump_file) > + { > + fprintf (symtab->dump_file, > + "New canonical ODR type: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, "\n"); > + } > + > + tree prevail = prevailing_odr_type (t); > + gcc_checking_assert (TYPE_CANONICAL (prevail) == NULL); > + > + /* Populate canonical type hash with type name. */ > + bool existed = canonical_type_hash_cache->put > + (prevail, > + htab_hash_string > + (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME > + (TYPE_NAME (prevail))))); but referencing types used a different hash value for this type? (computed multiple times, see that quadraticness issue) Why use a different hash value and why not insert the hash in the cache during first processing? > + gcc_assert (!existed); > + /* Set TYPE_CANONICAL for all variants and duplicates of T > + including incompete ones. */ > + set_type_canonical_for_odr_type (t, prevail); > + /* And inform gimple_canonical_types_compatible_p that > + it should rely on TYPE_CANONICAL compares only. */ > + enable_odr_based_tbaa (t); > + gcc_checking_assert (TYPE_CANONICAL (t) = prevail); > + } > + } > + } > + > + /* Now compute canonical types for all ODR derived types. */ > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + if (!TYPE_CANONICAL (t)) > + { > + gimple_register_canonical_type (t); note this will re-compute the hash value the old way. > + /* We store only main variants; propagate info to all variants. */ > + for (tree t2 = TYPE_NEXT_VARIANT (t); t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = TYPE_CANONICAL (t); > + } > +} > + > > /* Remember trees that contains references to declarations. */ > vec <tree, va_gc> *tree_with_vars; > @@ -1655,6 +1778,33 @@ unify_scc (struct data_in *data_in, unsi > } > > > +/* TYPE is expected to be record or union. Figure out if it is an ODR > + type or derived from it. This is intended to be used only from > + stream in process and rely on fact that all types with TYPE_CANONICAL > + set are not ODR derived. This reducts most of recursive lookups. */ Still don't understand this issue fully but I don't really like this... iff then the FE should set this and we should stream it as extra bit. > +static bool > +odr_or_derived_type_p (tree type) > +{ > + if (TYPE_CANONICAL (type)) > + return false; > + if (odr_type_p (TYPE_MAIN_VARIANT (type)) > + || (TYPE_CONTEXT (type) > + && TYPE_P (TYPE_CONTEXT (type)) > + && odr_type_p (TYPE_CONTEXT (type)))) > + return true; > + for (tree f = TYPE_FIELDS (type); f; f = TREE_CHAIN (f)) > + { > + tree t = TREE_TYPE (f); > + while (TREE_CODE (t) == ARRAY_TYPE) > + t = TREE_TYPE (t); > + if (RECORD_OR_UNION_TYPE_P (t) > + && odr_or_derived_type_p (t)) > + return true; > + } > + return false; > +} > + > /* Read all the symbols from buffer DATA, using descriptors in DECL_DATA. > RESOLUTIONS is the set of symbols picked by the linker (read from the > resolution file when the linker plugin is being used). */ > @@ -1747,12 +1897,24 @@ lto_read_decls (struct lto_file_decl_dat > num_prevailing_types++; > lto_fixup_prevailing_type (t); > > - /* Compute the canonical type of all types. > + /* Compute the canonical type of all non-ODR types. > + Delay ODR types for the end of merging process - the canonical > + type for those can be computed using the (unique) name however > + we want to do this only if units in other languages do not > + contain structurally equivalent type. > + > Because SCC components are streamed in random (hash) order > we may have encountered the type before while registering > type canonical of a derived type in the same SCC. */ > if (!TYPE_CANONICAL (t)) > - gimple_register_canonical_type (t); > + { > + if (!RECORD_OR_UNION_TYPE_P (t) > + || !odr_or_derived_type_p (t)) > + gimple_register_canonical_type (t); > + else if (COMPLETE_TYPE_P (t) > + && TYPE_MAIN_VARIANT (t) == t) > + vec_safe_push (types_to_register, t); > + } > if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t)) > register_odr_type (t); > } > @@ -2602,6 +2764,8 @@ read_cgraph_and_symbols (unsigned nfiles > ggc_free(decl_data); > real_file_decl_data = NULL; > > + lto_register_canonical_types_for_odr_types (); > + > if (resolution_file_name) > fclose (resolution); > > Index: testsuite/g++.dg/lto/alias-2_0.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_0.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_0.C (working copy) > @@ -0,0 +1,31 @@ > +/* { dg-lto-do run } */ > +/* { dg-lto-options { { -O2 -flto } } } */ > + > +/* With LTO we consider all pointers to incomplete types to be possibly > + aliasing. This makes *bptr to alias with aptr. > + For C++ ODR types we however can work out that they are actually > + different. */ > + > +#include <string.h> > + > +typedef int (*fnptr) (); > + > +__attribute__ ((used)) > +struct a *aptr; > + > +__attribute__ ((used)) > +struct b **bptr = (struct b**)&aptr; > +extern void init (); > +extern void inline_me_late (int); > + > + > +int > +main (int argc, char **argv) > +{ > + init (); > + aptr = 0; > + inline_me_late (argc); > + if (!__builtin_constant_p (aptr == 0)) > + __builtin_abort (); > + return (size_t)aptr; > +} > Index: testsuite/g++.dg/lto/alias-2_1.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_1.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_1.C (working copy) > @@ -0,0 +1,16 @@ > +#include <string.h> > +struct a {int a;} a; > +struct b {int b;} b; > +extern struct b **bptr; > +void > +inline_me_late (int argc) > +{ > + if (argc == -1) > + *bptr = (struct b *)(size_t)1; > +} > +void > +init() > +{ > + a.a=1; > + b.b=2; > +} > Index: tree.c > =================================================================== > --- tree.c (revision 271843) > +++ tree.c (working copy) > @@ -14083,6 +14083,7 @@ gimple_canonical_types_compatible_p (con > > gcc_assert (!trust_type_canonical > || (type_with_alias_set_p (t1) && type_with_alias_set_p (t2))); > + > /* If the types have been previously registered and found equal > they still are. */ > > @@ -14100,6 +14101,14 @@ gimple_canonical_types_compatible_p (con > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > } > > + /* For types where we do ODR based TBAA the canonical type is always > + set correctly, so we know that types are different if their > + canonical types does not match. */ > + if (trust_type_canonical > + && (odr_type_p (t1) && odr_based_tbaa_p (t1)) > + != (odr_type_p (t2) && odr_based_tbaa_p (t2))) > + return false; > + ? But then TYPE_CANONICAL is properly set and thus the code above catched it? It looks like this is for the transitional case where TYPE_CANONICAL was delayed? If so it doesn't really belong here but in the (hopefully single...) caller that matters? Richard. > /* Can't be the same type if the types don't have the same code. */ > enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1)); > if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2))) >
> > Step 2 remain same. > > Additional step 3 registers all odr derived types into canonical type hash. > > This requires canonical type hash to play well with ODR types (i.e. not > > consider them equivalent based on structural equivalety). > > > > The decision on whether given type has ODR based canonical > > type is stored in odr_based_tbaa_p and is used > > 1) by gimple_canonical_types_compatible_p so the comparsion is > > behaving well after ODR types are inserted into the hash > > 2) by alias.c to be build more precise alias sets - for ODR types > > it is not necessary to glob pointers into void *. > > There is no need to update canonical type hash since I simply insert > > hash codes of ODR names into canonical type hash cache. > > ICK. Can you please solve the C++ issue differently? The patch > also seems to do many other things ... What do you mean by C++ issue here? I do not think it is a problem of C++, but rather how type system work. It has named structures/enums/unions and unnamed. The unnamed ones needs to be handled same way as canonical type hasing. For named ones we want to use type names unless we decide to unify them with types from other languages that does not do ODR. So first we figure out what clashes we have and then we insert remaining types. > > + /* LTO non-ODR type merging does not make any difference between > > + component pointer types. We may have > > + > > + struct foo {int *a;}; > > + > > + as TYPE_CANONICAL of > > + > > + struct bar {float *a;}; > > + > > + Because accesses to int * and float * do not alias, we would get > > + false negative when accessing the same memory location by > > + float ** and bar *. We thus record the canonical type as: > > + > > + struct {void *a;}; > > + > > + void * is special cased and works as a universal pointer type. > > + Accesses to it conflicts with accesses to any other pointer > > + type. */ > > + bool void_pointers = in_lto_p > > + && (!odr_type_p (type) > > + || !odr_based_tbaa_p (type)); > > This seems like an independent improvement to me. Kind of - the way we glob pointers here must match way we glob them while deciding on canonical types (or we may just do subtypes walk for every type merged into canonical type to get better info). So w/o the rest of patch this would be wrong. But I can do this incrementaly - first refine canonical types and then refine aliasing DAG. > > Index: ipa-devirt.c > > =================================================================== > > --- ipa-devirt.c (revision 271843) > > +++ ipa-devirt.c (working copy) > > @@ -213,6 +213,8 @@ struct GTY(()) odr_type_d > > bool odr_violated; > > /* Set when virtual table without RTTI previaled table with. */ > > bool rtti_broken; > > + /* Set when we need to give up on ODR based TBAA info. */ > > + bool tbaa_enabled; > > ? I will fix the comment, used to have disabled flag. This is about handling structurally equivalent ODR types. > > @@ -332,10 +337,11 @@ hash_canonical_type (tree type) > > return hstate.end(); > > } > > > > -/* Returning a hash value for gimple type TYPE combined with VAL. */ > > +/* Returning a hash value for gimple type TYPE combined with VAL. > > + If INSERT is true possibly insert TYPE to canonical type hash. */ > > > > static void > > -iterative_hash_canonical_type (tree type, inchash::hash &hstate) > > +iterative_hash_canonical_type (tree type, inchash::hash &hstate, bool insert) > > { > > hashval_t v; > > > > @@ -343,7 +349,7 @@ iterative_hash_canonical_type (tree type > > type = TYPE_MAIN_VARIANT (type); > > > > if (!canonical_type_used_p (type)) > > - v = hash_canonical_type (type); > > + v = hash_canonical_type (type, insert); > > /* An already processed type. */ > > else if (TYPE_CANONICAL (type)) > > { > > @@ -355,9 +361,11 @@ iterative_hash_canonical_type (tree type > > /* Canonical types should not be able to form SCCs by design, this > > recursion is just because we do not register canonical types in > > optimal order. To avoid quadratic behavior also register the > > - type here. */ > > - v = hash_canonical_type (type); > > - gimple_register_canonical_type_1 (type, v); > > + type here. Do not assign canonical types to ODR types - this > > + is done later using ODR name hash. */ > > + v = hash_canonical_type (type, insert); > > + if (insert) > > + gimple_register_canonical_type_1 (type, v); > > So the comment explains that gimple_register_canonical_type_1 is to > avoid quadratic behavior for types referenced multiple times. > You remove this and I see why but I fear this will cause issues. I was benmarking this on firefox. Thing is that the hash is not very large and we do not have that many types even for large programs, so walking it multiple times does not seem much of issue. I can introduce cache here that will put computed hash values when insert is false and also look it up. In a way perhaps this is better to do if we actually get performance problem. But can do it proactively. > > + /* Punt on ODR violations - if there are structurally different > > + types of the same name we are better to not try too hard to > > + use TBAA. */ > > + if (odr_type_violation_reported_p (t)) > > + { > > + if (symtab->dump_file) > > + { > > + fprintf (symtab->dump_file, > > + "Disabling ODR TBAA because of ODR violation: "); > > + print_generic_expr (symtab->dump_file, t); > > + fprintf (symtab->dump_file, "\n"); > > + } > > But you don't actually do anything to the type? Yes, but not setting the canonical type the last loop will process it as if it was unnamed type and use structural hash. > > + /* Populate canonical type hash with type name. */ > > + bool existed = canonical_type_hash_cache->put > > + (prevail, > > + htab_hash_string > > + (IDENTIFIER_POINTER > > + (DECL_ASSEMBLER_NAME > > + (TYPE_NAME (prevail))))); > > but referencing types used a different hash value for this type? > (computed multiple times, see that quadraticness issue) I never insert ODR derived types into the hash during streaming in, so this is safe (as described in the comment). During the last pass I add them bassed on the hash value I computed here. > > Why use a different hash value and why not insert the hash in > the cache during first processing? We could get more conflicts then, but it is probalby not important - it just seemed that given we have easy way to hash names well, we should do it :) > > > + gcc_assert (!existed); > > + /* Set TYPE_CANONICAL for all variants and duplicates of T > > + including incompete ones. */ > > + set_type_canonical_for_odr_type (t, prevail); > > + /* And inform gimple_canonical_types_compatible_p that > > + it should rely on TYPE_CANONICAL compares only. */ > > + enable_odr_based_tbaa (t); > > + gcc_checking_assert (TYPE_CANONICAL (t) = prevail); > > + } > > + } > > + } > > + > > + /* Now compute canonical types for all ODR derived types. */ > > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > > + if (!TYPE_CANONICAL (t)) > > + { > > + gimple_register_canonical_type (t); > > note this will re-compute the hash value the old way. Yes, t is type referring to ODR types but not having name by itself (or we decided to give up because of ODR violation). We will compute has now but because the hash values of types with canonical types are hashed, we will use ODR names for ODR types. This way we merge i.e. struct a {int b}; struct {struct a c,d;}; and struct a {int b}; struct {struct a c,d;}; from two different units under assumption that for some reason type merging did not merge "struct a" and "struct a" for whatever reason (like difference in virtual table). The first type is ODR type so it will be added into hash after verifying that there are no reasons to give up and in this loop we will structural compare two copies of struct {struct a c,d;}; struct {struct a c,d;}; assuming type merging failed earlier and give them same canoincal type. > > +/* TYPE is expected to be record or union. Figure out if it is an ODR > > + type or derived from it. This is intended to be used only from > > + stream in process and rely on fact that all types with TYPE_CANONICAL > > + set are not ODR derived. This reducts most of recursive lookups. */ > > Still don't understand this issue fully but I don't really like this... > iff then the FE should set this and we should stream it as extra bit. Hope the above example makes it cleaner. We could stream extra bit to differentiate C++ struct {struct a c,d;}; form C struct {struct a c,d;}; Since first is referring ODR type struct a, while second is not. actually I also think it would be cleaner. I am not sure where to put it in though. > > > > + /* For types where we do ODR based TBAA the canonical type is always > > + set correctly, so we know that types are different if their > > + canonical types does not match. */ > > + if (trust_type_canonical > > + && (odr_type_p (t1) && odr_based_tbaa_p (t1)) > > + != (odr_type_p (t2) && odr_based_tbaa_p (t2))) > > + return false; > > + > > ? But then TYPE_CANONICAL is properly set and thus the code above > catched it? It looks like this is for the transitional case where > TYPE_CANONICAL was delayed? If so it doesn't really belong here > but in the (hopefully single...) caller that matters? Consider struct a {int a;} struct b {struct a a;} struct {struct a a;} Now "struct b" will have TYPE_CANONICAl but "struct <unnamed>" not yet. We want to figure out that they are not equivalent. Honza
Hi,
here is patch that adds TYPE_ODR_P to determine type that comply C++
ODR rules (i.e. ODR types themselves or structures/unions derived
from them).
I have decided to use STRING_FLAG which have meaning only for integers
and arrays which forced me to add type checks on places where
we check STRING_FLAG on other types.
The patch also let me to verify that all types we consider to have
linkage actually are created by C++ FE which turned out to not be the
case for Ada which I fixed in needs_assembler_name_p.
Bootstrapped/regtested x86_64-linux, OK?
* ipa-utils.h (type_with_linkage_p): Verify that type is
CXX_ODR_P.
(odr_type_p): Remove extra return.
* lto-streamer-out.c (hash_tree): Hash TYPE_CXX_ODR_P;
hash STRING_FLAG only for arrays and integers.
* tree-stremaer-in.c (unpack_ts_type_common_value_fields):
Update analogously.
* tree-streamer-out.c (pack_ts_type_common_value_fields):
Likewise.
* print-tree.c (print_node): Print cxx-odr-p
and string-flag.
* tree.c (need_assembler_name_p): Also check that type
is CXX_ODR_TYPE_P
(verify_type_variant): Update verification of SRING_FLAG;
also check CXX_ODR_P.
* tree.h (ARRAY_OR_INTEGER_TYPE_CHECK): New macro.
(TYPE_STRING_FLAG): Use it.
(TYPE_CXX_ODR_P): New macro.
* lto-common.c (compare_tree_sccs_1): Compare CXX_ODR_P;
compare STRING_FLAG only for arrays and integers.
* gcc-interface/decl.c (gnat_to_gnu_entity): Check that
type is array or integer prior checking string flag.
* gcc-interface/gigi.h (gnat_signed_type_for,
maybe_character_value): Likewise.
* c-common.c (braced_lists_to_strings): Check that
type is array or integer prior checking string flag.
* lex.c (cxx_make_type): Set TYPE_CXX_ODR_P.
* dwarf2out.c (gen_array_type_die): First check that type
is an array and then test string flag.
* trans-expr.c (gfc_conv_substring): Check that
type is array or integer prior checking string flag.
(gfc_conv_string_parameter): Likewise.
* trans-openmp.c (gfc_omp_scalar_p): Likewise.
* trans.c (gfc_build_array_ref): Likewise.
Index: ada/gcc-interface/decl.c
===================================================================
--- ada/gcc-interface/decl.c (revision 272353)
+++ ada/gcc-interface/decl.c (working copy)
@@ -1855,7 +1855,9 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
= Has_Biased_Representation (gnat_entity);
/* Do the same processing for Character subtypes as for types. */
- if (TYPE_STRING_FLAG (TREE_TYPE (gnu_type)))
+ if ((TREE_CODE (TREE_TYPE (gnu_type)) == INTEGER_TYPE
+ || TREE_CODE (TREE_TYPE (gnu_type)) == ARRAY_TYPE)
+ && TYPE_STRING_FLAG (TREE_TYPE (gnu_type)))
{
TYPE_NAME (gnu_type) = gnu_entity_name;
TYPE_STRING_FLAG (gnu_type) = 1;
Index: ada/gcc-interface/gigi.h
===================================================================
--- ada/gcc-interface/gigi.h (revision 272353)
+++ ada/gcc-interface/gigi.h (working copy)
@@ -1138,7 +1138,8 @@ gnat_signed_type_for (tree type_node)
static inline tree
maybe_character_type (tree type)
{
- if (TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type))
+ if (TREE_CODE (type) == INTEGER_TYPE
+ && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type))
type = gnat_unsigned_type_for (type);
return type;
@@ -1151,7 +1152,8 @@ maybe_character_value (tree expr)
{
tree type = TREE_TYPE (expr);
- if (TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type))
+ if (TREE_CODE (type) == INTEGER_TYPE
+ && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type))
{
type = gnat_unsigned_type_for (type);
expr = convert (type, expr);
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c (revision 272353)
+++ c-family/c-common.c (working copy)
@@ -8847,7 +8847,8 @@ braced_lists_to_strings (tree type, tree
else
return ctor;
- if (TYPE_STRING_FLAG (ttp))
+ if ((TREE_CODE (ttp) == ARRAY_TYPE || TREE_CODE (ttp) == INTEGER_TYPE)
+ && TYPE_STRING_FLAG (ttp))
return braced_list_to_string (type, ctor);
code = TREE_CODE (ttp);
Index: cp/lex.c
===================================================================
--- cp/lex.c (revision 272353)
+++ cp/lex.c (working copy)
@@ -884,6 +884,9 @@ cxx_make_type (enum tree_code code MEM_S
CLASSTYPE_INTERFACE_ONLY (t) = finfo->interface_only;
}
+ if (code == RECORD_TYPE || code == UNION_TYPE)
+ TYPE_CXX_ODR_P (t) = 1;
+
return t;
}
Index: dwarf2out.c
===================================================================
--- dwarf2out.c (revision 272353)
+++ dwarf2out.c (working copy)
@@ -21850,8 +21850,8 @@ gen_array_type_die (tree type, dw_die_re
/* Emit DW_TAG_string_type for Fortran character types (with kind 1 only, as
DW_TAG_string_type doesn't have DW_AT_type attribute). */
- if (TYPE_STRING_FLAG (type)
- && TREE_CODE (type) == ARRAY_TYPE
+ if (TREE_CODE (type) == ARRAY_TYPE
+ && TYPE_STRING_FLAG (type)
&& is_fortran ()
&& TYPE_MODE (TREE_TYPE (type)) == TYPE_MODE (char_type_node))
{
Index: fortran/trans-expr.c
===================================================================
--- fortran/trans-expr.c (revision 272353)
+++ fortran/trans-expr.c (working copy)
@@ -2309,7 +2309,9 @@ gfc_conv_substring (gfc_se * se, gfc_ref
start.expr = gfc_evaluate_now (start.expr, &se->pre);
/* Change the start of the string. */
- if (TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
+ if ((TREE_CODE (TREE_TYPE (se->expr)) == ARRAY_TYPE
+ || TREE_CODE (TREE_TYPE (se->expr)) == INTEGER_TYPE)
+ && TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
tmp = se->expr;
else
tmp = build_fold_indirect_ref_loc (input_location,
@@ -9479,7 +9481,9 @@ gfc_conv_string_parameter (gfc_se * se)
return;
}
- if (TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
+ if ((TREE_CODE (TREE_TYPE (se->expr)) == ARRAY_TYPE
+ || TREE_CODE (TREE_TYPE (se->expr)) == INTEGER_TYPE)
+ && TYPE_STRING_FLAG (TREE_TYPE (se->expr)))
{
if (TREE_CODE (se->expr) != INDIRECT_REF)
{
Index: fortran/trans-openmp.c
===================================================================
--- fortran/trans-openmp.c (revision 272353)
+++ fortran/trans-openmp.c (working copy)
@@ -1222,7 +1222,8 @@ gfc_omp_scalar_p (tree decl)
|| GFC_CLASS_TYPE_P (type))
return false;
}
- if (TYPE_STRING_FLAG (type))
+ if ((TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == INTEGER_TYPE)
+ && TYPE_STRING_FLAG (type))
return false;
if (INTEGRAL_TYPE_P (type)
|| SCALAR_FLOAT_TYPE_P (type)
Index: fortran/trans.c
===================================================================
--- fortran/trans.c (revision 272353)
+++ fortran/trans.c (working copy)
@@ -418,7 +418,8 @@ gfc_build_array_ref (tree base, tree off
tmp = gfc_build_addr_expr (pvoid_type_node, base);
tmp = fold_build_pointer_plus_loc (input_location, tmp, offset);
tmp = fold_convert (build_pointer_type (type), tmp);
- if (!TYPE_STRING_FLAG (type))
+ if ((TREE_CODE (type) != INTEGER_TYPE && TREE_CODE (type) != ARRAY_TYPE)
+ || !TYPE_STRING_FLAG (type))
tmp = build_fold_indirect_ref_loc (input_location, tmp);
return tmp;
}
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 272353)
+++ ipa-utils.h (working copy)
@@ -205,6 +205,8 @@ type_with_linkage_p (const_tree t)
if (!TYPE_CONTEXT (t))
return false;
+ gcc_checking_assert (TREE_CODE (t) == ENUMERAL_TYPE || TYPE_CXX_ODR_P (t));
+
return true;
}
@@ -240,7 +242,6 @@ odr_type_p (const_tree t)
gcc_checking_assert (in_lto_p || flag_lto);
return TYPE_NAME (t) && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL
&& DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t));
- return false;
}
#endif /* GCC_IPA_UTILS_H */
Index: lto/lto-common.c
===================================================================
--- lto/lto-common.c (revision 272353)
+++ lto/lto-common.c (working copy)
@@ -1124,15 +1124,17 @@ compare_tree_sccs_1 (tree t1, tree t2, t
if (CODE_CONTAINS_STRUCT (code, TS_TYPE_COMMON))
{
compare_values (TYPE_MODE);
- compare_values (TYPE_STRING_FLAG);
compare_values (TYPE_NEEDS_CONSTRUCTING);
if (RECORD_OR_UNION_TYPE_P (t1))
{
compare_values (TYPE_TRANSPARENT_AGGR);
compare_values (TYPE_FINAL_P);
+ compare_values (TYPE_CXX_ODR_P);
}
else if (code == ARRAY_TYPE)
compare_values (TYPE_NONALIASED_COMPONENT);
+ if (code == ARRAY_TYPE || code == INTEGER_TYPE)
+ compare_values (TYPE_STRING_FLAG);
if (AGGREGATE_TYPE_P (t1))
compare_values (TYPE_TYPELESS_STORAGE);
compare_values (TYPE_EMPTY_P);
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c (revision 272353)
+++ lto-streamer-out.c (working copy)
@@ -1143,7 +1143,6 @@ hash_tree (struct streamer_tree_cache_d
if (CODE_CONTAINS_STRUCT (code, TS_TYPE_COMMON))
{
hstate.add_hwi (TYPE_MODE (t));
- hstate.add_flag (TYPE_STRING_FLAG (t));
/* TYPE_NO_FORCE_BLK is private to stor-layout and need
no streaming. */
hstate.add_flag (TYPE_PACKED (t));
@@ -1154,9 +1153,12 @@ hash_tree (struct streamer_tree_cache_d
{
hstate.add_flag (TYPE_TRANSPARENT_AGGR (t));
hstate.add_flag (TYPE_FINAL_P (t));
+ hstate.add_flag (TYPE_CXX_ODR_P (t));
}
else if (code == ARRAY_TYPE)
hstate.add_flag (TYPE_NONALIASED_COMPONENT (t));
+ if (code == ARRAY_TYPE || code == INTEGER_TYPE)
+ hstate.add_flag (TYPE_STRING_FLAG (t));
if (AGGREGATE_TYPE_P (t))
hstate.add_flag (TYPE_TYPELESS_STORAGE (t));
hstate.commit_flag ();
Index: print-tree.c
===================================================================
--- print-tree.c (revision 272353)
+++ print-tree.c (working copy)
@@ -601,7 +601,7 @@ print_node (FILE *file, const char *pref
if (TYPE_NO_FORCE_BLK (node))
fputs (" no-force-blk", file);
- if (TYPE_STRING_FLAG (node))
+ if (code == ARRAY_TYPE && TYPE_STRING_FLAG (node))
fputs (" string-flag", file);
if (TYPE_NEEDS_CONSTRUCTING (node))
@@ -614,6 +614,11 @@ print_node (FILE *file, const char *pref
&& TYPE_REVERSE_STORAGE_ORDER (node))
fputs (" reverse-storage-order", file);
+ if ((code == RECORD_TYPE
+ || code == UNION_TYPE)
+ && TYPE_CXX_ODR_P (node))
+ fputs (" cxx-odr-p", file);
+
/* The transparent-union flag is used for different things in
different nodes. */
if ((code == UNION_TYPE || code == RECORD_TYPE)
Index: tree-streamer-in.c
===================================================================
--- tree-streamer-in.c (revision 272353)
+++ tree-streamer-in.c (working copy)
@@ -372,7 +372,6 @@ unpack_ts_type_common_value_fields (stru
mode = bp_unpack_machine_mode (bp);
SET_TYPE_MODE (expr, mode);
- TYPE_STRING_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1);
/* TYPE_NO_FORCE_BLK is private to stor-layout and need
no streaming. */
TYPE_PACKED (expr) = (unsigned) bp_unpack_value (bp, 1);
@@ -383,9 +382,12 @@ unpack_ts_type_common_value_fields (stru
{
TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+ TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
}
else if (TREE_CODE (expr) == ARRAY_TYPE)
TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
+ if (TREE_CODE (expr) == ARRAY_TYPE || TREE_CODE (expr) == INTEGER_TYPE)
+ TYPE_STRING_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1);
if (AGGREGATE_TYPE_P (expr))
TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1);
TYPE_EMPTY_P (expr) = (unsigned) bp_unpack_value (bp, 1);
Index: tree-streamer-out.c
===================================================================
--- tree-streamer-out.c (revision 272353)
+++ tree-streamer-out.c (working copy)
@@ -319,7 +319,6 @@ pack_ts_type_common_value_fields (struct
not necessary valid in a global context.
Use the raw value previously set by layout_type. */
bp_pack_machine_mode (bp, TYPE_MODE_RAW (expr));
- bp_pack_value (bp, TYPE_STRING_FLAG (expr), 1);
/* TYPE_NO_FORCE_BLK is private to stor-layout and need
no streaming. */
bp_pack_value (bp, TYPE_PACKED (expr), 1);
@@ -333,9 +332,12 @@ pack_ts_type_common_value_fields (struct
{
bp_pack_value (bp, TYPE_TRANSPARENT_AGGR (expr), 1);
bp_pack_value (bp, TYPE_FINAL_P (expr), 1);
+ bp_pack_value (bp, TYPE_CXX_ODR_P (expr), 1);
}
else if (TREE_CODE (expr) == ARRAY_TYPE)
bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
+ if (TREE_CODE (expr) == ARRAY_TYPE || TREE_CODE (expr) == INTEGER_TYPE)
+ bp_pack_value (bp, TYPE_STRING_FLAG (expr), 1);
if (AGGREGATE_TYPE_P (expr))
bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1);
bp_pack_value (bp, TYPE_EMPTY_P (expr), 1);
Index: tree.c
===================================================================
--- tree.c (revision 272353)
+++ tree.c (working copy)
@@ -5598,6 +5598,9 @@ need_assembler_name_p (tree decl)
&& decl == TYPE_NAME (TREE_TYPE (decl))
&& TYPE_MAIN_VARIANT (TREE_TYPE (decl)) == TREE_TYPE (decl)
&& !TYPE_ARTIFICIAL (TREE_TYPE (decl))
+ && ((TREE_CODE (TREE_TYPE (decl)) != RECORD_TYPE
+ && TREE_CODE (TREE_TYPE (decl)) != UNION_TYPE)
+ || TYPE_CXX_ODR_P (TREE_TYPE (decl)))
&& (type_with_linkage_p (TREE_TYPE (decl))
|| TREE_CODE (TREE_TYPE (decl)) == INTEGER_TYPE)
&& !variably_modified_type_p (TREE_TYPE (decl), NULL_TREE))
@@ -13881,7 +13884,10 @@ verify_type_variant (const_tree t, tree
Ada also builds variants of types with different TYPE_CONTEXT. */
if ((!in_lto_p || !TYPE_FILE_SCOPE_P (t)) && 0)
verify_variant_match (TYPE_CONTEXT);
- verify_variant_match (TYPE_STRING_FLAG);
+ if (TREE_CODE (t) == ARRAY_TYPE || TREE_CODE (t) == INTEGER_TYPE)
+ verify_variant_match (TYPE_STRING_FLAG);
+ if (TREE_CODE (t) == RECORD_TYPE || TREE_CODE (t) == UNION_TYPE)
+ verify_variant_match (TYPE_CXX_ODR_P);
if (TYPE_ALIAS_SET_KNOWN_P (t))
{
error ("type variant with %<TYPE_ALIAS_SET_KNOWN_P%>");
@@ -14627,12 +14633,6 @@ verify_type (const_tree t)
error ("%<TYPE_CACHED_VALUES_P%> is set while it should not be");
error_found = true;
}
- if (TYPE_STRING_FLAG (t)
- && TREE_CODE (t) != ARRAY_TYPE && TREE_CODE (t) != INTEGER_TYPE)
- {
- error ("%<TYPE_STRING_FLAG%> is set on wrong type code");
- error_found = true;
- }
/* ipa-devirt makes an assumption that TYPE_METHOD_BASETYPE is always
TYPE_MAIN_VARIANT and it would be odd to add methods only to variatns
Index: tree.h
===================================================================
--- tree.h (revision 272353)
+++ tree.h (working copy)
@@ -439,6 +439,8 @@ extern void omp_clause_range_check_faile
TREE_CHECK3 (T, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE)
#define NOT_RECORD_OR_UNION_CHECK(T) \
TREE_NOT_CHECK3 (T, RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE)
+#define ARRAY_OR_INTEGER_TYPE_CHECK(T) \
+ TREE_CHECK2 (T, ARRAY_TYPE, INTEGER_TYPE)
#define NUMERICAL_TYPE_CHECK(T) \
TREE_CHECK5 (T, INTEGER_TYPE, ENUMERAL_TYPE, BOOLEAN_TYPE, REAL_TYPE, \
@@ -2118,7 +2120,14 @@ extern machine_mode vector_type_mode (co
/* If set in an ARRAY_TYPE, indicates a string type (for languages
that distinguish string from array of char).
If set in a INTEGER_TYPE, indicates a character type. */
-#define TYPE_STRING_FLAG(NODE) (TYPE_CHECK (NODE)->type_common.string_flag)
+#define TYPE_STRING_FLAG(NODE) \
+ (ARRAY_OR_INTEGER_TYPE_CHECK (NODE)->type_common.string_flag)
+
+/* If set for RECORD_TYPE or UNION_TYPE it indicates that the type conforms
+ to the C++ one definition rule. This is used for LTO canonical type
+ computation. */
+#define TYPE_CXX_ODR_P(NODE) \
+ (RECORD_OR_UNION_CHECK (NODE)->type_common.string_flag)
/* Nonzero in a VECTOR_TYPE if the frontends should not emit warnings
about missing conversions to other vector types of the same size. */
> > ICK. Can you please solve the C++ issue differently? The patch > also seems to do many other things ... If you refer to the fact that C++ seem to create non-ODR types that are structurally equivalent to ODR types, i tracked it down. Testcase is testsuite/g++.dg/lto/20080904_0.C compiled with -O0 -flto. Then we get structural match of: truct Base struct { int (*__vtbl_ptr_type) () * _vptr.Base; char * _buf; unsigned int _len; } More precisely: <record_type 0x7ffff6963a80 Base addressable cxx-odr-p type_1 type_5 type_6 BLK size <integer_cst 0x7ffff682d2e8 type <integer_type 0x7ffff68290a8 bitsizetype> constant 192> unit-size <integer_cst 0x7ffff682d2b8 type <integer_type 0x7ffff6829000 sizetype> constant 24> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6963a80 fields <field_decl 0x7ffff6930980 _vptr.Base type <pointer_type 0x7ffff6963540 type <pointer_type 0x7ffff69631f8 __vtbl_ptr_type> unsigned DI size <integer_cst 0x7ffff680ce28 constant 64> unit-size <integer_cst 0x7ffff680ce40 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6963540> unsigned virtual DI a.C:5:7 size <integer_cst 0x7ffff680ce28 64> unit-size <integer_cst 0x7ffff680ce40 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff680ce58 constant 0> bit-offset <integer_cst 0x7ffff680cea0 constant 0> context <record_type 0x7ffff6963a80 Base> chain <field_decl 0x7ffff6930850 _buf type <pointer_type 0x7ffff6834e70> used private unsigned nonlocal decl_3 DI a.C:15:10 size <integer_cst 0x7ffff680ce28 64> unit-size <integer_cst 0x7ffff680ce40 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff680ce58 0> bit-offset <integer_cst 0x7ffff680ce28 64> context <record_type 0x7ffff6963a80 Base> chain <field_decl 0x7ffff69308e8 _len>>> context <translation_unit_decl 0x7ffff6819168 a.C> pointer_to_this <pointer_type 0x7ffff6978150> reference_to_this <reference_type 0x7ffff69815e8>> <record_type 0x7ffff69783f0 BLK size <integer_cst 0x7ffff69741c8 type <integer_type 0x7ffff68290a8 bitsizetype> constant 160> unit-size <integer_cst 0x7ffff69741f8 type <integer_type 0x7ffff6829000 sizetype> constant 20> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff69783f0 fields <field_decl 0x7ffff6930a18 _vptr.Base type <pointer_type 0x7ffff6963540 type <pointer_type 0x7ffff69631f8 __vtbl_ptr_type> unsigned DI size <integer_cst 0x7ffff680ce28 constant 64> unit-size <integer_cst 0x7ffff680ce40 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff6963540> unsigned virtual DI a.C:5:7 size <integer_cst 0x7ffff680ce28 64> unit-size <integer_cst 0x7ffff680ce40 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff680ce58 constant 0> bit-offset <integer_cst 0x7ffff680cea0 constant 0> context <record_type 0x7ffff69783f0> chain <field_decl 0x7ffff6930ab0 _buf type <pointer_type 0x7ffff6834e70> private unsigned nonlocal decl_3 DI a.C:15:10 size <integer_cst 0x7ffff680ce28 64> unit-size <integer_cst 0x7ffff680ce40 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff680ce58 0> bit-offset <integer_cst 0x7ffff680ce28 64> context <record_type 0x7ffff69783f0> chain <field_decl 0x7ffff6930b48 _len>>> context <record_type 0x7ffff6963a80 Base> reference_to_this <reference_type 0x7ffff69789d8>> which differ by nothing except of TYPE_NAME and TYPE_SIZE. They are created here. if (CLASSTYPE_NON_LAYOUT_POD_P (t) || CLASSTYPE_EMPTY_P (t)) { /* T needs a different layout as a base (eliding virtual bases or whatever). Create that version. */ tree base_t = make_node (TREE_CODE (t)); which makes sense I suppose, but confuses ODR logic and also goes odd way through canonical type calculations. In non-LTO alias sets are same because C++ FE does: 31 cxx_get_alias_set (tree t) 32 { 33 if (IS_FAKE_BASE_TYPE (t)) 34 /* The base variant of a type must be in the same alias set as the 35 complete type. */ 36 return get_alias_set (TYPE_CONTEXT (t)); I am not quite sure why it is not simply copying the TYPE_CANONICAL when the type is created instead. In LTO I think they may end up with different canonical types and later alias sets because they may have different virtual bases. I am not sure why this does not seem to reak with TBAA, but will try to get testcase where both types are structurally different (and do not differ only by size). Perhaps we want to expose IS_FAKE_BASE to middle end to get things working more reasonably. Honza
Hi,
Jason, this is about C++ frontend producing two copies of same type with
different TYPE_CANONICAL but same get_alias_set. Since the types are
structurally different, this does not go well with LTO which no longer
sees they are same. They are created in
if (CLASSTYPE_NON_LAYOUT_POD_P (t) || CLASSTYPE_EMPTY_P (t))
{
/* T needs a different layout as a base (eliding virtual bases
or whatever). Create that version. */
tree base_t = make_node (TREE_CODE (t));
and later cxx_get_alias_set arranges they have same set by testing
IS_FAKE_BASE_TYPE.
I am not able to come with a testcase that triggers wrong code with LTO
However compiling this testcase:
struct E {
~E();
virtual void f() const;
};
struct B : E {};
struct G : virtual B {};
struct A {
virtual ~A();
};
struct J : E {
int val;
~J() {if (val) __builtin_abort ();}
void f() const {
E *p = 0;
p->f();
}
};
J h;
struct I : A, G, virtual B {};
Front-end produce two versions of J that do not pass
gimple_canonical_types_compatible_p thus at compile time they have same
alias set and at LTO time they are considered non-conflicting.
Both types are mixed in one function:
J::~J (struct J * const this)
{
int _1;
struct E * _2;
<bb 2> [local count: 1073741824]:
this_4(D)->D.2376._vptr.E = &MEM <int (*__vtbl_ptr_type) ()[3]> [(void *)&_ZTV1J + 16B];
_1 = this_4(D)->val;
if (_1 != 0)
goto <bb 3>; [0.00%]
else
goto <bb 4>; [100.00%]
<bb 3> [count: 0]:
abort ();
<bb 4> [local count: 1073741824]:
_2 = &this_4(D)->D.2376;
E::~E (_2);
MEM[(struct &)this_4(D)] ={v} {CLOBBER};
return;
}
Here "(struct &)" cast is turning struct J into the other, incompatible,
variant. I can't make FE to use the type anywhere else and this is why
I do not get wrong code since we sort of ignore the clobber then
(consider that the memory it guards is not used/initialized).
Jason, is there a way to actually read/write into the copied structure?
Putting same alias set but keeping canonical types different comfuses
same_type_for_tbaa, is there a reason why we do not make canonical types
same?
It seems to me that we want
1) use same TYPE_CANONICAL for both variants of the type
2) teach LTO canonical type merging about fake bases (i.e. add a
middle-end flag to do so).
Or can we avoid these types from getting into instruction stream?
(I suppose not - I guess point of using them in clobber is to get the
size right)
Honza
Jason,
I also wonder if something like this would make sense:
* decl.c (build_clobber_this): Use original basetype if they
match.
Index: decl.c
===================================================================
--- decl.c (revision 272381)
+++ decl.c (working copy)
@@ -15210,7 +15210,11 @@ build_clobber_this ()
tree ctype = current_class_type;
if (!vbases)
- ctype = CLASSTYPE_AS_BASE (ctype);
+ {
+ if (!tree_int_cst_equal (TYPE_SIZE (ctype),
+ TYPE_SIZE (CLASSTYPE_AS_BASE (ctype))))
+ ctype = CLASSTYPE_AS_BASE (ctype);
+ }
tree clobber = build_clobber (ctype);
Having to LTO stream many classtypes twice just for clobber seems
wasteful
call.c already seem to contain similar logic in build_over_call:
it checks both types for match and if they do it uses class type
for the copy and otherwise it builds array and copies it
(I wonder why I does not just copy CLASSTYPE_AS_BASE instead,
though I see that with the type merging bug this may break with LTO)
Honza
> Jason, > I also wonder if something like this would make sense: > > * decl.c (build_clobber_this): Use original basetype if they > match. > > Index: decl.c > =================================================================== > --- decl.c (revision 272381) > +++ decl.c (working copy) > @@ -15210,7 +15210,11 @@ build_clobber_this () > > tree ctype = current_class_type; > if (!vbases) > - ctype = CLASSTYPE_AS_BASE (ctype); > + { > + if (!tree_int_cst_equal (TYPE_SIZE (ctype), > + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) > + ctype = CLASSTYPE_AS_BASE (ctype); > + } > > tree clobber = build_clobber (ctype); I have lto-bootstrapped/regtested the aptch and it seems to work just fine. For tramp3d it reduces number of types: [WPA] read 56122 SCCs of average size 1.127419 [WPA] 63273 tree bodies read in total [WPA] tree SCC table: size 65521, 40469 elements, collision ratio: 1.141428 [WPA] tree SCC max chain length 12 (size 1) [WPA] Compared 3603 SCCs, 657 collisions (0.182348) [WPA] Merged 3488 SCCs [WPA] Merged 3496 tree bodies [WPA] Merged 1949 types [WPA] 24104 types prevailed (31196 associated trees) [WPA] GIMPLE canonical type table: size 16381, 412 elements, 2106 searches, 19 collisions (ratio: 0.009022) [WPA] GIMPLE canonical type pointer-map: 412 elements, 3430 searches to [WPA] read 55182 SCCs of average size 1.098384 [WPA] 60611 tree bodies read in total [WPA] tree SCC table: size 65521, 39444 elements, collision ratio: 1.127024 [WPA] tree SCC max chain length 12 (size 1) [WPA] Compared 3603 SCCs, 657 collisions (0.182348) [WPA] Merged 3488 SCCs [WPA] Merged 3496 tree bodies [WPA] Merged 1949 types [WPA] 22908 types prevailed (28278 associated trees) [WPA] GIMPLE canonical type table: size 16381, 405 elements, 1390 searches, 13 collisions (ratio: 0.009353) [WPA] GIMPLE canonical type pointer-map: 405 elements, 1998 searches since the number of GIMPLE canonical type hash searches roughly corresponds to number of types in the LTO stream, it is down by 50% that is nice. Morever the patch improves TBAA oracle from: Alias oracle query stats: refs_may_alias_p: 3021544 disambiguations, 3321141 queries ref_maybe_used_by_call_p: 7118 disambiguations, 3047138 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries nonoverlapping_component_refs_p: 27 disambiguations, 63312 queries nonoverlapping_component_refs_of_decl_p: 19 disambiguations, 2192 queries aliasing_component_refs_p: 2050 disambiguations, 20312 queries TBAA oracle: 1419961 disambiguations 2917965 queries 555158 are in alias set 0 575113 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 253187 are dependent in the DAG 114546 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 671982 disambiguations, 952515 queries pt_solutions_intersect: 97060 disambiguations, 437910 queries to Alias oracle query stats: refs_may_alias_p: 3243740 disambiguations, 3545965 queries ref_maybe_used_by_call_p: 6861 disambiguations, 3268658 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries nonoverlapping_component_refs_p: 27 disambiguations, 64095 queries nonoverlapping_component_refs_of_decl_p: 19 disambiguations, 2192 queries aliasing_component_refs_p: 2058 disambiguations, 21010 queries TBAA oracle: 1488225 disambiguations 3022119 queries 551266 are in alias set 0 575109 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 290651 are dependent in the DAG 116868 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 679953 disambiguations, 965286 queries pt_solutions_intersect: 97067 disambiguations, 437676 queries This is bit inexpected, but I think it mostly comes from the fact that same_type_for_tbaa currently returns -1 for the fake type and original type. This disables good part of access path querries. I also tested the patch setting TYPE_CANONICAL to be same between original and the fake copy. It works but I have to disable type verifier that wants match between TYPE_MODE and TYPE_SIZE. Honza
On 6/19/19 9:28 AM, Jan Hubicka wrote: >> Jason, >> I also wonder if something like this would make sense: >> >> * decl.c (build_clobber_this): Use original basetype if they >> match. >> >> Index: decl.c >> =================================================================== >> --- decl.c (revision 272381) >> +++ decl.c (working copy) >> @@ -15210,7 +15210,11 @@ build_clobber_this () >> >> tree ctype = current_class_type; >> if (!vbases) >> - ctype = CLASSTYPE_AS_BASE (ctype); >> + { >> + if (!tree_int_cst_equal (TYPE_SIZE (ctype), >> + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) >> + ctype = CLASSTYPE_AS_BASE (ctype); >> + } >> >> tree clobber = build_clobber (ctype); I have noticed we build a distinct as-base type in rather more cases than strictly necessary. For instance when there's a member of reference type or we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch of things that don't affect ABI layout) nathan
> > > - ctype = CLASSTYPE_AS_BASE (ctype); > > > + { > > > + if (!tree_int_cst_equal (TYPE_SIZE (ctype), > > > + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) > > > + ctype = CLASSTYPE_AS_BASE (ctype); > > > + } > > > tree clobber = build_clobber (ctype); > > I have noticed we build a distinct as-base type in rather more cases than > strictly necessary. For instance when there's a member of reference type or > we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch > of things that don't affect ABI layout) Avoiding the extra copies at first place would be great. In my understanding the types differ by virtual bases and also by their size since the fake types are not padded to multiply of their alignment. I guess this can be tested ahead of producing the copy and saving some memory... I am not sure if my C++ FE abilities are on par to implement this tough. Honza > > nathan > > -- > Nathan Sidwell
On 6/19/19 1:53 PM, Jan Hubicka wrote: >>>> - ctype = CLASSTYPE_AS_BASE (ctype); >>>> + { >>>> + if (!tree_int_cst_equal (TYPE_SIZE (ctype), >>>> + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) >>>> + ctype = CLASSTYPE_AS_BASE (ctype); >>>> + } >>>> tree clobber = build_clobber (ctype); >> >> I have noticed we build a distinct as-base type in rather more cases than >> strictly necessary. For instance when there's a member of reference type or >> we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch >> of things that don't affect ABI layout) > > Avoiding the extra copies at first place would be great. In my > understanding the types differ by virtual bases and also by their size > since the fake types are not padded to multiply of their alignment. > I guess this can be tested ahead of producing the copy and saving some > memory... > > I am not sure if my C++ FE abilities are on par to implement this tough. I don't think it's simple to fix there, just unfortunate. your understanding is correct, and I think your workaround will work. However, remember it's possible for T == CLASSTYPE_AS_BASE (T), so might be worth checking that before doing the size comparison? It'd be great to comment on why you're not just using classtype_as_base there. I suppose I'm serializing this stuff too, with the same inefficiencies ... nathan
On Wed, Jun 19, 2019 at 2:47 PM Nathan Sidwell <nathan@acm.org> wrote: > > On 6/19/19 1:53 PM, Jan Hubicka wrote: > >>>> - ctype = CLASSTYPE_AS_BASE (ctype); > >>>> + { > >>>> + if (!tree_int_cst_equal (TYPE_SIZE (ctype), > >>>> + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) > >>>> + ctype = CLASSTYPE_AS_BASE (ctype); > >>>> + } > >>>> tree clobber = build_clobber (ctype); > >> > >> I have noticed we build a distinct as-base type in rather more cases than > >> strictly necessary. For instance when there's a member of reference type or > >> we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch > >> of things that don't affect ABI layout) > > > > Avoiding the extra copies at first place would be great. In my > > understanding the types differ by virtual bases and also by their size > > since the fake types are not padded to multiply of their alignment. > > I guess this can be tested ahead of producing the copy and saving some > > memory... > > > > I am not sure if my C++ FE abilities are on par to implement this tough. > > I don't think it's simple to fix there, just unfortunate. your > understanding is correct, and I think your workaround will work. > However, remember it's possible for T == CLASSTYPE_AS_BASE (T), so might > be worth checking that before doing the size comparison? > > It'd be great to comment on why you're not just using classtype_as_base > there. I suppose I'm serializing this stuff too, with the same > inefficiencies ... This simple (untested) patch doesn't avoid creating the unnecessary as-base types, but it should avoid using them in a way that causes them to be streamed, and should let them be discarded by GC. Thoughts? diff --git a/gcc/cp/class.c b/gcc/cp/class.c index de37e43d04c..e0df9ef2b20 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -6453,6 +6453,12 @@ layout_class_type (tree t, tree *virtuals_p) /* Let the back end lay out the type. */ finish_record_layout (rli, /*free_p=*/true); + /* If we didn't end up needing an as-base type, don't use it. */ + if (CLASSTYPE_AS_BASE (t) != t + && tree_int_cst_equal (TYPE_SIZE (t), + TYPE_SIZE (CLASSTYPE_AS_BASE (t)))) + CLASSTYPE_AS_BASE (t) = t; + if (TYPE_SIZE_UNIT (t) && TREE_CODE (TYPE_SIZE_UNIT (t)) == INTEGER_CST && !TREE_OVERFLOW (TYPE_SIZE_UNIT (t))
On 6/20/19 12:34 AM, Jason Merrill wrote: > On Wed, Jun 19, 2019 at 2:47 PM Nathan Sidwell <nathan@acm.org> wrote: > This simple (untested) patch doesn't avoid creating the unnecessary > as-base types, but it should avoid using them in a way that causes > them to be streamed, and should let them be discarded by GC. > Thoughts? I was thinking something like that might work. nathan
> On 6/19/19 1:53 PM, Jan Hubicka wrote: > > > > > - ctype = CLASSTYPE_AS_BASE (ctype); > > > > > + { > > > > > + if (!tree_int_cst_equal (TYPE_SIZE (ctype), > > > > > + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) > > > > > + ctype = CLASSTYPE_AS_BASE (ctype); > > > > > + } > > > > > tree clobber = build_clobber (ctype); > > > > > > I have noticed we build a distinct as-base type in rather more cases than > > > strictly necessary. For instance when there's a member of reference type or > > > we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch > > > of things that don't affect ABI layout) > > > > Avoiding the extra copies at first place would be great. In my > > understanding the types differ by virtual bases and also by their size > > since the fake types are not padded to multiply of their alignment. > > I guess this can be tested ahead of producing the copy and saving some > > memory... > > > > I am not sure if my C++ FE abilities are on par to implement this tough. > > I don't think it's simple to fix there, just unfortunate. your > understanding is correct, and I think your workaround will work. However, > remember it's possible for T == CLASSTYPE_AS_BASE (T), so might be worth > checking that before doing the size comparison? > > It'd be great to comment on why you're not just using classtype_as_base > there. I suppose I'm serializing this stuff too, with the same > inefficiencies ... Hi, here is updated patch. Bootstrapped/regtested x86_64-linux, OK? It would be still nice to avoid copies at least in the commmon cases - it is easy to generate many types via templates and having basically every type twice is not very nice (we also copy all the fields, so overall memory use can be large). * decl.c (build_clobber_this): Do not use CLASSTYPE_AS_BASE when possible. Index: decl.c =================================================================== --- decl.c (revision 272506) +++ decl.c (working copy) @@ -15229,7 +15229,20 @@ build_clobber_this () tree ctype = current_class_type; if (!vbases) - ctype = CLASSTYPE_AS_BASE (ctype); + { + /* When clobbering base type, we need to be careful to not clobber + extra padding at the end of structure or virtual bases, which are + not considered part of the base by the C++ ABI. + + However try to avoid using CLASSTYPE_AS_BASE when possible because + typically this is the only use in the final intermediate language + where this type is needed. Doing so avoids need to stream many + duplciate type copies to LTO. */ + if (ctype != CLASSTYPE_AS_BASE (ctype) + && !tree_int_cst_equal (TYPE_SIZE (ctype), + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) + ctype = CLASSTYPE_AS_BASE (ctype); + } tree clobber = build_clobber (ctype);
On Thu, 20 Jun 2019, Jason Merrill wrote: > On Wed, Jun 19, 2019 at 2:47 PM Nathan Sidwell <nathan@acm.org> wrote: > > > > On 6/19/19 1:53 PM, Jan Hubicka wrote: > > >>>> - ctype = CLASSTYPE_AS_BASE (ctype); > > >>>> + { > > >>>> + if (!tree_int_cst_equal (TYPE_SIZE (ctype), > > >>>> + TYPE_SIZE (CLASSTYPE_AS_BASE (ctype)))) > > >>>> + ctype = CLASSTYPE_AS_BASE (ctype); > > >>>> + } > > >>>> tree clobber = build_clobber (ctype); > > >> > > >> I have noticed we build a distinct as-base type in rather more cases than > > >> strictly necessary. For instance when there's a member of reference type or > > >> we have a non-trivial dtor. (CLASSTYPE_NON_LAYOUT_POD_P gets set by a bunch > > >> of things that don't affect ABI layout) > > > > > > Avoiding the extra copies at first place would be great. In my > > > understanding the types differ by virtual bases and also by their size > > > since the fake types are not padded to multiply of their alignment. > > > I guess this can be tested ahead of producing the copy and saving some > > > memory... > > > > > > I am not sure if my C++ FE abilities are on par to implement this tough. > > > > I don't think it's simple to fix there, just unfortunate. your > > understanding is correct, and I think your workaround will work. > > However, remember it's possible for T == CLASSTYPE_AS_BASE (T), so might > > be worth checking that before doing the size comparison? > > > > It'd be great to comment on why you're not just using classtype_as_base > > there. I suppose I'm serializing this stuff too, with the same > > inefficiencies ... > > This simple (untested) patch doesn't avoid creating the unnecessary > as-base types, but it should avoid using them in a way that causes > them to be streamed, and should let them be discarded by GC. > Thoughts? Looks better than Honzas patch fixing a single place. I've spent some thoughts on this and I wonder whether we can re-implement classtype-as-base with fake inheritance (which would also solve the TBAA alias set issue in a natural way). That is, we'd lay out structs as-base and make instances of it use a class as-instance { as-base b; X pad1; Y pad2; }; with either explicit padding fields or with implicit ones (I didn't check how we trick stor-layout to not pad the as-base type to its natural alignment...). I realize that this impacts all code building component-refs ontop of as-instance typed objects so this might rule out this approach completely - but maybe that's reasonably well abstracted into common code so only few places need adjustments. Regular derived classes would simply derive from the as-base type (as they do now I guess). Richard.
> > This simple (untested) patch doesn't avoid creating the unnecessary > > as-base types, but it should avoid using them in a way that causes > > them to be streamed, and should let them be discarded by GC. > > Thoughts? > > Looks better than Honzas patch fixing a single place. Indeed, I think in this case it is also possible to drop else if (tree_int_cst_equal (TYPE_SIZE (type), TYPE_SIZE (as_base))) in call.c since they will now be always different. > > I've spent some thoughts on this and I wonder whether we can > re-implement classtype-as-base with fake inheritance (which would > also solve the TBAA alias set issue in a natural way). That is, > we'd lay out structs as-base and make instances of it use a > > class as-instance { as-base b; X pad1; Y pad2; }; > > with either explicit padding fields or with implicit ones > (I didn't check how we trick stor-layout to not pad the as-base > type to its natural alignment...). > > I realize that this impacts all code building component-refs ontop > of as-instance typed objects so this might rule out this approach > completely - but maybe that's reasonably well abstracted into common > code so only few places need adjustments. > > Regular derived classes would simply derive from the as-base type > (as they do now I guess). I was trying to construct testcase using this (i.e. have different access paths one using CLASSTYPE_AS_BASE and other the actual type and failed to do so. Having one would be nice. Honza > > Richard.
On 6/20/19 9:37 AM, Richard Biener wrote: > I've spent some thoughts on this and I wonder whether we can > re-implement classtype-as-base with fake inheritance (which would > also solve the TBAA alias set issue in a natural way). That is, > we'd lay out structs as-base and make instances of it use a > > class as-instance { as-base b; X pad1; Y pad2; }; > > with either explicit padding fields or with implicit ones > (I didn't check how we trick stor-layout to not pad the as-base > type to its natural alignment...). I think you might end up with unordered fields? virtual empty bases don't appear in the as-base variant, and I think they could appear in the middle of the as-instance variant. that might or might not be a problem? nathan
On June 20, 2019 4:06:58 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote: >On 6/20/19 9:37 AM, Richard Biener wrote: > >> I've spent some thoughts on this and I wonder whether we can >> re-implement classtype-as-base with fake inheritance (which would >> also solve the TBAA alias set issue in a natural way). That is, >> we'd lay out structs as-base and make instances of it use a >> >> class as-instance { as-base b; X pad1; Y pad2; }; >> >> with either explicit padding fields or with implicit ones >> (I didn't check how we trick stor-layout to not pad the as-base >> type to its natural alignment...). > >I think you might end up with unordered fields? virtual empty bases >don't appear in the as-base variant, and I think they could appear in >the middle of the as-instance variant. that might or might not be a >problem? Certainly interesting, but if they are empty it might not be an issue if they are never referred to (address taken?) Richard. >nathan
On 6/20/19 11:49 AM, Richard Biener wrote: > On June 20, 2019 4:06:58 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote: >> On 6/20/19 9:37 AM, Richard Biener wrote: >> >>> I've spent some thoughts on this and I wonder whether we can >>> re-implement classtype-as-base with fake inheritance (which would >>> also solve the TBAA alias set issue in a natural way). That is, >>> we'd lay out structs as-base and make instances of it use a >>> >>> class as-instance { as-base b; X pad1; Y pad2; }; >>> >>> with either explicit padding fields or with implicit ones >>> (I didn't check how we trick stor-layout to not pad the as-base >>> type to its natural alignment...). >> >> I think you might end up with unordered fields? virtual empty bases >> don't appear in the as-base variant, and I think they could appear in >> the middle of the as-instance variant. that might or might not be a >> problem? > > Certainly interesting, but if they are empty it might not be an issue if they are never referred to (address taken?) their address could be taken -- the layout rules are such that no two empty objects of the same type can have the same offset. This is important to determine if two references are to the same instance or not. It can't be dereferenced though. and I guess emitting a static initializer wouldn't have anything to put there -- though varasm might barf on non-monotonic offsets regardless. I don't know how common virtual empty bases are in practice. (non-virtual ones would be in the as-base instance, and not have this problem) nathan
> Hi, > here is patch that adds TYPE_ODR_P to determine type that comply C++ > ODR rules (i.e. ODR types themselves or structures/unions derived > from them). > I have decided to use STRING_FLAG which have meaning only for integers > and arrays which forced me to add type checks on places where > we check STRING_FLAG on other types. > > The patch also let me to verify that all types we consider to have > linkage actually are created by C++ FE which turned out to not be the > case for Ada which I fixed in needs_assembler_name_p. > > Bootstrapped/regtested x86_64-linux, OK? > > * ipa-utils.h (type_with_linkage_p): Verify that type is > CXX_ODR_P. > (odr_type_p): Remove extra return. > * lto-streamer-out.c (hash_tree): Hash TYPE_CXX_ODR_P; > hash STRING_FLAG only for arrays and integers. > * tree-stremaer-in.c (unpack_ts_type_common_value_fields): > Update analogously. > * tree-streamer-out.c (pack_ts_type_common_value_fields): > Likewise. > * print-tree.c (print_node): Print cxx-odr-p > and string-flag. > * tree.c (need_assembler_name_p): Also check that type > is CXX_ODR_TYPE_P > (verify_type_variant): Update verification of SRING_FLAG; > also check CXX_ODR_P. > * tree.h (ARRAY_OR_INTEGER_TYPE_CHECK): New macro. > (TYPE_STRING_FLAG): Use it. > (TYPE_CXX_ODR_P): New macro. > > * lto-common.c (compare_tree_sccs_1): Compare CXX_ODR_P; > compare STRING_FLAG only for arrays and integers. > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > type is array or integer prior checking string flag. > * gcc-interface/gigi.h (gnat_signed_type_for, > maybe_character_value): Likewise. > > * c-common.c (braced_lists_to_strings): Check that > type is array or integer prior checking string flag. > > * lex.c (cxx_make_type): Set TYPE_CXX_ODR_P. > > * dwarf2out.c (gen_array_type_die): First check that type > is an array and then test string flag. > > * trans-expr.c (gfc_conv_substring): Check that > type is array or integer prior checking string flag. > (gfc_conv_string_parameter): Likewise. > * trans-openmp.c (gfc_omp_scalar_p): Likewise. > * trans.c (gfc_build_array_ref): Likewise. Hi, I would like to ping the patch - if it makes sense updating the original ODR patch should be easy. Honza
> > This simple (untested) patch doesn't avoid creating the unnecessary > > as-base types, but it should avoid using them in a way that causes > > them to be streamed, and should let them be discarded by GC. > > Thoughts? > > Looks better than Honzas patch fixing a single place. I wonder if we can go ahead with Jason's patch to handle the common case. > > I've spent some thoughts on this and I wonder whether we can > re-implement classtype-as-base with fake inheritance (which would > also solve the TBAA alias set issue in a natural way). That is, > we'd lay out structs as-base and make instances of it use a > > class as-instance { as-base b; X pad1; Y pad2; }; > > with either explicit padding fields or with implicit ones > (I didn't check how we trick stor-layout to not pad the as-base > type to its natural alignment...). > > I realize that this impacts all code building component-refs ontop > of as-instance typed objects so this might rule out this approach > completely - but maybe that's reasonably well abstracted into common > code so only few places need adjustments. Modulo the empty virtual bases which I have no understnading to I suppose this should work. One issue is that we will need to introduce view_convert_exprs at some times. As class a var; class b:a {} *bptr; var.foo; Expanding this as var.as_base_a.foo would make access path oracle to disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with gimple memory moel where we allow placement new replacing var by instance of b. So the way to generate it would be to first view convert expr *bptr to as_base_b and continue from that. This would probably force us to not give up at view converts in the access path disambiguation :) Honza
On Mon, 24 Jun 2019, Jan Hubicka wrote: > > > This simple (untested) patch doesn't avoid creating the unnecessary > > > as-base types, but it should avoid using them in a way that causes > > > them to be streamed, and should let them be discarded by GC. > > > Thoughts? > > > > Looks better than Honzas patch fixing a single place. > > I wonder if we can go ahead with Jason's patch to handle the common > case. I hope so - Jason? > > > > I've spent some thoughts on this and I wonder whether we can > > re-implement classtype-as-base with fake inheritance (which would > > also solve the TBAA alias set issue in a natural way). That is, > > we'd lay out structs as-base and make instances of it use a > > > > class as-instance { as-base b; X pad1; Y pad2; }; > > > > with either explicit padding fields or with implicit ones > > (I didn't check how we trick stor-layout to not pad the as-base > > type to its natural alignment...). > > > > I realize that this impacts all code building component-refs ontop > > of as-instance typed objects so this might rule out this approach > > completely - but maybe that's reasonably well abstracted into common > > code so only few places need adjustments. > > Modulo the empty virtual bases which I have no understnading to I > suppose this should work. > > One issue is that we will need to introduce view_convert_exprs at some > times. > > As > > class a var; > class b:a {} *bptr; > > var.foo; > > Expanding this as var.as_base_a.foo would make access path oracle to > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > gimple memory moel where we allow placement new replacing var by > instance of b. Ick. IIRC the as-base types were necessary only for copying and clobber operations that may not touch the possibly re-used tail-padding. The question is whether we cannot invent a better mechanism to do this -- IIRC we used memmove for the former case at some point (that's arguably worse, also for TBAA). There's WITH_SIZE_EXPR which we only handle rudimentary in the middle-end though. > So the way to generate it would be to first view convert expr *bptr to > as_base_b and continue from that. This would probably force us to not > give up at view converts in the access path disambiguation :) Yeah. No good solution either :/ Richard.
> On Mon, 24 Jun 2019, Jan Hubicka wrote: > > > > > This simple (untested) patch doesn't avoid creating the unnecessary > > > > as-base types, but it should avoid using them in a way that causes > > > > them to be streamed, and should let them be discarded by GC. > > > > Thoughts? > > > > > > Looks better than Honzas patch fixing a single place. > > > > I wonder if we can go ahead with Jason's patch to handle the common > > case. > > I hope so - Jason? > > > > > > > I've spent some thoughts on this and I wonder whether we can > > > re-implement classtype-as-base with fake inheritance (which would > > > also solve the TBAA alias set issue in a natural way). That is, > > > we'd lay out structs as-base and make instances of it use a > > > > > > class as-instance { as-base b; X pad1; Y pad2; }; > > > > > > with either explicit padding fields or with implicit ones > > > (I didn't check how we trick stor-layout to not pad the as-base > > > type to its natural alignment...). > > > > > > I realize that this impacts all code building component-refs ontop > > > of as-instance typed objects so this might rule out this approach > > > completely - but maybe that's reasonably well abstracted into common > > > code so only few places need adjustments. > > > > Modulo the empty virtual bases which I have no understnading to I > > suppose this should work. > > > > One issue is that we will need to introduce view_convert_exprs at some > > times. > > > > As > > > > class a var; > > class b:a {} *bptr; > > > > var.foo; > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > gimple memory moel where we allow placement new replacing var by > > instance of b. > > Ick. IIRC the as-base types were necessary only for > copying and clobber operations that may not touch the possibly > re-used tail-padding. The question is whether we cannot invent > a better mechanism to do this -- IIRC we used memmove for the > former case at some point (that's arguably worse, also for TBAA). > There's WITH_SIZE_EXPR which we only handle rudimentary in the > middle-end though. Yep, only place where as-base type surface to middle-end are clobbers. call.c does: { /* We must only copy the non-tail padding parts. */ tree arg0, arg2, t; tree array_type, alias_set; arg2 = TYPE_SIZE_UNIT (as_base); arg0 = cp_build_addr_expr (to, complain); array_type = build_array_type (unsigned_char_type_node, build_index_type (size_binop (MINUS_EXPR, arg2, size_int (1)))); alias_set = build_int_cst (build_pointer_type (type), 0); t = build2 (MODIFY_EXPR, void_type_node, build2 (MEM_REF, array_type, arg0, alias_set), build2 (MEM_REF, array_type, arg, alias_set)); val = build2 (COMPOUND_EXPR, TREE_TYPE (to), t, to); TREE_NO_WARNING (val) = 1; } I have noticed this earlier too since it disables access path oracle (TREE_TYPE (mem_ref) is char array, while ptrtype is the original type) and thus is pretty bad, too. At least it tests if types are different and therefore this path is relatively rare. We should solve both these places indeed. I think call.c could also use as-base type like clobber codegen if LTO TBAA was fixed on those. I agree that having solution that does not complicate middle-end would be nice, but if we want to get as-base types right the way they are now we need to expose IS_FAKE_BASE_TYPE falg to the middle-end (perhaps with better name). Then we can make canonical_type_used_p return false for those and make get_alias_set and same_type_for_tbaa_p to use TYPE_CONTEXT (type) in this case. This ought to get things work quite smoothly. Since we do not do component refs on those, we should not get into problem with nonoverlapping_component_refs not considering the types the same. Honza
On Mon, 24 Jun 2019, Jan Hubicka wrote: > > Hi, > > here is patch that adds TYPE_ODR_P to determine type that comply C++ > > ODR rules (i.e. ODR types themselves or structures/unions derived > > from them). > > I have decided to use STRING_FLAG which have meaning only for integers > > and arrays which forced me to add type checks on places where > > we check STRING_FLAG on other types. > > > > The patch also let me to verify that all types we consider to have > > linkage actually are created by C++ FE which turned out to not be the > > case for Ada which I fixed in needs_assembler_name_p. > > > > Bootstrapped/regtested x86_64-linux, OK? > > > > * ipa-utils.h (type_with_linkage_p): Verify that type is > > CXX_ODR_P. > > (odr_type_p): Remove extra return. > > * lto-streamer-out.c (hash_tree): Hash TYPE_CXX_ODR_P; > > hash STRING_FLAG only for arrays and integers. > > * tree-stremaer-in.c (unpack_ts_type_common_value_fields): > > Update analogously. > > * tree-streamer-out.c (pack_ts_type_common_value_fields): > > Likewise. > > * print-tree.c (print_node): Print cxx-odr-p > > and string-flag. > > * tree.c (need_assembler_name_p): Also check that type > > is CXX_ODR_TYPE_P > > (verify_type_variant): Update verification of SRING_FLAG; > > also check CXX_ODR_P. > > * tree.h (ARRAY_OR_INTEGER_TYPE_CHECK): New macro. > > (TYPE_STRING_FLAG): Use it. > > (TYPE_CXX_ODR_P): New macro. > > > > * lto-common.c (compare_tree_sccs_1): Compare CXX_ODR_P; > > compare STRING_FLAG only for arrays and integers. > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > > type is array or integer prior checking string flag. > > * gcc-interface/gigi.h (gnat_signed_type_for, > > maybe_character_value): Likewise. > > > > * c-common.c (braced_lists_to_strings): Check that > > type is array or integer prior checking string flag. > > > > * lex.c (cxx_make_type): Set TYPE_CXX_ODR_P. > > > > * dwarf2out.c (gen_array_type_die): First check that type > > is an array and then test string flag. > > > > * trans-expr.c (gfc_conv_substring): Check that > > type is array or integer prior checking string flag. > > (gfc_conv_string_parameter): Likewise. > > * trans-openmp.c (gfc_omp_scalar_p): Likewise. > > * trans.c (gfc_build_array_ref): Likewise. > > Hi, > I would like to ping the patch - if it makes sense updating the original > ODR patch should be easy. Yes, this patch is OK if you amend the string_flag declaration in tree-core.h with a comment explaining the uses on the two different type classes. Btw, I still wonder what the ODR says in the face of language inter-operation and what this means here? For C++ I suppose PODs are not ODR? Thanks, Richard.
On Mon, Jun 24, 2019 at 7:28 AM Richard Biener <rguenther@suse.de> wrote: > On Mon, 24 Jun 2019, Jan Hubicka wrote: > > > > > This simple (untested) patch doesn't avoid creating the unnecessary > > > > as-base types, but it should avoid using them in a way that causes > > > > them to be streamed, and should let them be discarded by GC. > > > > Thoughts? > > > > > > Looks better than Honzas patch fixing a single place. > > > > I wonder if we can go ahead with Jason's patch to handle the common > > case. > > I hope so - Jason? Committed. > > > I've spent some thoughts on this and I wonder whether we can > > > re-implement classtype-as-base with fake inheritance (which would > > > also solve the TBAA alias set issue in a natural way). That is, > > > we'd lay out structs as-base and make instances of it use a > > > > > > class as-instance { as-base b; X pad1; Y pad2; }; > > > > > > with either explicit padding fields or with implicit ones > > > (I didn't check how we trick stor-layout to not pad the as-base > > > type to its natural alignment...). > > > > > > I realize that this impacts all code building component-refs ontop > > > of as-instance typed objects so this might rule out this approach > > > completely - but maybe that's reasonably well abstracted into common > > > code so only few places need adjustments. > > > > Modulo the empty virtual bases which I have no understnading to I > > suppose this should work. > > > > One issue is that we will need to introduce view_convert_exprs at some > > times. > > > > As > > > > class a var; > > class b:a {} *bptr; > > > > var.foo; > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > gimple memory moel where we allow placement new replacing var by > > instance of b. Why do we allow that? I would expect that to only be allowed if a is something like aligned_storage, i.e. a thin wrapper around a char/byte buffer. > Ick. IIRC the as-base types were necessary only for > copying and clobber operations that may not touch the possibly > re-used tail-padding. And temporarily during layout, yes. This is all closely related to PR 22488. > Btw, I still wonder what the ODR says in the face of language > inter-operation and what this means here? For C++ I suppose PODs > are not ODR? The ODR applies to PODs just like other classes. But the ODR says nothing about language interoperation, that's all implementation-defined. Jason
Hi, thanks for comitting the patch! > > > As > > > > > > class a var; > > > class b:a {} *bptr; > > > > > > var.foo; > > > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > > gimple memory moel where we allow placement new replacing var by > > > instance of b. > > Why do we allow that? I would expect that to only be allowed if a is > something like aligned_storage, i.e. a thin wrapper around a char/byte > buffer. I think because Richard defined gimple memory model this way after fair amount of frustration from placement news, stack slot sharing issues and non-conforming codebases :) I think for normal user variables this is overly conservative. At the moment TBAA is bit of a mess. Once it is cleaner up, we could see if restricting this more pays back and then we would need to find way to pass the info to middle-end (as it does not know difference between aligned_storage and other stuff). For dynamically allocated memory as well as for stack space after stack slot sharing done in cfgexpand I see this is necessary since we do not preserve any information about placement new. Note that devirtualization machinery is bit more agressive than TBAA model I am currently aiming for (for example assuming that user variable of given type are not placement new replaced), but I think here we are relatively safe because we do so only for non-POD types where construction/destruction ought to be paired. > > > Ick. IIRC the as-base types were necessary only for > > copying and clobber operations that may not touch the possibly > > re-used tail-padding. > > And temporarily during layout, yes. This is all closely related to PR 22488. I think this is what Richard reffers to the code generating clobber statements that is only leaking as-base types to the middle-end visible part of IL and the code in call.c copying base structures. > > > Btw, I still wonder what the ODR says in the face of language > > inter-operation and what this means here? For C++ I suppose PODs > > are not ODR? > > The ODR applies to PODs just like other classes. But the ODR says > nothing about language interoperation, that's all > implementation-defined. My patchset considers all C++ types inter-operating with non-C++ types. So first we load all types and do the following: During streaming I populate ODR type hash with ODR types and canonical type hash with types not originating from C++. Once all types are in memory I do following: 1) For every structure/union with linkage - see if there is structurally equivalent non-C++ type in canonical type hash (where structural equivalence is defined in very generous way ignoring pointer types, type tags and field names so interoperability with fortran is safe) if there is no mathing type and no detected ODR violation declare mark the type to be hanled by ODR name in step 2 2) for every structure/union originating from C++ compute the canonical type by canonical type hash query. If in 1) we decided that given ODR type is unique the cnaonical type hash compare type by name rather than by structure. I do not handle enums since those conflicts with integer that is declared in every translation unit. So at this time basically every C++ type can inter-operate with non-C++. I was thinking of relaxing this somewhat but wanted to see if C++ standard says something here. Things that may be sensible include: 1) perhaps non-POD types especially those with vptr pointers do not need to be inter-operable. 2) anonymous namespace types 3) types in namespace Honza > > Jason
On Mon, Jun 24, 2019 at 11:57 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > Hi, > thanks for comitting the patch! > > > > As > > > > > > > > class a var; > > > > class b:a {} *bptr; > > > > > > > > var.foo; > > > > > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > > > gimple memory moel where we allow placement new replacing var by > > > > instance of b. > > > > Why do we allow that? I would expect that to only be allowed if a is > > something like aligned_storage, i.e. a thin wrapper around a char/byte > > buffer. > > I think because Richard defined gimple memory model this way after fair > amount of frustration from placement news, stack slot sharing issues > and non-conforming codebases :) > > I think for normal user variables this is overly conservative. > At the moment TBAA is bit of a mess. Once it is cleaned up, we could > see if restricting this more pays back and then we would need to > find way to pass the info to middle-end (as it does not > know difference between aligned_storage and other stuff). I thought I remembered someone's recent-ish work to treat specially types containing a char array, but I'm not finding it now. > For dynamically allocated memory as well as for stack space after stack > slot sharing done in cfgexpand I see this is necessary since we do not > preserve any information about placement new. Yes, untyped memory is different, but I'd think that memory allocated through (non-placement) new should also be considered typed. > Note that devirtualization machinery is bit more agressive than TBAA > model I am currently aiming for (for example assuming that user variable > of given type are not placement new replaced), but I think here we are > relatively safe because we do so only for non-POD types where > construction/destruction ought to be paired. Agreed. > > > Ick. IIRC the as-base types were necessary only for > > > copying and clobber operations that may not touch the possibly > > > re-used tail-padding. > > > > And temporarily during layout, yes. This is all closely related to PR 22488. > > I think this is what Richard refers to the code generating clobber > statements that is only leaking as-base types to the middle-end visible > part of IL and the code in call.c copying base structures. Right. Is there a better way we could express copying/clobbering only part of the object without involving the as-base types? > > > Btw, I still wonder what the ODR says in the face of language > > > inter-operation and what this means here? For C++ I suppose PODs > > > are not ODR? > > > > The ODR applies to PODs just like other classes. But the ODR says > > nothing about language interoperation, that's all > > implementation-defined. > > My patchset considers all C++ types inter-operating with non-C++ types. > So first we load all types and do the following: > > During streaming I populate ODR type hash with ODR types and canonical > type hash with types not originating from C++. > > Once all types are in memory I do following: > 1) For every structure/union with linkage > - see if there is structurally equivalent non-C++ type in canonical > type hash (where structural equivalence is defined in very > generous way ignoring pointer types, type tags and field names so > interoperability with fortran is safe) > > if there is no matching type and no detected ODR violation declare > mark the type to be handled by ODR name in step 2 > 2) for every structure/union originating from C++ compute the canonical > type by canonical type hash query. If in 1) we decided that given > ODR type is unique the canonical type hash compare type by name > rather than by structure. > I do not handle enums since those conflicts with integer that is > declared in every translation unit. > > So at this time basically every C++ type can inter-operate with non-C++. > I was thinking of relaxing this somewhat but wanted to see if C++ > standard says something here. Things that may be sensible include: > 1) perhaps non-POD types especially those with vptr pointers do > not need to be inter-operable. PODs were intended to be the C-compatible subset, yes. > 2) anonymous namespace types > 3) types in namespace As long as these types don't have explicit language linkage (e.g. extern "C"), sure. Jason
On Mon, Jun 24, 2019 at 12:40 PM Jason Merrill <jason@redhat.com> wrote: > On Mon, Jun 24, 2019 at 11:57 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > As > > > > > > > > > > class a var; > > > > > class b:a {} *bptr; > > > > > > > > > > var.foo; > > > > > > > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > > > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > > > > gimple memory moel where we allow placement new replacing var by > > > > > instance of b. > > > > > > Why do we allow that? I would expect that to only be allowed if a is > > > something like aligned_storage, i.e. a thin wrapper around a char/byte > > > buffer. > > > > I think because Richard defined gimple memory model this way after fair > > amount of frustration from placement news, stack slot sharing issues > > and non-conforming codebases :) > > > > I think for normal user variables this is overly conservative. > > At the moment TBAA is bit of a mess. Once it is cleaned up, we could > > see if restricting this more pays back and then we would need to > > find way to pass the info to middle-end (as it does not > > know difference between aligned_storage and other stuff). > > I thought I remembered someone's recent-ish work to treat specially > types containing a char array, but I'm not finding it now. Specifically, aligned_storage has a first member which is a char array, and a char array can alias anything, so we can put anything in a char array, and a class can alias its first member, so transitively we can put anything in such a class. Jason
On Mon, 24 Jun 2019, Jason Merrill wrote: > On Mon, Jun 24, 2019 at 12:40 PM Jason Merrill <jason@redhat.com> wrote: > > On Mon, Jun 24, 2019 at 11:57 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > > > As > > > > > > > > > > > > class a var; > > > > > > class b:a {} *bptr; > > > > > > > > > > > > var.foo; > > > > > > > > > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > > > > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > > > > > gimple memory moel where we allow placement new replacing var by > > > > > > instance of b. > > > > > > > > Why do we allow that? I would expect that to only be allowed if a is > > > > something like aligned_storage, i.e. a thin wrapper around a char/byte > > > > buffer. > > > > > > I think because Richard defined gimple memory model this way after fair > > > amount of frustration from placement news, stack slot sharing issues > > > and non-conforming codebases :) > > > > > > I think for normal user variables this is overly conservative. > > > At the moment TBAA is bit of a mess. Once it is cleaned up, we could > > > see if restricting this more pays back and then we would need to > > > find way to pass the info to middle-end (as it does not > > > know difference between aligned_storage and other stuff). > > > > I thought I remembered someone's recent-ish work to treat specially > > types containing a char array, but I'm not finding it now. > > Specifically, aligned_storage has a first member which is a char > array, and a char array can alias anything, so we can put anything in > a char array, and a class can alias its first member, so transitively > we can put anything in such a class. You refer to TYPE_TYPELESS_STORAGE I guess. Richard.
On Mon, Jun 24, 2019 at 1:23 PM Richard Biener <rguenther@suse.de> wrote: > On Mon, 24 Jun 2019, Jason Merrill wrote: > > > On Mon, Jun 24, 2019 at 12:40 PM Jason Merrill <jason@redhat.com> wrote: > > > On Mon, Jun 24, 2019 at 11:57 AM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > > > > > As > > > > > > > > > > > > > > class a var; > > > > > > > class b:a {} *bptr; > > > > > > > > > > > > > > var.foo; > > > > > > > > > > > > > > Expanding this as var.as_base_a.foo would make access path oracle to > > > > > > > disambiguate it from bptr->as_base_b->as_base_a.foo which is wrong with > > > > > > > gimple memory moel where we allow placement new replacing var by > > > > > > > instance of b. > > > > > > > > > > Why do we allow that? I would expect that to only be allowed if a is > > > > > something like aligned_storage, i.e. a thin wrapper around a char/byte > > > > > buffer. > > > > > > > > I think because Richard defined gimple memory model this way after fair > > > > amount of frustration from placement news, stack slot sharing issues > > > > and non-conforming codebases :) > > > > > > > > I think for normal user variables this is overly conservative. > > > > At the moment TBAA is bit of a mess. Once it is cleaned up, we could > > > > see if restricting this more pays back and then we would need to > > > > find way to pass the info to middle-end (as it does not > > > > know difference between aligned_storage and other stuff). > > > > > > I thought I remembered someone's recent-ish work to treat specially > > > types containing a char array, but I'm not finding it now. > > > > Specifically, aligned_storage has a first member which is a char > > array, and a char array can alias anything, so we can put anything in > > a char array, and a class can alias its first member, so transitively > > we can put anything in such a class. > > You refer to TYPE_TYPELESS_STORAGE I guess. Yes, thanks. Jason
Hi, > > I thought I remembered someone's recent-ish work to treat specially > types containing a char array, but I'm not finding it now. > > > For dynamically allocated memory as well as for stack space after stack > > slot sharing done in cfgexpand I see this is necessary since we do not > > preserve any information about placement new. > > Yes, untyped memory is different, but I'd think that memory allocated > through (non-placement) new should also be considered typed. I will try to return to this once the code is cleaned up. It would be quite interesting to make this better defined. > > > > I think this is what Richard refers to the code generating clobber > > statements that is only leaking as-base types to the middle-end visible > > part of IL and the code in call.c copying base structures. > > Right. Is there a better way we could express copying/clobbering only > part of the object without involving the as-base types? I think currently two variants was discussed 1) use the trik Richard proposed for class a containing fake subclass a_as_base that has reduced size (i.e. is the AS_BASE type we have now) and adjust all component_refs accordingly introducing the view_convert_exprs for outer decls. Then clobbers and copies in call.c could use the as_base type. 2) expose IS_FAKE_BASE_TYPE to middle-end and teach TBAA machinery about the fact that these are actually same types for everything it cares about. We have similar hacks for Fortran commons already, but of couse it is not most pretty (I outlined plan in previous mail) I would personally lean towards 2 since it will keep all component_refs in its current natural form and because I know how to implement it :) I guess it is Richi and yours call to pick variant which is better... > > > So at this time basically every C++ type can inter-operate with non-C++. > > I was thinking of relaxing this somewhat but wanted to see if C++ > > standard says something here. Things that may be sensible include: > > 1) perhaps non-POD types especially those with vptr pointers do > > not need to be inter-operable. > > PODs were intended to be the C-compatible subset, yes. > > > 2) anonymous namespace types > > 3) types in namespace > > As long as these types don't have explicit language linkage (e.g. > extern "C"), sure. Great, I will add those to my TODOs. Do we have any way to tell language linkage from middle-end? Honza > > Jason
> * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > type is array or integer prior checking string flag. The test for array is superfluous here. > * gcc-interface/gigi.h (gnat_signed_type_for, > maybe_character_value): Likewise. Wrong ChangeLog, the first modified function is maybe_character_type. I have installed the attached patchlet after testing it on x86-64/Linux. * gcc-interface/decl.c (gnat_to_gnu_entity): Remove superfluous test in previous change. * gcc-interface/gigi.h (maybe_character_type): Fix formatting. (maybe_character_value): Likewise.
> > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > > type is array or integer prior checking string flag. > > The test for array is superfluous here. > > > * gcc-interface/gigi.h (gnat_signed_type_for, > > maybe_character_value): Likewise. > > Wrong ChangeLog, the first modified function is maybe_character_type. > > I have installed the attached patchlet after testing it on x86-64/Linux. > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Remove superfluous test in > previous change. > * gcc-interface/gigi.h (maybe_character_type): Fix formatting. > (maybe_character_value): Likewise. Thanks a lot. I was not quite sure if ARRAY_TYPEs can happen there and I should have added you to the CC. Honza > > -- > Eric Botcazou > Index: gcc-interface/decl.c > =================================================================== > --- gcc-interface/decl.c (revision 272633) > +++ gcc-interface/decl.c (working copy) > @@ -1855,8 +1855,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit > = Has_Biased_Representation (gnat_entity); > > /* Do the same processing for Character subtypes as for types. */ > - if ((TREE_CODE (TREE_TYPE (gnu_type)) == INTEGER_TYPE > - || TREE_CODE (TREE_TYPE (gnu_type)) == ARRAY_TYPE) > + if (TREE_CODE (TREE_TYPE (gnu_type)) == INTEGER_TYPE > && TYPE_STRING_FLAG (TREE_TYPE (gnu_type))) > { > TYPE_NAME (gnu_type) = gnu_entity_name; > Index: gcc-interface/gigi.h > =================================================================== > --- gcc-interface/gigi.h (revision 272633) > +++ gcc-interface/gigi.h (working copy) > @@ -1139,7 +1139,8 @@ static inline tree > maybe_character_type (tree type) > { > if (TREE_CODE (type) == INTEGER_TYPE > - && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type)) > + && TYPE_STRING_FLAG (type) > + && !TYPE_UNSIGNED (type)) > type = gnat_unsigned_type_for (type); > > return type; > @@ -1153,7 +1154,8 @@ maybe_character_value (tree expr) > tree type = TREE_TYPE (expr); > > if (TREE_CODE (type) == INTEGER_TYPE > - && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type)) > + && TYPE_STRING_FLAG (type) > + && !TYPE_UNSIGNED (type)) > { > type = gnat_unsigned_type_for (type); > expr = convert (type, expr);
Hi, On Tue, 25 Jun 2019 at 10:20, Jan Hubicka <hubicka@ucw.cz> wrote: > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > > > type is array or integer prior checking string flag. > > > > The test for array is superfluous here. > > > > > * gcc-interface/gigi.h (gnat_signed_type_for, > > > maybe_character_value): Likewise. > > > > Wrong ChangeLog, the first modified function is maybe_character_type. > > > > I have installed the attached patchlet after testing it on x86-64/Linux. > > > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Remove superfluous test in > > previous change. > > * gcc-interface/gigi.h (maybe_character_type): Fix formatting. > > (maybe_character_value): Likewise. > > Thanks a lot. I was not quite sure if ARRAY_TYPEs can happen there > and I should have added you to the CC. > After the main commit (r272628), I have noticed regressions on arm and aarch64: g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto -flto-partition=1to1 -fno-use-linker-plugin (internal compiler error) g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto -flto-partition=none -fuse-linker-plugin (internal compiler error) g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto -flto-partition=1to1 -fno-use-linker-plugin (internal compiler error) g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto -flto-partition=none -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto -fuse-linker-plugin (internal compiler error) g++.dg/torture/pr45843.C -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) g++.dg/torture/pr45843.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler error) g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler error) g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler error) g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler error) A sample ICE: lto1: error: type variant differs by TYPE_CXX_ODR_P <record_type 0x2b3d78275dc8 __va_list BLK size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8 bitsizetype> constant 256> unit-size <integer_cst 0x2b3d78270060 type <integer_type 0x2b3d78269000 sizetype> constant 32> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x2b3d78275dc8 fields <field_decl 0x2b3d78273130 __stack type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18 void> public unsigned DI size <integer_cst 0x2b3d7825cc00 constant 64> unit-size <integer_cst 0x2b3d7825cc18 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality pointer_to_this <pointer_type 0x2b3d782769d8>> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x2b3d7825cc30 constant 0> bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type 0x2b3d78271000> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl 0x2b3d78273260 __vr_top>>> reference_to_this <reference_type 0x2b3d782760a8> chain <type_decl 0x2b3d78273098 __va_list>> <record_type 0x2b3d783f6930 va_list cxx-odr-p BLK size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8 bitsizetype> constant 256> unit-size <integer_cst 0x2b3d78270060 type <integer_type 0x2b3d78269000 sizetype> constant 32> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x2b3d78275dc8 fields <field_decl 0x2b3d78273130 __stack type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18 void> public unsigned DI size <integer_cst 0x2b3d7825cc00 constant 64> unit-size <integer_cst 0x2b3d7825cc18 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality pointer_to_this <pointer_type 0x2b3d782769d8>> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x2b3d7825cc30 constant 0> bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type 0x2b3d78271000> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl 0x2b3d78273260 __vr_top>>> pointer_to_this <pointer_type 0x2b3d783f69d8>> lto1: internal compiler error: 'verify_type' failed 0xe667b0 verify_type(tree_node const*) /gcc/tree.c:14650 0x632cd7 lto_fixup_state /gcc/lto/lto-common.c:2429 0x63f459 lto_fixup_decls /gcc/lto/lto-common.c:2460 0x63f459 read_cgraph_and_symbols(unsigned int, char const**) /gcc/lto/lto-common.c:2693 0x620fa2 lto_main() /gcc/lto/lto.c:616 Please submit a full bug report, Christophe > Honza > > > > -- > > Eric Botcazou > > > Index: gcc-interface/decl.c > > =================================================================== > > --- gcc-interface/decl.c (revision 272633) > > +++ gcc-interface/decl.c (working copy) > > @@ -1855,8 +1855,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit > > = Has_Biased_Representation (gnat_entity); > > > > /* Do the same processing for Character subtypes as for types. */ > > - if ((TREE_CODE (TREE_TYPE (gnu_type)) == INTEGER_TYPE > > - || TREE_CODE (TREE_TYPE (gnu_type)) == ARRAY_TYPE) > > + if (TREE_CODE (TREE_TYPE (gnu_type)) == INTEGER_TYPE > > && TYPE_STRING_FLAG (TREE_TYPE (gnu_type))) > > { > > TYPE_NAME (gnu_type) = gnu_entity_name; > > Index: gcc-interface/gigi.h > > =================================================================== > > --- gcc-interface/gigi.h (revision 272633) > > +++ gcc-interface/gigi.h (working copy) > > @@ -1139,7 +1139,8 @@ static inline tree > > maybe_character_type (tree type) > > { > > if (TREE_CODE (type) == INTEGER_TYPE > > - && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type)) > > + && TYPE_STRING_FLAG (type) > > + && !TYPE_UNSIGNED (type)) > > type = gnat_unsigned_type_for (type); > > > > return type; > > @@ -1153,7 +1154,8 @@ maybe_character_value (tree expr) > > tree type = TREE_TYPE (expr); > > > > if (TREE_CODE (type) == INTEGER_TYPE > > - && TYPE_STRING_FLAG (type) && !TYPE_UNSIGNED (type)) > > + && TYPE_STRING_FLAG (type) > > + && !TYPE_UNSIGNED (type)) > > { > > type = gnat_unsigned_type_for (type); > > expr = convert (type, expr); >
Hi,
This commit https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=272628 is breaking trunk LTO on some real benchmarks, so can it be fixed or reverted? For example,
lto1: error: type variant differs by TYPE_CXX_ODR_P
<record_type 0xffff99943d08 __va_list BLK
size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 bitsizetype> constant 256>
unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 sizetype> constant 32>
align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0xffff99943d08
fields <field_decl 0xffff99960130 __stack
type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void>
public unsigned DI
size <integer_cst 0xffff99890c00 constant 64>
unit-size <integer_cst 0xffff99890c18 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality
pointer_to_this <pointer_type 0xffff99b5d740>>
unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8>
align:64 warn_if_not_align:0 offset_align 128
offset <integer_cst 0xffff99890c30 constant 0>
bit-offset <integer_cst 0xffff99890c78 constant 0> context <record_type 0xffff99943d08 __va_list>
chain <field_decl 0xffff999601c8 __gr_top type <pointer_type 0xffff99940fc0>
unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8>
align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 __vr_top>>>
reference_to_this <reference_type 0xffff99943fa8> chain <type_decl 0xffff99960098 __va_list>>
<record_type 0xffff99c59cd8 va_list cxx-odr-p BLK
size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 bitsizetype> constant 256>
unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 sizetype> constant 32>
align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0xffff99943d08
fields <field_decl 0xffff99960130 __stack
type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void>
public unsigned DI
size <integer_cst 0xffff99890c00 constant 64>
unit-size <integer_cst 0xffff99890c18 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality
pointer_to_this <pointer_type 0xffff99b5d740>>
unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8>
align:64 warn_if_not_align:0 offset_align 128
offset <integer_cst 0xffff99890c30 constant 0>
bit-offset <integer_cst 0xffff99890c78 constant 0> context <record_type 0xffff99943d08 __va_list>
chain <field_decl 0xffff999601c8 __gr_top type <pointer_type 0xffff99940fc0>
unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8>
align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 __vr_top>>>
pointer_to_this <pointer_type 0xffff99c59e28> reference_to_this <reference_type 0xffff9a5b0d20>>
lto1: internal compiler error: 'verify_type' failed
0xe33e93 verify_type(tree_node const*)
../../gcc/gcc/tree.c:14655
0x5efc4b lto_fixup_state
../../gcc/gcc/lto/lto-common.c:2429
0x5fc01b lto_fixup_decls
../../gcc/gcc/lto/lto-common.c:2460
0x5fc01b read_cgraph_and_symbols(unsigned int, char const**)
../../gcc/gcc/lto/lto-common.c:2693
0x5ded23 lto_main()
../../gcc/gcc/lto/lto.c:616
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
lto-wrapper: fatal error: /home/amptest/gcc/install_last//bin/g++ returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
Thanks,
-Jiangning
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org>
> On Behalf Of Christophe Lyon
> Sent: Tuesday, June 25, 2019 8:30 PM
> To: Jan Hubicka <hubicka@ucw.cz>
> Cc: Eric Botcazou <ebotcazou@adacore.com>; gcc Patches <gcc-
> patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>; d@dcepelik.cz;
> Martin Liška <mliska@suse.cz>
> Subject: Re: Use ODR for canonical types construction in LTO
>
> Hi,
>
>
> On Tue, 25 Jun 2019 at 10:20, Jan Hubicka <hubicka@ucw.cz> wrote:
> >
> > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that
> > > > type is array or integer prior checking string flag.
> > >
> > > The test for array is superfluous here.
> > >
> > > > * gcc-interface/gigi.h (gnat_signed_type_for,
> > > > maybe_character_value): Likewise.
> > >
> > > Wrong ChangeLog, the first modified function is maybe_character_type.
> > >
> > > I have installed the attached patchlet after testing it on x86-64/Linux.
> > >
> > >
> > > * gcc-interface/decl.c (gnat_to_gnu_entity): Remove superfluous test
> in
> > > previous change.
> > > * gcc-interface/gigi.h (maybe_character_type): Fix formatting.
> > > (maybe_character_value): Likewise.
> >
> > Thanks a lot. I was not quite sure if ARRAY_TYPEs can happen there and
> > I should have added you to the CC.
> >
>
> After the main commit (r272628), I have noticed regressions on arm and
> aarch64:
>
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto
> -flto-partition=1to1 -fno-use-linker-plugin (internal compiler
> error)
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto
> -flto-partition=none -fuse-linker-plugin (internal compiler
> error)
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto
> -fuse-linker-plugin -fno-fat-lto-objects (internal compiler
> error)
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto
> -flto-partition=1to1 -fno-use-linker-plugin (internal compiler
> error)
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto
> -flto-partition=none -fuse-linker-plugin -fno-fat-lto-objects (internal compiler
> error)
> g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto
> -fuse-linker-plugin (internal compiler error)
> g++.dg/torture/pr45843.C -O2 -flto -fno-use-linker-plugin
> -flto-partition=none (internal compiler error)
> g++.dg/torture/pr45843.C -O2 -flto -fuse-linker-plugin
> -fno-fat-lto-objects (internal compiler error)
> g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto
> -fno-use-linker-plugin -flto-partition=none (internal compiler error)
> g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto
> -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler
> error)
> g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto
> -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error)
> g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto
> -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler
> error)
> g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto
> -fno-use-linker-plugin -flto-partition=none (internal compiler error)
> g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto
> -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler
> error)
> g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto
> -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error)
> g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto
> -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler
> error)
>
> A sample ICE:
> lto1: error: type variant differs by TYPE_CXX_ODR_P <record_type
> 0x2b3d78275dc8 __va_list BLK
> size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8
> bitsizetype> constant 256>
> unit-size <integer_cst 0x2b3d78270060 type <integer_type
> 0x2b3d78269000 sizetype> constant 32>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x2b3d78275dc8
> fields <field_decl 0x2b3d78273130 __stack
> type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18
> void>
> public unsigned DI
> size <integer_cst 0x2b3d7825cc00 constant 64>
> unit-size <integer_cst 0x2b3d7825cc18 constant 8>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality
> pointer_to_this <pointer_type 0x2b3d782769d8>>
> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00
> 64> unit-size <integer_cst 0x2b3d7825cc18 8>
> align:64 warn_if_not_align:0 offset_align 128
> offset <integer_cst 0x2b3d7825cc30 constant 0>
> bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context
> <record_type 0x2b3d78275dc8 __va_list>
> chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type
> 0x2b3d78271000>
> unsigned DI <built-in>:0:0 size <integer_cst
> 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8>
> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst
> 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00
> 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl
> 0x2b3d78273260 __vr_top>>>
> reference_to_this <reference_type 0x2b3d782760a8> chain <type_decl
> 0x2b3d78273098 __va_list>>
> <record_type 0x2b3d783f6930 va_list cxx-odr-p BLK
> size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8
> bitsizetype> constant 256>
> unit-size <integer_cst 0x2b3d78270060 type <integer_type
> 0x2b3d78269000 sizetype> constant 32>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
> 0x2b3d78275dc8
> fields <field_decl 0x2b3d78273130 __stack
> type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18
> void>
> public unsigned DI
> size <integer_cst 0x2b3d7825cc00 constant 64>
> unit-size <integer_cst 0x2b3d7825cc18 constant 8>
> align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality
> pointer_to_this <pointer_type 0x2b3d782769d8>>
> unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00
> 64> unit-size <integer_cst 0x2b3d7825cc18 8>
> align:64 warn_if_not_align:0 offset_align 128
> offset <integer_cst 0x2b3d7825cc30 constant 0>
> bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context
> <record_type 0x2b3d78275dc8 __va_list>
> chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type
> 0x2b3d78271000>
> unsigned DI <built-in>:0:0 size <integer_cst
> 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8>
> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst
> 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00
> 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl
> 0x2b3d78273260 __vr_top>>>
> pointer_to_this <pointer_type 0x2b3d783f69d8>>
> lto1: internal compiler error: 'verify_type' failed
> 0xe667b0 verify_type(tree_node const*)
> /gcc/tree.c:14650
> 0x632cd7 lto_fixup_state
> /gcc/lto/lto-common.c:2429
> 0x63f459 lto_fixup_decls
> /gcc/lto/lto-common.c:2460
> 0x63f459 read_cgraph_and_symbols(unsigned int, char const**)
> /gcc/lto/lto-common.c:2693
> 0x620fa2 lto_main()
> /gcc/lto/lto.c:616
> Please submit a full bug report,
>
> Christophe
> Hi, > > This commit https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=272628 is breaking trunk LTO on some real benchmarks, so can it be fixed or reverted? For example, Do you have a testcase? Honza > > lto1: error: type variant differs by TYPE_CXX_ODR_P > <record_type 0xffff99943d08 __va_list BLK > size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 bitsizetype> constant 256> > unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 sizetype> constant 32> > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0xffff99943d08 > fields <field_decl 0xffff99960130 __stack > type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void> > public unsigned DI > size <integer_cst 0xffff99890c00 constant 64> > unit-size <integer_cst 0xffff99890c18 constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality > pointer_to_this <pointer_type 0xffff99b5d740>> > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8> > align:64 warn_if_not_align:0 offset_align 128 > offset <integer_cst 0xffff99890c30 constant 0> > bit-offset <integer_cst 0xffff99890c78 constant 0> context <record_type 0xffff99943d08 __va_list> > chain <field_decl 0xffff999601c8 __gr_top type <pointer_type 0xffff99940fc0> > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8> > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 __vr_top>>> > reference_to_this <reference_type 0xffff99943fa8> chain <type_decl 0xffff99960098 __va_list>> > <record_type 0xffff99c59cd8 va_list cxx-odr-p BLK > size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 bitsizetype> constant 256> > unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 sizetype> constant 32> > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0xffff99943d08 > fields <field_decl 0xffff99960130 __stack > type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void> > public unsigned DI > size <integer_cst 0xffff99890c00 constant 64> > unit-size <integer_cst 0xffff99890c18 constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality > pointer_to_this <pointer_type 0xffff99b5d740>> > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8> > align:64 warn_if_not_align:0 offset_align 128 > offset <integer_cst 0xffff99890c30 constant 0> > bit-offset <integer_cst 0xffff99890c78 constant 0> context <record_type 0xffff99943d08 __va_list> > chain <field_decl 0xffff999601c8 __gr_top type <pointer_type 0xffff99940fc0> > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size <integer_cst 0xffff99890c18 8> > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 __vr_top>>> > pointer_to_this <pointer_type 0xffff99c59e28> reference_to_this <reference_type 0xffff9a5b0d20>> > lto1: internal compiler error: 'verify_type' failed > 0xe33e93 verify_type(tree_node const*) > ../../gcc/gcc/tree.c:14655 > 0x5efc4b lto_fixup_state > ../../gcc/gcc/lto/lto-common.c:2429 > 0x5fc01b lto_fixup_decls > ../../gcc/gcc/lto/lto-common.c:2460 > 0x5fc01b read_cgraph_and_symbols(unsigned int, char const**) > ../../gcc/gcc/lto/lto-common.c:2693 > 0x5ded23 lto_main() > ../../gcc/gcc/lto/lto.c:616 > Please submit a full bug report, > with preprocessed source if appropriate. > Please include the complete backtrace with any bug report. > See <https://gcc.gnu.org/bugs/> for instructions. > lto-wrapper: fatal error: /home/amptest/gcc/install_last//bin/g++ returned 1 exit status > compilation terminated. > /usr/bin/ld: error: lto-wrapper failed > collect2: error: ld returned 1 exit status > > Thanks, > -Jiangning > > > -----Original Message----- > > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> > > On Behalf Of Christophe Lyon > > Sent: Tuesday, June 25, 2019 8:30 PM > > To: Jan Hubicka <hubicka@ucw.cz> > > Cc: Eric Botcazou <ebotcazou@adacore.com>; gcc Patches <gcc- > > patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>; d@dcepelik.cz; > > Martin Liška <mliska@suse.cz> > > Subject: Re: Use ODR for canonical types construction in LTO > > > > Hi, > > > > > > On Tue, 25 Jun 2019 at 10:20, Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > > > > > type is array or integer prior checking string flag. > > > > > > > > The test for array is superfluous here. > > > > > > > > > * gcc-interface/gigi.h (gnat_signed_type_for, > > > > > maybe_character_value): Likewise. > > > > > > > > Wrong ChangeLog, the first modified function is maybe_character_type. > > > > > > > > I have installed the attached patchlet after testing it on x86-64/Linux. > > > > > > > > > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Remove superfluous test > > in > > > > previous change. > > > > * gcc-interface/gigi.h (maybe_character_type): Fix formatting. > > > > (maybe_character_value): Likewise. > > > > > > Thanks a lot. I was not quite sure if ARRAY_TYPEs can happen there and > > > I should have added you to the CC. > > > > > > > After the main commit (r272628), I have noticed regressions on arm and > > aarch64: > > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto > > -flto-partition=1to1 -fno-use-linker-plugin (internal compiler > > error) > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto > > -flto-partition=none -fuse-linker-plugin (internal compiler > > error) > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O0 -flto > > -fuse-linker-plugin -fno-fat-lto-objects (internal compiler > > error) > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto > > -flto-partition=1to1 -fno-use-linker-plugin (internal compiler > > error) > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto > > -flto-partition=none -fuse-linker-plugin -fno-fat-lto-objects (internal compiler > > error) > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, -O2 -flto > > -fuse-linker-plugin (internal compiler error) > > g++.dg/torture/pr45843.C -O2 -flto -fno-use-linker-plugin > > -flto-partition=none (internal compiler error) > > g++.dg/torture/pr45843.C -O2 -flto -fuse-linker-plugin > > -fno-fat-lto-objects (internal compiler error) > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > -fno-use-linker-plugin -flto-partition=none (internal compiler error) > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler > > error) > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler > > error) > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > -fno-use-linker-plugin -flto-partition=none (internal compiler error) > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler > > error) > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler > > error) > > > > A sample ICE: > > lto1: error: type variant differs by TYPE_CXX_ODR_P <record_type > > 0x2b3d78275dc8 __va_list BLK > > size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8 > > bitsizetype> constant 256> > > unit-size <integer_cst 0x2b3d78270060 type <integer_type > > 0x2b3d78269000 sizetype> constant 32> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > > 0x2b3d78275dc8 > > fields <field_decl 0x2b3d78273130 __stack > > type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18 > > void> > > public unsigned DI > > size <integer_cst 0x2b3d7825cc00 constant 64> > > unit-size <integer_cst 0x2b3d7825cc18 constant 8> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality > > pointer_to_this <pointer_type 0x2b3d782769d8>> > > unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 > > 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > align:64 warn_if_not_align:0 offset_align 128 > > offset <integer_cst 0x2b3d7825cc30 constant 0> > > bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context > > <record_type 0x2b3d78275dc8 __va_list> > > chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type > > 0x2b3d78271000> > > unsigned DI <built-in>:0:0 size <integer_cst > > 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst > > 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 > > 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl > > 0x2b3d78273260 __vr_top>>> > > reference_to_this <reference_type 0x2b3d782760a8> chain <type_decl > > 0x2b3d78273098 __va_list>> > > <record_type 0x2b3d783f6930 va_list cxx-odr-p BLK > > size <integer_cst 0x2b3d7825cf60 type <integer_type 0x2b3d782690a8 > > bitsizetype> constant 256> > > unit-size <integer_cst 0x2b3d78270060 type <integer_type > > 0x2b3d78269000 sizetype> constant 32> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > > 0x2b3d78275dc8 > > fields <field_decl 0x2b3d78273130 __stack > > type <pointer_type 0x2b3d78271000 type <void_type 0x2b3d78269f18 > > void> > > public unsigned DI > > size <integer_cst 0x2b3d7825cc00 constant 64> > > unit-size <integer_cst 0x2b3d7825cc18 constant 8> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural-equality > > pointer_to_this <pointer_type 0x2b3d782769d8>> > > unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 > > 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > align:64 warn_if_not_align:0 offset_align 128 > > offset <integer_cst 0x2b3d7825cc30 constant 0> > > bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context > > <record_type 0x2b3d78275dc8 __va_list> > > chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type > > 0x2b3d78271000> > > unsigned DI <built-in>:0:0 size <integer_cst > > 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst > > 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 > > 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl > > 0x2b3d78273260 __vr_top>>> > > pointer_to_this <pointer_type 0x2b3d783f69d8>> > > lto1: internal compiler error: 'verify_type' failed > > 0xe667b0 verify_type(tree_node const*) > > /gcc/tree.c:14650 > > 0x632cd7 lto_fixup_state > > /gcc/lto/lto-common.c:2429 > > 0x63f459 lto_fixup_decls > > /gcc/lto/lto-common.c:2460 > > 0x63f459 read_cgraph_and_symbols(unsigned int, char const**) > > /gcc/lto/lto-common.c:2693 > > 0x620fa2 lto_main() > > /gcc/lto/lto.c:616 > > Please submit a full bug report, > > > > Christophe
Hi,
this is updated patch for ODR based canonical type calculation. It makes use
of TYPE_CXX_ODR_P instead of doing the guesswork and while thinking how to get
rid of the quadratic behaviour of the hash I noticed that all the logic fits
quite naturally into gimple_register_canonical_type_1.
We only need to arrange that all non-ODR types gets to hash before we has
TYPE_CXX_ODR_P and since hash functions recursively populate the type hash
we know that subtypes with linkage are processed before types.
From the WPA stats we now get relatively few non-ODR types building cc1plus
[WPA] Compared 1062055 SCCs, 890518 collisions (0.838486)
[WPA] Merged 1059104 SCCs
[WPA] Merged 6025225 tree bodies
[WPA] Merged 639655 types
[WPA] 101515 types prevailed (177264 associated trees)
[WPA] GIMPLE canonical type table: size 16381, 262 elements, 5685 searches, 48 collisions (ratio: 0.008443)
[WPA] GIMPLE canonical type pointer-map: 3548 elements, 14261 searches
So 262 distinct types compared to 1590 which is an effect of Jason's patch.
There are 3286 ODR types having type canonical set by name and 910 have
conflict to non-ODR type.
I am still waiting for the statistics of alias oracle, but for tramp3d there
are no significant changes compared to the first patch.
lto-bootstrapped/regtested x86_64-linux, OK?
* class.c (layout_class_type): Set TYPE_CXX_ODR_P for as-base
type copy.
* ipa-devirt.c (odr_type_d): Add tbaa_enabled flag.
(add_type_duplicate): When odr hash is not allocated, to nothing.
(odr_based_tbaa_p): New function.
(set_type_canonical_for_odr_type): New function.
* ipa-utils.h (enable_odr_based_tbaa, odr_based_tbaa_p,
set_type_canonical_for_odr_type): New.
* lto-common.c: Include demangle.h and tree-pretty-print.h
(type_streaming_finished): New static var.
(gimple_register_canonical_type_1): Return updated hash; handle ODR
types.
(iterative_hash_canonical_type): Update use of
gimple_register_canonical_type_1.
* tree.c (gimple_canonical_types_compatible_p): ODR types with
ODR based TBAA are not equivalent to non-ODR types.
* g++.dg/lto/alias-2_0.C: New testcase.
* g++.dg/lto/alias-2_1.C: New testcase.
Index: cp/class.c
===================================================================
--- cp/class.c (revision 272656)
+++ cp/class.c (working copy)
@@ -6395,6 +6395,7 @@ layout_class_type (tree t, tree *virtual
SET_TYPE_ALIGN (base_t, rli->record_align);
TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t);
TYPE_TYPELESS_STORAGE (base_t) = TYPE_TYPELESS_STORAGE (t);
+ TYPE_CXX_ODR_P (base_t) = true;
/* Copy the non-static data members of T. This will include its
direct non-virtual bases & vtable. */
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c (revision 272656)
+++ ipa-devirt.c (working copy)
@@ -213,6 +213,8 @@ struct GTY(()) odr_type_d
bool odr_violated;
/* Set when virtual table without RTTI previaled table with. */
bool rtti_broken;
+ /* Set when the canonical type is determined using the type name. */
+ bool tbaa_enabled;
};
/* Return TRUE if all derived types of T are known and thus
@@ -1610,6 +1612,9 @@ add_type_duplicate (odr_type val, tree t
val->types_set->add (type);
+ if (!odr_hash)
+ return NULL;
+
gcc_checking_assert (can_be_name_hashed_p (type)
&& can_be_name_hashed_p (val->type));
@@ -1989,6 +1994,46 @@ prevailing_odr_type (tree type)
return t->type;
}
+/* Set tbaa_enabled flag for TYPE. */
+
+void
+enable_odr_based_tbaa (tree type)
+{
+ odr_type t = get_odr_type (type, true);
+ t->tbaa_enabled = true;
+}
+
+/* True if canonical type of TYPE is determined using ODR name. */
+
+bool
+odr_based_tbaa_p (const_tree type)
+{
+ if (!RECORD_OR_UNION_TYPE_P (type))
+ return false;
+ odr_type t = get_odr_type (const_cast <tree> (type), false);
+ if (!t || !t->tbaa_enabled)
+ return false;
+ return true;
+}
+
+/* Set TYPE_CANONICAL of type and all its variants and duplicates
+ to CANONICAL. */
+
+void
+set_type_canonical_for_odr_type (tree type, tree canonical)
+{
+ odr_type t = get_odr_type (type, false);
+ unsigned int i;
+ tree tt;
+
+ for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2))
+ TYPE_CANONICAL (t2) = canonical;
+ if (t->types)
+ FOR_EACH_VEC_ELT (*t->types, i, tt)
+ for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2))
+ TYPE_CANONICAL (t2) = canonical;
+}
+
/* Return true if we reported some ODR violation on TYPE. */
bool
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 272656)
+++ ipa-utils.h (working copy)
@@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t
bool odr_types_equivalent_p (tree type1, tree type2);
bool odr_type_violation_reported_p (tree type);
tree prevailing_odr_type (tree type);
+void enable_odr_based_tbaa (tree type);
+bool odr_based_tbaa_p (const_tree type);
+void set_type_canonical_for_odr_type (tree type, tree canonical);
/* Return vector containing possible targets of polymorphic call E.
If COMPLETEP is non-NULL, store true if the list is complete.
Index: lto/lto-common.c
===================================================================
--- lto/lto-common.c (revision 272656)
+++ lto/lto-common.c (working copy)
@@ -1,5 +1,5 @@
/* Top-level LTO routines.
- Copyright (C) 2009-2018 Free Software Foundation, Inc.
+ Copyright (C) 2009-2019 Free Software Foundation, Inc.
Contributed by CodeSourcery, Inc.
This file is part of GCC.
@@ -56,6 +56,12 @@ along with GCC; see the file COPYING3.
#include "attribs.h"
#include "builtins.h"
#include "lto-common.h"
+#include "tree-pretty-print.h"
+#include "demangle.h"
+
+/* True when no new types are going to be streamd from the global stream. */
+
+static bool type_streaming_finished = false;
GTY(()) tree first_personality_decl;
@@ -217,9 +223,14 @@ static hash_map<const_tree, hashval_t> *
static unsigned long num_canonical_type_hash_entries;
static unsigned long num_canonical_type_hash_queries;
+/* Types postponed for registration to the canonical type table.
+ During streaming we postpone all TYPE_CXX_ODR_P types so we can alter
+ decide whether there is conflict with non-ODR type or not. */
+static GTY(()) vec<tree, va_gc> *types_to_register = NULL;
+
static void iterative_hash_canonical_type (tree type, inchash::hash &hstate);
static hashval_t gimple_canonical_type_hash (const void *p);
-static void gimple_register_canonical_type_1 (tree t, hashval_t hash);
+static hashval_t gimple_register_canonical_type_1 (tree t, hashval_t hash);
/* Returning a hash value for gimple type TYPE.
@@ -357,7 +368,7 @@ iterative_hash_canonical_type (tree type
optimal order. To avoid quadratic behavior also register the
type here. */
v = hash_canonical_type (type);
- gimple_register_canonical_type_1 (type, v);
+ v = gimple_register_canonical_type_1 (type, v);
}
hstate.add_int (v);
}
@@ -388,7 +399,7 @@ gimple_canonical_type_eq (const void *p1
/* Main worker for gimple_register_canonical_type. */
-static void
+static hashval_t
gimple_register_canonical_type_1 (tree t, hashval_t hash)
{
void **slot;
@@ -397,6 +408,77 @@ gimple_register_canonical_type_1 (tree t
&& type_with_alias_set_p (t)
&& canonical_type_used_p (t));
+ /* ODR types for which there is no ODR violation and we did not record
+ structurally equivalent non-ODR type can be treated as unique by their
+ name.
+
+ hash passed to gimple_register_canonical_type_1 is a structural hash
+ that we can use to lookup structurally equivalent non-ODR type.
+ In case we decide to treat type as unique ODR type we recompute hash based
+ on name and let TBAA machinery know about our decision. */
+ if (RECORD_OR_UNION_TYPE_P (t)
+ && odr_type_p (t) && !odr_type_violation_reported_p (t))
+ {
+ /* Here we rely on fact that all non-ODR types was inserted into
+ canonical type hash and thus we can safely detect conflicts between
+ ODR types and interoperable non-ODR types. */
+ gcc_checking_assert (type_streaming_finished
+ && TYPE_MAIN_VARIANT (t) == t);
+ slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash,
+ NO_INSERT);
+ if (slot && !TYPE_CXX_ODR_P (*(tree *)slot))
+ {
+ tree nonodr = *(tree *)slot;
+ if (symtab->dump_file)
+ {
+ char *name = cplus_demangle_v3
+ (IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0);
+ fprintf (symtab->dump_file,
+ "ODR and non-ODR type conflict: ");
+ print_generic_expr (symtab->dump_file, t);
+ fprintf (symtab->dump_file, " and ");
+ print_generic_expr (symtab->dump_file, nonodr);
+ fprintf (symtab->dump_file, " demangled:%s\n", name);
+ free (name);
+ }
+ /* Set canonical for T and all other ODR equivalent duplicates
+ including incomplete structures. */
+ set_type_canonical_for_odr_type (t, nonodr);
+ }
+ else
+ {
+ tree prevail = prevailing_odr_type (t);
+
+ if (symtab->dump_file)
+ {
+ char *name = cplus_demangle_v3
+ (IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0);
+
+ fprintf (symtab->dump_file,
+ "New canonical ODR type: ");
+ print_generic_expr (symtab->dump_file, t);
+ fprintf (symtab->dump_file, " demangled:%s\n", name);
+ free (name);
+ }
+ /* Set canonical for T and all other ODR equivalent duplicates
+ including incomplete structures. */
+ set_type_canonical_for_odr_type (t, prevail);
+ enable_odr_based_tbaa (t);
+ if (!type_in_anonymous_namespace_p (t))
+ hash = htab_hash_string (IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME
+ (TYPE_NAME (prevail))));
+ else
+ hash = TYPE_UID (TYPE_MAIN_VARIANT (t));
+ num_canonical_type_hash_entries++;
+ bool existed_p = canonical_type_hash_cache->put (prevail, hash);
+ gcc_checking_assert (!existed_p);
+ }
+ return hash;
+ }
+
slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT);
if (*slot)
{
@@ -413,6 +495,7 @@ gimple_register_canonical_type_1 (tree t
bool existed_p = canonical_type_hash_cache->put (t, hash);
gcc_assert (!existed_p);
}
+ return hash;
}
/* Register type T in the global type table gimple_types and set
@@ -464,6 +547,34 @@ lto_register_canonical_types (tree node,
gimple_register_canonical_type (node);
}
+/* Finish canonical type calculation: after all units has been streamed in we
+ can check if given ODR type structurally conflicts with a non-ODR type. In
+ the first case we set type canonical according to the canonical type hash.
+ In the second case we use type names. */
+
+static void
+lto_register_canonical_types_for_odr_types ()
+{
+ tree t;
+ unsigned int i;
+
+ if (!types_to_register)
+ return;
+
+ type_streaming_finished = true;
+
+ /* Be sure that no types derived from ODR types was
+ not inserted into the hash table. */
+ if (flag_checking)
+ FOR_EACH_VEC_ELT (*types_to_register, i, t)
+ gcc_assert (!TYPE_CANONICAL (t));
+
+ /* Register all remaining types. */
+ FOR_EACH_VEC_ELT (*types_to_register, i, t)
+ if (!TYPE_CANONICAL (t))
+ gimple_register_canonical_type (t);
+}
+
/* Remember trees that contains references to declarations. */
vec <tree, va_gc> *tree_with_vars;
@@ -1657,6 +1768,7 @@ unify_scc (struct data_in *data_in, unsi
}
+
/* Read all the symbols from buffer DATA, using descriptors in DECL_DATA.
RESOLUTIONS is the set of symbols picked by the linker (read from the
resolution file when the linker plugin is being used). */
@@ -1749,12 +1861,23 @@ lto_read_decls (struct lto_file_decl_dat
num_prevailing_types++;
lto_fixup_prevailing_type (t);
- /* Compute the canonical type of all types.
+ /* Compute the canonical type of all non-ODR types.
+ Delay ODR types for the end of merging process - the canonical
+ type for those can be computed using the (unique) name however
+ we want to do this only if units in other languages do not
+ contain structurally equivalent type.
+
Because SCC components are streamed in random (hash) order
we may have encountered the type before while registering
type canonical of a derived type in the same SCC. */
if (!TYPE_CANONICAL (t))
- gimple_register_canonical_type (t);
+ {
+ if (!RECORD_OR_UNION_TYPE_P (t)
+ || !TYPE_CXX_ODR_P (t))
+ gimple_register_canonical_type (t);
+ else if (COMPLETE_TYPE_P (t))
+ vec_safe_push (types_to_register, t);
+ }
if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t))
register_odr_type (t);
}
@@ -2605,6 +2728,8 @@ read_cgraph_and_symbols (unsigned nfiles
ggc_free(decl_data);
real_file_decl_data = NULL;
+ lto_register_canonical_types_for_odr_types ();
+
if (resolution_file_name)
fclose (resolution);
Index: testsuite/g++.dg/lto/alias-2_0.C
===================================================================
--- testsuite/g++.dg/lto/alias-2_0.C (nonexistent)
+++ testsuite/g++.dg/lto/alias-2_0.C (working copy)
@@ -0,0 +1,31 @@
+/* { dg-lto-do run } */
+/* { dg-lto-options { { -O2 -flto } } } */
+
+/* With LTO we consider all pointers to incomplete types to be possibly
+ aliasing. This makes *bptr to alias with aptr.
+ For C++ ODR types we however can work out that they are actually
+ different. */
+
+#include <string.h>
+
+typedef int (*fnptr) ();
+
+__attribute__ ((used))
+struct a *aptr;
+
+__attribute__ ((used))
+struct b **bptr = (struct b**)&aptr;
+extern void init ();
+extern void inline_me_late (int);
+
+
+int
+main (int argc, char **argv)
+{
+ init ();
+ aptr = 0;
+ inline_me_late (argc);
+ if (!__builtin_constant_p (aptr == 0))
+ __builtin_abort ();
+ return (size_t)aptr;
+}
Index: testsuite/g++.dg/lto/alias-2_1.C
===================================================================
--- testsuite/g++.dg/lto/alias-2_1.C (nonexistent)
+++ testsuite/g++.dg/lto/alias-2_1.C (working copy)
@@ -0,0 +1,16 @@
+#include <string.h>
+struct a {int a;} a;
+struct b {int b;} b;
+extern struct b **bptr;
+void
+inline_me_late (int argc)
+{
+ if (argc == -1)
+ *bptr = (struct b *)(size_t)1;
+}
+void
+init()
+{
+ a.a=1;
+ b.b=2;
+}
Index: tree.c
===================================================================
--- tree.c (revision 272656)
+++ tree.c (working copy)
@@ -14103,6 +14103,7 @@ gimple_canonical_types_compatible_p (con
gcc_assert (!trust_type_canonical
|| (type_with_alias_set_p (t1) && type_with_alias_set_p (t2)));
+
/* If the types have been previously registered and found equal
they still are. */
@@ -14120,6 +14121,14 @@ gimple_canonical_types_compatible_p (con
return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2);
}
+ /* For types where we do ODR based TBAA the canonical type is always
+ set correctly, so we know that types are different if their
+ canonical types does not match. */
+ if (trust_type_canonical
+ && (odr_type_p (t1) && odr_based_tbaa_p (t1))
+ != (odr_type_p (t2) && odr_based_tbaa_p (t2)))
+ return false;
+
/* Can't be the same type if the types don't have the same code. */
enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1));
if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2)))
On Thu, 27 Jun 2019, Jan Hubicka wrote: > Hi, > this is updated patch for ODR based canonical type calculation. It makes use > of TYPE_CXX_ODR_P instead of doing the guesswork and while thinking how to get > rid of the quadratic behaviour of the hash I noticed that all the logic fits > quite naturally into gimple_register_canonical_type_1. > > We only need to arrange that all non-ODR types gets to hash before we has > TYPE_CXX_ODR_P and since hash functions recursively populate the type hash > we know that subtypes with linkage are processed before types. > > From the WPA stats we now get relatively few non-ODR types building cc1plus > [WPA] Compared 1062055 SCCs, 890518 collisions (0.838486) > [WPA] Merged 1059104 SCCs > [WPA] Merged 6025225 tree bodies > [WPA] Merged 639655 types > [WPA] 101515 types prevailed (177264 associated trees) > [WPA] GIMPLE canonical type table: size 16381, 262 elements, 5685 searches, 48 collisions (ratio: 0.008443) > [WPA] GIMPLE canonical type pointer-map: 3548 elements, 14261 searches > > So 262 distinct types compared to 1590 which is an effect of Jason's patch. > There are 3286 ODR types having type canonical set by name and 910 have > conflict to non-ODR type. > > I am still waiting for the statistics of alias oracle, but for tramp3d there > are no significant changes compared to the first patch. > > lto-bootstrapped/regtested x86_64-linux, OK? > > * class.c (layout_class_type): Set TYPE_CXX_ODR_P for as-base > type copy. > > * ipa-devirt.c (odr_type_d): Add tbaa_enabled flag. > (add_type_duplicate): When odr hash is not allocated, to nothing. > (odr_based_tbaa_p): New function. > (set_type_canonical_for_odr_type): New function. > * ipa-utils.h (enable_odr_based_tbaa, odr_based_tbaa_p, > set_type_canonical_for_odr_type): New. > * lto-common.c: Include demangle.h and tree-pretty-print.h > (type_streaming_finished): New static var. > (gimple_register_canonical_type_1): Return updated hash; handle ODR > types. > (iterative_hash_canonical_type): Update use of > gimple_register_canonical_type_1. > * tree.c (gimple_canonical_types_compatible_p): ODR types with > ODR based TBAA are not equivalent to non-ODR types. > > * g++.dg/lto/alias-2_0.C: New testcase. > * g++.dg/lto/alias-2_1.C: New testcase. > Index: cp/class.c > =================================================================== > --- cp/class.c (revision 272656) > +++ cp/class.c (working copy) > @@ -6395,6 +6395,7 @@ layout_class_type (tree t, tree *virtual > SET_TYPE_ALIGN (base_t, rli->record_align); > TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t); > TYPE_TYPELESS_STORAGE (base_t) = TYPE_TYPELESS_STORAGE (t); > + TYPE_CXX_ODR_P (base_t) = true; = TYPE_CXX_ODR_P (t); would be much more obvious here. > /* Copy the non-static data members of T. This will include its > direct non-virtual bases & vtable. */ > Index: ipa-devirt.c > =================================================================== > --- ipa-devirt.c (revision 272656) > +++ ipa-devirt.c (working copy) > @@ -213,6 +213,8 @@ struct GTY(()) odr_type_d > bool odr_violated; > /* Set when virtual table without RTTI previaled table with. */ > bool rtti_broken; > + /* Set when the canonical type is determined using the type name. */ > + bool tbaa_enabled; > }; > > /* Return TRUE if all derived types of T are known and thus > @@ -1610,6 +1612,9 @@ add_type_duplicate (odr_type val, tree t > > val->types_set->add (type); > > + if (!odr_hash) > + return NULL; > + > gcc_checking_assert (can_be_name_hashed_p (type) > && can_be_name_hashed_p (val->type)); > > @@ -1989,6 +1994,46 @@ prevailing_odr_type (tree type) > return t->type; > } > > +/* Set tbaa_enabled flag for TYPE. */ > + > +void > +enable_odr_based_tbaa (tree type) > +{ > + odr_type t = get_odr_type (type, true); > + t->tbaa_enabled = true; > +} > + > +/* True if canonical type of TYPE is determined using ODR name. */ > + > +bool > +odr_based_tbaa_p (const_tree type) > +{ > + if (!RECORD_OR_UNION_TYPE_P (type)) > + return false; > + odr_type t = get_odr_type (const_cast <tree> (type), false); > + if (!t || !t->tbaa_enabled) > + return false; > + return true; > +} > + > +/* Set TYPE_CANONICAL of type and all its variants and duplicates > + to CANONICAL. */ > + > +void > +set_type_canonical_for_odr_type (tree type, tree canonical) > +{ > + odr_type t = get_odr_type (type, false); > + unsigned int i; > + tree tt; > + > + for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > + if (t->types) > + FOR_EACH_VEC_ELT (*t->types, i, tt) > + for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > +} > + > /* Return true if we reported some ODR violation on TYPE. */ > > bool > Index: ipa-utils.h > =================================================================== > --- ipa-utils.h (revision 272656) > +++ ipa-utils.h (working copy) > @@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t > bool odr_types_equivalent_p (tree type1, tree type2); > bool odr_type_violation_reported_p (tree type); > tree prevailing_odr_type (tree type); > +void enable_odr_based_tbaa (tree type); > +bool odr_based_tbaa_p (const_tree type); > +void set_type_canonical_for_odr_type (tree type, tree canonical); > > /* Return vector containing possible targets of polymorphic call E. > If COMPLETEP is non-NULL, store true if the list is complete. > Index: lto/lto-common.c > =================================================================== > --- lto/lto-common.c (revision 272656) > +++ lto/lto-common.c (working copy) > @@ -1,5 +1,5 @@ > /* Top-level LTO routines. > - Copyright (C) 2009-2018 Free Software Foundation, Inc. > + Copyright (C) 2009-2019 Free Software Foundation, Inc. > Contributed by CodeSourcery, Inc. > > This file is part of GCC. > @@ -56,6 +56,12 @@ along with GCC; see the file COPYING3. > #include "attribs.h" > #include "builtins.h" > #include "lto-common.h" > +#include "tree-pretty-print.h" > +#include "demangle.h" > + > +/* True when no new types are going to be streamd from the global stream. */ > + > +static bool type_streaming_finished = false; > > GTY(()) tree first_personality_decl; > > @@ -217,9 +223,14 @@ static hash_map<const_tree, hashval_t> * > static unsigned long num_canonical_type_hash_entries; > static unsigned long num_canonical_type_hash_queries; > > +/* Types postponed for registration to the canonical type table. > + During streaming we postpone all TYPE_CXX_ODR_P types so we can alter > + decide whether there is conflict with non-ODR type or not. */ > +static GTY(()) vec<tree, va_gc> *types_to_register = NULL; > + > static void iterative_hash_canonical_type (tree type, inchash::hash &hstate); > static hashval_t gimple_canonical_type_hash (const void *p); > -static void gimple_register_canonical_type_1 (tree t, hashval_t hash); > +static hashval_t gimple_register_canonical_type_1 (tree t, hashval_t hash); > > /* Returning a hash value for gimple type TYPE. > > @@ -357,7 +368,7 @@ iterative_hash_canonical_type (tree type > optimal order. To avoid quadratic behavior also register the > type here. */ > v = hash_canonical_type (type); > - gimple_register_canonical_type_1 (type, v); > + v = gimple_register_canonical_type_1 (type, v); > } > hstate.add_int (v); > } > @@ -388,7 +399,7 @@ gimple_canonical_type_eq (const void *p1 > > /* Main worker for gimple_register_canonical_type. */ > > -static void > +static hashval_t > gimple_register_canonical_type_1 (tree t, hashval_t hash) > { > void **slot; > @@ -397,6 +408,77 @@ gimple_register_canonical_type_1 (tree t > && type_with_alias_set_p (t) > && canonical_type_used_p (t)); > > + /* ODR types for which there is no ODR violation and we did not record > + structurally equivalent non-ODR type can be treated as unique by their > + name. > + > + hash passed to gimple_register_canonical_type_1 is a structural hash > + that we can use to lookup structurally equivalent non-ODR type. > + In case we decide to treat type as unique ODR type we recompute hash based > + on name and let TBAA machinery know about our decision. */ > + if (RECORD_OR_UNION_TYPE_P (t) > + && odr_type_p (t) && !odr_type_violation_reported_p (t)) > + { > + /* Here we rely on fact that all non-ODR types was inserted into > + canonical type hash and thus we can safely detect conflicts between > + ODR types and interoperable non-ODR types. */ > + gcc_checking_assert (type_streaming_finished > + && TYPE_MAIN_VARIANT (t) == t); > + slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, > + NO_INSERT); > + if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) > + { > + tree nonodr = *(tree *)slot; > + if (symtab->dump_file) > + { > + char *name = cplus_demangle_v3 > + (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0); Ugh - do we really want to demangle here? I think not. Surely not without -details or with -slim. Readers can c++filt this easily. > + fprintf (symtab->dump_file, > + "ODR and non-ODR type conflict: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, " and "); > + print_generic_expr (symtab->dump_file, nonodr); > + fprintf (symtab->dump_file, " demangled:%s\n", name); > + free (name); > + } > + /* Set canonical for T and all other ODR equivalent duplicates > + including incomplete structures. */ > + set_type_canonical_for_odr_type (t, nonodr); > + } > + else > + { > + tree prevail = prevailing_odr_type (t); > + > + if (symtab->dump_file) > + { > + char *name = cplus_demangle_v3 > + (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0); Likewise. > + > + fprintf (symtab->dump_file, > + "New canonical ODR type: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, " demangled:%s\n", name); > + free (name); > + } > + /* Set canonical for T and all other ODR equivalent duplicates > + including incomplete structures. */ > + set_type_canonical_for_odr_type (t, prevail); > + enable_odr_based_tbaa (t); I suppose there never will be a set of ODR types with the same prevailing type but some of them having a conflict with a nonodr type and some not? > + if (!type_in_anonymous_namespace_p (t)) > + hash = htab_hash_string (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME > + (TYPE_NAME (prevail)))); > + else > + hash = TYPE_UID (TYPE_MAIN_VARIANT (t)); > + num_canonical_type_hash_entries++; > + bool existed_p = canonical_type_hash_cache->put (prevail, hash); but those hash differently? I think you wanted to put t, not prevail here. And you want to use the TYPE_UID of prevail as well? Otherwise looks good. You can commit the C++ FE change with the adjustment in case it fixes the reported verification ICEs. Richard. > + gcc_checking_assert (!existed_p); > + } > + return hash; > + } > + > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > { > @@ -413,6 +495,7 @@ gimple_register_canonical_type_1 (tree t > bool existed_p = canonical_type_hash_cache->put (t, hash); > gcc_assert (!existed_p); > } > + return hash; > } > > /* Register type T in the global type table gimple_types and set > @@ -464,6 +547,34 @@ lto_register_canonical_types (tree node, > gimple_register_canonical_type (node); > } > > +/* Finish canonical type calculation: after all units has been streamed in we > + can check if given ODR type structurally conflicts with a non-ODR type. In > + the first case we set type canonical according to the canonical type hash. > + In the second case we use type names. */ > + > +static void > +lto_register_canonical_types_for_odr_types () > +{ > + tree t; > + unsigned int i; > + > + if (!types_to_register) > + return; > + > + type_streaming_finished = true; > + > + /* Be sure that no types derived from ODR types was > + not inserted into the hash table. */ > + if (flag_checking) > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + gcc_assert (!TYPE_CANONICAL (t)); > + > + /* Register all remaining types. */ > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + if (!TYPE_CANONICAL (t)) > + gimple_register_canonical_type (t); > +} > + > > /* Remember trees that contains references to declarations. */ > vec <tree, va_gc> *tree_with_vars; > @@ -1657,6 +1768,7 @@ unify_scc (struct data_in *data_in, unsi > } > > > + > /* Read all the symbols from buffer DATA, using descriptors in DECL_DATA. > RESOLUTIONS is the set of symbols picked by the linker (read from the > resolution file when the linker plugin is being used). */ > @@ -1749,12 +1861,23 @@ lto_read_decls (struct lto_file_decl_dat > num_prevailing_types++; > lto_fixup_prevailing_type (t); > > - /* Compute the canonical type of all types. > + /* Compute the canonical type of all non-ODR types. > + Delay ODR types for the end of merging process - the canonical > + type for those can be computed using the (unique) name however > + we want to do this only if units in other languages do not > + contain structurally equivalent type. > + > Because SCC components are streamed in random (hash) order > we may have encountered the type before while registering > type canonical of a derived type in the same SCC. */ > if (!TYPE_CANONICAL (t)) > - gimple_register_canonical_type (t); > + { > + if (!RECORD_OR_UNION_TYPE_P (t) > + || !TYPE_CXX_ODR_P (t)) > + gimple_register_canonical_type (t); > + else if (COMPLETE_TYPE_P (t)) > + vec_safe_push (types_to_register, t); > + } > if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t)) > register_odr_type (t); > } > @@ -2605,6 +2728,8 @@ read_cgraph_and_symbols (unsigned nfiles > ggc_free(decl_data); > real_file_decl_data = NULL; > > + lto_register_canonical_types_for_odr_types (); > + > if (resolution_file_name) > fclose (resolution); > > Index: testsuite/g++.dg/lto/alias-2_0.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_0.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_0.C (working copy) > @@ -0,0 +1,31 @@ > +/* { dg-lto-do run } */ > +/* { dg-lto-options { { -O2 -flto } } } */ > + > +/* With LTO we consider all pointers to incomplete types to be possibly > + aliasing. This makes *bptr to alias with aptr. > + For C++ ODR types we however can work out that they are actually > + different. */ > + > +#include <string.h> > + > +typedef int (*fnptr) (); > + > +__attribute__ ((used)) > +struct a *aptr; > + > +__attribute__ ((used)) > +struct b **bptr = (struct b**)&aptr; > +extern void init (); > +extern void inline_me_late (int); > + > + > +int > +main (int argc, char **argv) > +{ > + init (); > + aptr = 0; > + inline_me_late (argc); > + if (!__builtin_constant_p (aptr == 0)) > + __builtin_abort (); > + return (size_t)aptr; > +} > Index: testsuite/g++.dg/lto/alias-2_1.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_1.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_1.C (working copy) > @@ -0,0 +1,16 @@ > +#include <string.h> > +struct a {int a;} a; > +struct b {int b;} b; > +extern struct b **bptr; > +void > +inline_me_late (int argc) > +{ > + if (argc == -1) > + *bptr = (struct b *)(size_t)1; > +} > +void > +init() > +{ > + a.a=1; > + b.b=2; > +} > Index: tree.c > =================================================================== > --- tree.c (revision 272656) > +++ tree.c (working copy) > @@ -14103,6 +14103,7 @@ gimple_canonical_types_compatible_p (con > > gcc_assert (!trust_type_canonical > || (type_with_alias_set_p (t1) && type_with_alias_set_p (t2))); > + > /* If the types have been previously registered and found equal > they still are. */ > > @@ -14120,6 +14121,14 @@ gimple_canonical_types_compatible_p (con > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > } > > + /* For types where we do ODR based TBAA the canonical type is always > + set correctly, so we know that types are different if their > + canonical types does not match. */ > + if (trust_type_canonical > + && (odr_type_p (t1) && odr_based_tbaa_p (t1)) > + != (odr_type_p (t2) && odr_based_tbaa_p (t2))) > + return false; > + > /* Can't be the same type if the types don't have the same code. */ > enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1)); > if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2))) >
> > + if (RECORD_OR_UNION_TYPE_P (t) > > + && odr_type_p (t) && !odr_type_violation_reported_p (t)) > > + { > > + /* Here we rely on fact that all non-ODR types was inserted into > > + canonical type hash and thus we can safely detect conflicts between > > + ODR types and interoperable non-ODR types. */ > > + gcc_checking_assert (type_streaming_finished > > + && TYPE_MAIN_VARIANT (t) == t); > > + slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, > > + NO_INSERT); > > + if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) > > + { > > + tree nonodr = *(tree *)slot; > > + if (symtab->dump_file) > > + { > > + char *name = cplus_demangle_v3 > > + (IDENTIFIER_POINTER > > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0); > > Ugh - do we really want to demangle here? I think not. Surely > not without -details or with -slim. Readers can c++filt this > easily. OK, i can dump mangled name. Just print_generic_expr prints all instantiations of the tempalte same way which makes it look odd that there are so many types of same name. > > + /* Set canonical for T and all other ODR equivalent duplicates > > + including incomplete structures. */ > > + set_type_canonical_for_odr_type (t, prevail); > > + enable_odr_based_tbaa (t); > > I suppose there never will be a set of ODR types with the same > prevailing type but some of them having a conflict with a nonodr > type and some not? No, we check them by structural equality while verifying ODR violations that is more strict than gimple_canonical_types_compatible. > > > + if (!type_in_anonymous_namespace_p (t)) > > + hash = htab_hash_string (IDENTIFIER_POINTER > > + (DECL_ASSEMBLER_NAME > > + (TYPE_NAME (prevail)))); > > + else > > + hash = TYPE_UID (TYPE_MAIN_VARIANT (t)); > > + num_canonical_type_hash_entries++; > > + bool existed_p = canonical_type_hash_cache->put (prevail, hash); > > but those hash differently? I think you wanted to put t, not prevail > here. And you want to use the TYPE_UID of prevail as well? Actually this was intended to be prevail in both cases, but it is harmless. The difference here is that anonymous types have DECL_ASSEMBLED_NAME <anon>, so if we inserted them to the hash table based on names they will all conflict. Anonymous namespace types never have duplicated main variants, so t==prevail here, but I will update the patch to use prevail since it is much more obvious (I am also not sure what I was thinking of when I typed TYPE_MAIN_VARIANT) > > Otherwise looks good. > > You can commit the C++ FE change with the adjustment in case it > fixes the reported verification ICEs. It only fixes ICE WRT the sanity checking that all non-ODR types are inserted first. W/o that change the as-base type will get inserted during streaming in and recursively we will insert ODR types early. I am waiting for testcase WRT the other ODR ICE reported today. I think tree.c when creating type variants wants to copy the flag: the wrong type is va_list which is likely created by a backend bypassing the C++ hook. I will re-test with these changes. Honza
On Thu, 27 Jun 2019, Jan Hubicka wrote: > > > + if (RECORD_OR_UNION_TYPE_P (t) > > > + && odr_type_p (t) && !odr_type_violation_reported_p (t)) > > > + { > > > + /* Here we rely on fact that all non-ODR types was inserted into > > > + canonical type hash and thus we can safely detect conflicts between > > > + ODR types and interoperable non-ODR types. */ > > > + gcc_checking_assert (type_streaming_finished > > > + && TYPE_MAIN_VARIANT (t) == t); > > > + slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, > > > + NO_INSERT); > > > + if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) > > > + { > > > + tree nonodr = *(tree *)slot; > > > + if (symtab->dump_file) > > > + { > > > + char *name = cplus_demangle_v3 > > > + (IDENTIFIER_POINTER > > > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t))), 0); > > > > Ugh - do we really want to demangle here? I think not. Surely > > not without -details or with -slim. Readers can c++filt this > > easily. > > OK, i can dump mangled name. > Just print_generic_expr prints all instantiations of the tempalte same > way which makes it look odd that there are so many types of same name. > > > + /* Set canonical for T and all other ODR equivalent duplicates > > > + including incomplete structures. */ > > > + set_type_canonical_for_odr_type (t, prevail); > > > + enable_odr_based_tbaa (t); > > > > I suppose there never will be a set of ODR types with the same > > prevailing type but some of them having a conflict with a nonodr > > type and some not? > > No, we check them by structural equality while verifying ODR violations > that is more strict than gimple_canonical_types_compatible. > > > > > + if (!type_in_anonymous_namespace_p (t)) > > > + hash = htab_hash_string (IDENTIFIER_POINTER > > > + (DECL_ASSEMBLER_NAME > > > + (TYPE_NAME (prevail)))); > > > + else > > > + hash = TYPE_UID (TYPE_MAIN_VARIANT (t)); > > > + num_canonical_type_hash_entries++; > > > + bool existed_p = canonical_type_hash_cache->put (prevail, hash); > > > > but those hash differently? I think you wanted to put t, not prevail > > here. And you want to use the TYPE_UID of prevail as well? > > Actually this was intended to be prevail in both cases, but it is > harmless. The difference here is that anonymous types have > DECL_ASSEMBLED_NAME <anon>, so if we inserted them to the hash > table based on names they will all conflict. > > Anonymous namespace types never have duplicated main variants, so > t==prevail here, but I will update the patch to use prevail since it is > much more obvious (I am also not sure what I was thinking of when > I typed TYPE_MAIN_VARIANT) I think using 't' is more obvious here since that's what we should cache the hash value for. That is, even in the !type_in_anonymous_namespace_p (t) case you want to cache 't's hash, not prevails. But yes, TYPE_UID (prevail) might be more obvious to you. > > > > Otherwise looks good. > > > > You can commit the C++ FE change with the adjustment in case it > > fixes the reported verification ICEs. > > It only fixes ICE WRT the sanity checking that all non-ODR types > are inserted first. W/o that change the as-base type will get inserted > during streaming in and recursively we will insert ODR types early. > > I am waiting for testcase WRT the other ODR ICE reported today. I think > tree.c when creating type variants wants to copy the flag: the wrong > type is va_list which is likely created by a backend bypassing the C++ > hook. > > I will re-test with these changes. > > Honza >
> > Actually this was intended to be prevail in both cases, but it is > > harmless. The difference here is that anonymous types have > > DECL_ASSEMBLED_NAME <anon>, so if we inserted them to the hash > > table based on names they will all conflict. > > > > Anonymous namespace types never have duplicated main variants, so > > t==prevail here, but I will update the patch to use prevail since it is > > much more obvious (I am also not sure what I was thinking of when > > I typed TYPE_MAIN_VARIANT) > > I think using 't' is more obvious here since that's what we should cache > the hash value for. That is, even in the !type_in_anonymous_namespace_p > (t) case you want to cache 't's hash, not prevails. But yes, > TYPE_UID (prevail) might be more obvious to you. OK, I will use t on both: for !anonoymous_namespace types the names are the same. Honza > > > > > > > Otherwise looks good. > > > > > > You can commit the C++ FE change with the adjustment in case it > > > fixes the reported verification ICEs. > > > > It only fixes ICE WRT the sanity checking that all non-ODR types > > are inserted first. W/o that change the as-base type will get inserted > > during streaming in and recursively we will insert ODR types early. > > > > I am waiting for testcase WRT the other ODR ICE reported today. I think > > tree.c when creating type variants wants to copy the flag: the wrong > > type is va_list which is likely created by a backend bypassing the C++ > > hook. > > > > I will re-test with these changes. > > > > Honza > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
Hi,
here is update patch I am re-testing. Ok if it suceeds?
Orace quary stats finished in meantime.
Alias oracle query stats:
refs_may_alias_p: 39232255 disambiguations, 47436580 queries
ref_maybe_used_by_call_p: 59801 disambiguations, 39811399 queries
call_may_clobber_ref_p: 5967 disambiguations, 9009 queries
nonoverlapping_component_refs_p: 10184 disambiguations, 754231 queries
nonoverlapping_component_refs_of_decl_p: 6488 disambiguations, 214611 queries
aliasing_component_refs_p: 88820 disambiguations, 291741 queries
TBAA oracle: 11687911 disambiguations 34188770 queries
13343414 are in alias set 0
5417962 queries asked about the same object
148 queries asked about the same alias set
0 access volatile
1672142 are dependent in the DAG
2067193 are aritificially in conflict with void *
PTA query stats:
pt_solution_includes: 502249 disambiguations, 7180660 queries
pt_solutions_intersect: 357826 disambiguations, 7059723 queries
So there is no big change in TBAA effectivity compared to previous patch.
There is however nice improvements in aliasing_component_refs_p
61523 -> 88820 which I think is due to Jason's change
Honza
Index: cp/class.c
===================================================================
--- cp/class.c (revision 272732)
+++ cp/class.c (working copy)
@@ -6395,6 +6395,7 @@ layout_class_type (tree t, tree *virtual
SET_TYPE_ALIGN (base_t, rli->record_align);
TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t);
TYPE_TYPELESS_STORAGE (base_t) = TYPE_TYPELESS_STORAGE (t);
+ TYPE_CXX_ODR_P (base_t) = TYPE_CXX_ODR_P (t);
/* Copy the non-static data members of T. This will include its
direct non-virtual bases & vtable. */
Index: ipa-devirt.c
===================================================================
--- ipa-devirt.c (revision 272732)
+++ ipa-devirt.c (working copy)
@@ -213,6 +213,8 @@ struct GTY(()) odr_type_d
bool odr_violated;
/* Set when virtual table without RTTI previaled table with. */
bool rtti_broken;
+ /* Set when the canonical type is determined using the type name. */
+ bool tbaa_enabled;
};
/* Return TRUE if all derived types of T are known and thus
@@ -1610,6 +1612,9 @@ add_type_duplicate (odr_type val, tree t
val->types_set->add (type);
+ if (!odr_hash)
+ return NULL;
+
gcc_checking_assert (can_be_name_hashed_p (type)
&& can_be_name_hashed_p (val->type));
@@ -1989,6 +1994,46 @@ prevailing_odr_type (tree type)
return t->type;
}
+/* Set tbaa_enabled flag for TYPE. */
+
+void
+enable_odr_based_tbaa (tree type)
+{
+ odr_type t = get_odr_type (type, true);
+ t->tbaa_enabled = true;
+}
+
+/* True if canonical type of TYPE is determined using ODR name. */
+
+bool
+odr_based_tbaa_p (const_tree type)
+{
+ if (!RECORD_OR_UNION_TYPE_P (type))
+ return false;
+ odr_type t = get_odr_type (const_cast <tree> (type), false);
+ if (!t || !t->tbaa_enabled)
+ return false;
+ return true;
+}
+
+/* Set TYPE_CANONICAL of type and all its variants and duplicates
+ to CANONICAL. */
+
+void
+set_type_canonical_for_odr_type (tree type, tree canonical)
+{
+ odr_type t = get_odr_type (type, false);
+ unsigned int i;
+ tree tt;
+
+ for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2))
+ TYPE_CANONICAL (t2) = canonical;
+ if (t->types)
+ FOR_EACH_VEC_ELT (*t->types, i, tt)
+ for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2))
+ TYPE_CANONICAL (t2) = canonical;
+}
+
/* Return true if we reported some ODR violation on TYPE. */
bool
Index: ipa-utils.h
===================================================================
--- ipa-utils.h (revision 272732)
+++ ipa-utils.h (working copy)
@@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t
bool odr_types_equivalent_p (tree type1, tree type2);
bool odr_type_violation_reported_p (tree type);
tree prevailing_odr_type (tree type);
+void enable_odr_based_tbaa (tree type);
+bool odr_based_tbaa_p (const_tree type);
+void set_type_canonical_for_odr_type (tree type, tree canonical);
/* Return vector containing possible targets of polymorphic call E.
If COMPLETEP is non-NULL, store true if the list is complete.
Index: lto/lto-common.c
===================================================================
--- lto/lto-common.c (revision 272732)
+++ lto/lto-common.c (working copy)
@@ -1,5 +1,5 @@
/* Top-level LTO routines.
- Copyright (C) 2009-2018 Free Software Foundation, Inc.
+ Copyright (C) 2009-2019 Free Software Foundation, Inc.
Contributed by CodeSourcery, Inc.
This file is part of GCC.
@@ -56,6 +56,11 @@ along with GCC; see the file COPYING3.
#include "attribs.h"
#include "builtins.h"
#include "lto-common.h"
+#include "tree-pretty-print.h"
+
+/* True when no new types are going to be streamd from the global stream. */
+
+static bool type_streaming_finished = false;
GTY(()) tree first_personality_decl;
@@ -217,9 +222,14 @@ static hash_map<const_tree, hashval_t> *
static unsigned long num_canonical_type_hash_entries;
static unsigned long num_canonical_type_hash_queries;
+/* Types postponed for registration to the canonical type table.
+ During streaming we postpone all TYPE_CXX_ODR_P types so we can alter
+ decide whether there is conflict with non-ODR type or not. */
+static GTY(()) vec<tree, va_gc> *types_to_register = NULL;
+
static void iterative_hash_canonical_type (tree type, inchash::hash &hstate);
static hashval_t gimple_canonical_type_hash (const void *p);
-static void gimple_register_canonical_type_1 (tree t, hashval_t hash);
+static hashval_t gimple_register_canonical_type_1 (tree t, hashval_t hash);
/* Returning a hash value for gimple type TYPE.
@@ -357,7 +367,7 @@ iterative_hash_canonical_type (tree type
optimal order. To avoid quadratic behavior also register the
type here. */
v = hash_canonical_type (type);
- gimple_register_canonical_type_1 (type, v);
+ v = gimple_register_canonical_type_1 (type, v);
}
hstate.add_int (v);
}
@@ -388,7 +398,7 @@ gimple_canonical_type_eq (const void *p1
/* Main worker for gimple_register_canonical_type. */
-static void
+static hashval_t
gimple_register_canonical_type_1 (tree t, hashval_t hash)
{
void **slot;
@@ -397,6 +407,72 @@ gimple_register_canonical_type_1 (tree t
&& type_with_alias_set_p (t)
&& canonical_type_used_p (t));
+ /* ODR types for which there is no ODR violation and we did not record
+ structurally equivalent non-ODR type can be treated as unique by their
+ name.
+
+ hash passed to gimple_register_canonical_type_1 is a structural hash
+ that we can use to lookup structurally equivalent non-ODR type.
+ In case we decide to treat type as unique ODR type we recompute hash based
+ on name and let TBAA machinery know about our decision. */
+ if (RECORD_OR_UNION_TYPE_P (t)
+ && odr_type_p (t) && !odr_type_violation_reported_p (t))
+ {
+ /* Here we rely on fact that all non-ODR types was inserted into
+ canonical type hash and thus we can safely detect conflicts between
+ ODR types and interoperable non-ODR types. */
+ gcc_checking_assert (type_streaming_finished
+ && TYPE_MAIN_VARIANT (t) == t);
+ slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash,
+ NO_INSERT);
+ if (slot && !TYPE_CXX_ODR_P (*(tree *)slot))
+ {
+ tree nonodr = *(tree *)slot;
+ if (symtab->dump_file)
+ {
+ fprintf (symtab->dump_file,
+ "ODR and non-ODR type conflict: ");
+ print_generic_expr (symtab->dump_file, t);
+ fprintf (symtab->dump_file, " and ");
+ print_generic_expr (symtab->dump_file, nonodr);
+ fprintf (symtab->dump_file, " mangled:%s\n",
+ IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME (TYPE_NAME (t))));
+ }
+ /* Set canonical for T and all other ODR equivalent duplicates
+ including incomplete structures. */
+ set_type_canonical_for_odr_type (t, nonodr);
+ }
+ else
+ {
+ tree prevail = prevailing_odr_type (t);
+
+ if (symtab->dump_file)
+ {
+ fprintf (symtab->dump_file,
+ "New canonical ODR type: ");
+ print_generic_expr (symtab->dump_file, t);
+ fprintf (symtab->dump_file, " mangled:%s\n",
+ IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME (TYPE_NAME (t))));
+ }
+ /* Set canonical for T and all other ODR equivalent duplicates
+ including incomplete structures. */
+ set_type_canonical_for_odr_type (t, prevail);
+ enable_odr_based_tbaa (t);
+ if (!type_in_anonymous_namespace_p (t))
+ hash = htab_hash_string (IDENTIFIER_POINTER
+ (DECL_ASSEMBLER_NAME
+ (TYPE_NAME (t))));
+ else
+ hash = TYPE_UID (t);
+ num_canonical_type_hash_entries++;
+ bool existed_p = canonical_type_hash_cache->put (prevail, hash);
+ gcc_checking_assert (!existed_p);
+ }
+ return hash;
+ }
+
slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT);
if (*slot)
{
@@ -413,6 +489,7 @@ gimple_register_canonical_type_1 (tree t
bool existed_p = canonical_type_hash_cache->put (t, hash);
gcc_assert (!existed_p);
}
+ return hash;
}
/* Register type T in the global type table gimple_types and set
@@ -464,6 +541,34 @@ lto_register_canonical_types (tree node,
gimple_register_canonical_type (node);
}
+/* Finish canonical type calculation: after all units has been streamed in we
+ can check if given ODR type structurally conflicts with a non-ODR type. In
+ the first case we set type canonical according to the canonical type hash.
+ In the second case we use type names. */
+
+static void
+lto_register_canonical_types_for_odr_types ()
+{
+ tree t;
+ unsigned int i;
+
+ if (!types_to_register)
+ return;
+
+ type_streaming_finished = true;
+
+ /* Be sure that no types derived from ODR types was
+ not inserted into the hash table. */
+ if (flag_checking)
+ FOR_EACH_VEC_ELT (*types_to_register, i, t)
+ gcc_assert (!TYPE_CANONICAL (t));
+
+ /* Register all remaining types. */
+ FOR_EACH_VEC_ELT (*types_to_register, i, t)
+ if (!TYPE_CANONICAL (t))
+ gimple_register_canonical_type (t);
+}
+
/* Remember trees that contains references to declarations. */
vec <tree, va_gc> *tree_with_vars;
@@ -1657,6 +1762,7 @@ unify_scc (struct data_in *data_in, unsi
}
+
/* Read all the symbols from buffer DATA, using descriptors in DECL_DATA.
RESOLUTIONS is the set of symbols picked by the linker (read from the
resolution file when the linker plugin is being used). */
@@ -1749,12 +1855,23 @@ lto_read_decls (struct lto_file_decl_dat
num_prevailing_types++;
lto_fixup_prevailing_type (t);
- /* Compute the canonical type of all types.
+ /* Compute the canonical type of all non-ODR types.
+ Delay ODR types for the end of merging process - the canonical
+ type for those can be computed using the (unique) name however
+ we want to do this only if units in other languages do not
+ contain structurally equivalent type.
+
Because SCC components are streamed in random (hash) order
we may have encountered the type before while registering
type canonical of a derived type in the same SCC. */
if (!TYPE_CANONICAL (t))
- gimple_register_canonical_type (t);
+ {
+ if (!RECORD_OR_UNION_TYPE_P (t)
+ || !TYPE_CXX_ODR_P (t))
+ gimple_register_canonical_type (t);
+ else if (COMPLETE_TYPE_P (t))
+ vec_safe_push (types_to_register, t);
+ }
if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t))
register_odr_type (t);
}
@@ -2605,6 +2722,8 @@ read_cgraph_and_symbols (unsigned nfiles
ggc_free(decl_data);
real_file_decl_data = NULL;
+ lto_register_canonical_types_for_odr_types ();
+
if (resolution_file_name)
fclose (resolution);
Index: tree.c
===================================================================
--- tree.c (revision 272732)
+++ tree.c (working copy)
@@ -14103,6 +14103,7 @@ gimple_canonical_types_compatible_p (con
gcc_assert (!trust_type_canonical
|| (type_with_alias_set_p (t1) && type_with_alias_set_p (t2)));
+
/* If the types have been previously registered and found equal
they still are. */
@@ -14120,6 +14121,14 @@ gimple_canonical_types_compatible_p (con
return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2);
}
+ /* For types where we do ODR based TBAA the canonical type is always
+ set correctly, so we know that types are different if their
+ canonical types does not match. */
+ if (trust_type_canonical
+ && (odr_type_p (t1) && odr_based_tbaa_p (t1))
+ != (odr_type_p (t2) && odr_based_tbaa_p (t2)))
+ return false;
+
/* Can't be the same type if the types don't have the same code. */
enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1));
if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2)))
Index: testsuite/g++.dg/lto/alias-2_0.C
===================================================================
--- testsuite/g++.dg/lto/alias-2_0.C (nonexistent)
+++ testsuite/g++.dg/lto/alias-2_0.C (working copy)
@@ -0,0 +1,31 @@
+/* { dg-lto-do run } */
+/* { dg-lto-options { { -O2 -flto } } } */
+
+/* With LTO we consider all pointers to incomplete types to be possibly
+ aliasing. This makes *bptr to alias with aptr.
+ For C++ ODR types we however can work out that they are actually
+ different. */
+
+#include <string.h>
+
+typedef int (*fnptr) ();
+
+__attribute__ ((used))
+struct a *aptr;
+
+__attribute__ ((used))
+struct b **bptr = (struct b**)&aptr;
+extern void init ();
+extern void inline_me_late (int);
+
+
+int
+main (int argc, char **argv)
+{
+ init ();
+ aptr = 0;
+ inline_me_late (argc);
+ if (!__builtin_constant_p (aptr == 0))
+ __builtin_abort ();
+ return (size_t)aptr;
+}
Index: testsuite/g++.dg/lto/alias-2_1.C
===================================================================
--- testsuite/g++.dg/lto/alias-2_1.C (nonexistent)
+++ testsuite/g++.dg/lto/alias-2_1.C (working copy)
@@ -0,0 +1,16 @@
+#include <string.h>
+struct a {int a;} a;
+struct b {int b;} b;
+extern struct b **bptr;
+void
+inline_me_late (int argc)
+{
+ if (argc == -1)
+ *bptr = (struct b *)(size_t)1;
+}
+void
+init()
+{
+ a.a=1;
+ b.b=2;
+}
On Thu, 27 Jun 2019, Jan Hubicka wrote: > Hi, > here is update patch I am re-testing. Ok if it suceeds? + if (!type_in_anonymous_namespace_p (t)) + hash = htab_hash_string (IDENTIFIER_POINTER + (DECL_ASSEMBLER_NAME + (TYPE_NAME (t)))); + else + hash = TYPE_UID (t); + num_canonical_type_hash_entries++; + bool existed_p = canonical_type_hash_cache->put (prevail, hash); but you still cache the hash for prevail rather than t while we are eventually asking for the hash of t again? I suppose not since we just adjusted TYPE_CANONICAL and we're then supposed to pick the hash from the canonical type? I think this deserves a comment. And it should use 'prevail' then everywhere here for consistency, otherwise it really looks odd. > @@ -357,7 +367,7 @@ iterative_hash_canonical_type (tree type > optimal order. To avoid quadratic behavior also register the > type here. */ > v = hash_canonical_type (type); > - gimple_register_canonical_type_1 (type, v); > + v = gimple_register_canonical_type_1 (type, v); > } > hstate.add_int (v); > } Just noticed this should be hstate.merge_hash (v); results in the very same effect but is clearer IMHO. Richard. > Orace quary stats finished in meantime. > > Alias oracle query stats: > refs_may_alias_p: 39232255 disambiguations, 47436580 queries > ref_maybe_used_by_call_p: 59801 disambiguations, 39811399 queries > call_may_clobber_ref_p: 5967 disambiguations, 9009 queries > nonoverlapping_component_refs_p: 10184 disambiguations, 754231 queries > nonoverlapping_component_refs_of_decl_p: 6488 disambiguations, 214611 queries > aliasing_component_refs_p: 88820 disambiguations, 291741 queries > TBAA oracle: 11687911 disambiguations 34188770 queries > 13343414 are in alias set 0 > 5417962 queries asked about the same object > 148 queries asked about the same alias set > 0 access volatile > 1672142 are dependent in the DAG > 2067193 are aritificially in conflict with void * > > PTA query stats: > pt_solution_includes: 502249 disambiguations, 7180660 queries > pt_solutions_intersect: 357826 disambiguations, 7059723 queries > > So there is no big change in TBAA effectivity compared to previous patch. > There is however nice improvements in aliasing_component_refs_p > 61523 -> 88820 which I think is due to Jason's change > > Honza > > Index: cp/class.c > =================================================================== > --- cp/class.c (revision 272732) > +++ cp/class.c (working copy) > @@ -6395,6 +6395,7 @@ layout_class_type (tree t, tree *virtual > SET_TYPE_ALIGN (base_t, rli->record_align); > TYPE_USER_ALIGN (base_t) = TYPE_USER_ALIGN (t); > TYPE_TYPELESS_STORAGE (base_t) = TYPE_TYPELESS_STORAGE (t); > + TYPE_CXX_ODR_P (base_t) = TYPE_CXX_ODR_P (t); > > /* Copy the non-static data members of T. This will include its > direct non-virtual bases & vtable. */ > Index: ipa-devirt.c > =================================================================== > --- ipa-devirt.c (revision 272732) > +++ ipa-devirt.c (working copy) > @@ -213,6 +213,8 @@ struct GTY(()) odr_type_d > bool odr_violated; > /* Set when virtual table without RTTI previaled table with. */ > bool rtti_broken; > + /* Set when the canonical type is determined using the type name. */ > + bool tbaa_enabled; > }; > > /* Return TRUE if all derived types of T are known and thus > @@ -1610,6 +1612,9 @@ add_type_duplicate (odr_type val, tree t > > val->types_set->add (type); > > + if (!odr_hash) > + return NULL; > + > gcc_checking_assert (can_be_name_hashed_p (type) > && can_be_name_hashed_p (val->type)); > > @@ -1989,6 +1994,46 @@ prevailing_odr_type (tree type) > return t->type; > } > > +/* Set tbaa_enabled flag for TYPE. */ > + > +void > +enable_odr_based_tbaa (tree type) > +{ > + odr_type t = get_odr_type (type, true); > + t->tbaa_enabled = true; > +} > + > +/* True if canonical type of TYPE is determined using ODR name. */ > + > +bool > +odr_based_tbaa_p (const_tree type) > +{ > + if (!RECORD_OR_UNION_TYPE_P (type)) > + return false; > + odr_type t = get_odr_type (const_cast <tree> (type), false); > + if (!t || !t->tbaa_enabled) > + return false; > + return true; > +} > + > +/* Set TYPE_CANONICAL of type and all its variants and duplicates > + to CANONICAL. */ > + > +void > +set_type_canonical_for_odr_type (tree type, tree canonical) > +{ > + odr_type t = get_odr_type (type, false); > + unsigned int i; > + tree tt; > + > + for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > + if (t->types) > + FOR_EACH_VEC_ELT (*t->types, i, tt) > + for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2)) > + TYPE_CANONICAL (t2) = canonical; > +} > + > /* Return true if we reported some ODR violation on TYPE. */ > > bool > Index: ipa-utils.h > =================================================================== > --- ipa-utils.h (revision 272732) > +++ ipa-utils.h (working copy) > @@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t > bool odr_types_equivalent_p (tree type1, tree type2); > bool odr_type_violation_reported_p (tree type); > tree prevailing_odr_type (tree type); > +void enable_odr_based_tbaa (tree type); > +bool odr_based_tbaa_p (const_tree type); > +void set_type_canonical_for_odr_type (tree type, tree canonical); > > /* Return vector containing possible targets of polymorphic call E. > If COMPLETEP is non-NULL, store true if the list is complete. > Index: lto/lto-common.c > =================================================================== > --- lto/lto-common.c (revision 272732) > +++ lto/lto-common.c (working copy) > @@ -1,5 +1,5 @@ > /* Top-level LTO routines. > - Copyright (C) 2009-2018 Free Software Foundation, Inc. > + Copyright (C) 2009-2019 Free Software Foundation, Inc. > Contributed by CodeSourcery, Inc. > > This file is part of GCC. > @@ -56,6 +56,11 @@ along with GCC; see the file COPYING3. > #include "attribs.h" > #include "builtins.h" > #include "lto-common.h" > +#include "tree-pretty-print.h" > + > +/* True when no new types are going to be streamd from the global stream. */ > + > +static bool type_streaming_finished = false; > > GTY(()) tree first_personality_decl; > > @@ -217,9 +222,14 @@ static hash_map<const_tree, hashval_t> * > static unsigned long num_canonical_type_hash_entries; > static unsigned long num_canonical_type_hash_queries; > > +/* Types postponed for registration to the canonical type table. > + During streaming we postpone all TYPE_CXX_ODR_P types so we can alter > + decide whether there is conflict with non-ODR type or not. */ > +static GTY(()) vec<tree, va_gc> *types_to_register = NULL; > + > static void iterative_hash_canonical_type (tree type, inchash::hash &hstate); > static hashval_t gimple_canonical_type_hash (const void *p); > -static void gimple_register_canonical_type_1 (tree t, hashval_t hash); > +static hashval_t gimple_register_canonical_type_1 (tree t, hashval_t hash); > > /* Returning a hash value for gimple type TYPE. > > @@ -357,7 +367,7 @@ iterative_hash_canonical_type (tree type > optimal order. To avoid quadratic behavior also register the > type here. */ > v = hash_canonical_type (type); > - gimple_register_canonical_type_1 (type, v); > + v = gimple_register_canonical_type_1 (type, v); > } > hstate.add_int (v); > } > @@ -388,7 +398,7 @@ gimple_canonical_type_eq (const void *p1 > > /* Main worker for gimple_register_canonical_type. */ > > -static void > +static hashval_t > gimple_register_canonical_type_1 (tree t, hashval_t hash) > { > void **slot; > @@ -397,6 +407,72 @@ gimple_register_canonical_type_1 (tree t > && type_with_alias_set_p (t) > && canonical_type_used_p (t)); > > + /* ODR types for which there is no ODR violation and we did not record > + structurally equivalent non-ODR type can be treated as unique by their > + name. > + > + hash passed to gimple_register_canonical_type_1 is a structural hash > + that we can use to lookup structurally equivalent non-ODR type. > + In case we decide to treat type as unique ODR type we recompute hash based > + on name and let TBAA machinery know about our decision. */ > + if (RECORD_OR_UNION_TYPE_P (t) > + && odr_type_p (t) && !odr_type_violation_reported_p (t)) > + { > + /* Here we rely on fact that all non-ODR types was inserted into > + canonical type hash and thus we can safely detect conflicts between > + ODR types and interoperable non-ODR types. */ > + gcc_checking_assert (type_streaming_finished > + && TYPE_MAIN_VARIANT (t) == t); > + slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, > + NO_INSERT); > + if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) > + { > + tree nonodr = *(tree *)slot; > + if (symtab->dump_file) > + { > + fprintf (symtab->dump_file, > + "ODR and non-ODR type conflict: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, " and "); > + print_generic_expr (symtab->dump_file, nonodr); > + fprintf (symtab->dump_file, " mangled:%s\n", > + IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))); > + } > + /* Set canonical for T and all other ODR equivalent duplicates > + including incomplete structures. */ > + set_type_canonical_for_odr_type (t, nonodr); > + } > + else > + { > + tree prevail = prevailing_odr_type (t); > + > + if (symtab->dump_file) > + { > + fprintf (symtab->dump_file, > + "New canonical ODR type: "); > + print_generic_expr (symtab->dump_file, t); > + fprintf (symtab->dump_file, " mangled:%s\n", > + IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))); > + } > + /* Set canonical for T and all other ODR equivalent duplicates > + including incomplete structures. */ > + set_type_canonical_for_odr_type (t, prevail); > + enable_odr_based_tbaa (t); > + if (!type_in_anonymous_namespace_p (t)) > + hash = htab_hash_string (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME > + (TYPE_NAME (t)))); > + else > + hash = TYPE_UID (t); > + num_canonical_type_hash_entries++; > + bool existed_p = canonical_type_hash_cache->put (prevail, hash); > + gcc_checking_assert (!existed_p); > + } > + return hash; > + } > + > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > { > @@ -413,6 +489,7 @@ gimple_register_canonical_type_1 (tree t > bool existed_p = canonical_type_hash_cache->put (t, hash); > gcc_assert (!existed_p); > } > + return hash; > } > > /* Register type T in the global type table gimple_types and set > @@ -464,6 +541,34 @@ lto_register_canonical_types (tree node, > gimple_register_canonical_type (node); > } > > +/* Finish canonical type calculation: after all units has been streamed in we > + can check if given ODR type structurally conflicts with a non-ODR type. In > + the first case we set type canonical according to the canonical type hash. > + In the second case we use type names. */ > + > +static void > +lto_register_canonical_types_for_odr_types () > +{ > + tree t; > + unsigned int i; > + > + if (!types_to_register) > + return; > + > + type_streaming_finished = true; > + > + /* Be sure that no types derived from ODR types was > + not inserted into the hash table. */ > + if (flag_checking) > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + gcc_assert (!TYPE_CANONICAL (t)); > + > + /* Register all remaining types. */ > + FOR_EACH_VEC_ELT (*types_to_register, i, t) > + if (!TYPE_CANONICAL (t)) > + gimple_register_canonical_type (t); > +} > + > > /* Remember trees that contains references to declarations. */ > vec <tree, va_gc> *tree_with_vars; > @@ -1657,6 +1762,7 @@ unify_scc (struct data_in *data_in, unsi > } > > > + > /* Read all the symbols from buffer DATA, using descriptors in DECL_DATA. > RESOLUTIONS is the set of symbols picked by the linker (read from the > resolution file when the linker plugin is being used). */ > @@ -1749,12 +1855,23 @@ lto_read_decls (struct lto_file_decl_dat > num_prevailing_types++; > lto_fixup_prevailing_type (t); > > - /* Compute the canonical type of all types. > + /* Compute the canonical type of all non-ODR types. > + Delay ODR types for the end of merging process - the canonical > + type for those can be computed using the (unique) name however > + we want to do this only if units in other languages do not > + contain structurally equivalent type. > + > Because SCC components are streamed in random (hash) order > we may have encountered the type before while registering > type canonical of a derived type in the same SCC. */ > if (!TYPE_CANONICAL (t)) > - gimple_register_canonical_type (t); > + { > + if (!RECORD_OR_UNION_TYPE_P (t) > + || !TYPE_CXX_ODR_P (t)) > + gimple_register_canonical_type (t); > + else if (COMPLETE_TYPE_P (t)) > + vec_safe_push (types_to_register, t); > + } > if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t)) > register_odr_type (t); > } > @@ -2605,6 +2722,8 @@ read_cgraph_and_symbols (unsigned nfiles > ggc_free(decl_data); > real_file_decl_data = NULL; > > + lto_register_canonical_types_for_odr_types (); > + > if (resolution_file_name) > fclose (resolution); > > Index: tree.c > =================================================================== > --- tree.c (revision 272732) > +++ tree.c (working copy) > @@ -14103,6 +14103,7 @@ gimple_canonical_types_compatible_p (con > > gcc_assert (!trust_type_canonical > || (type_with_alias_set_p (t1) && type_with_alias_set_p (t2))); > + > /* If the types have been previously registered and found equal > they still are. */ > > @@ -14120,6 +14121,14 @@ gimple_canonical_types_compatible_p (con > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > } > > + /* For types where we do ODR based TBAA the canonical type is always > + set correctly, so we know that types are different if their > + canonical types does not match. */ > + if (trust_type_canonical > + && (odr_type_p (t1) && odr_based_tbaa_p (t1)) > + != (odr_type_p (t2) && odr_based_tbaa_p (t2))) > + return false; > + > /* Can't be the same type if the types don't have the same code. */ > enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1)); > if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2))) > Index: testsuite/g++.dg/lto/alias-2_0.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_0.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_0.C (working copy) > @@ -0,0 +1,31 @@ > +/* { dg-lto-do run } */ > +/* { dg-lto-options { { -O2 -flto } } } */ > + > +/* With LTO we consider all pointers to incomplete types to be possibly > + aliasing. This makes *bptr to alias with aptr. > + For C++ ODR types we however can work out that they are actually > + different. */ > + > +#include <string.h> > + > +typedef int (*fnptr) (); > + > +__attribute__ ((used)) > +struct a *aptr; > + > +__attribute__ ((used)) > +struct b **bptr = (struct b**)&aptr; > +extern void init (); > +extern void inline_me_late (int); > + > + > +int > +main (int argc, char **argv) > +{ > + init (); > + aptr = 0; > + inline_me_late (argc); > + if (!__builtin_constant_p (aptr == 0)) > + __builtin_abort (); > + return (size_t)aptr; > +} > Index: testsuite/g++.dg/lto/alias-2_1.C > =================================================================== > --- testsuite/g++.dg/lto/alias-2_1.C (nonexistent) > +++ testsuite/g++.dg/lto/alias-2_1.C (working copy) > @@ -0,0 +1,16 @@ > +#include <string.h> > +struct a {int a;} a; > +struct b {int b;} b; > +extern struct b **bptr; > +void > +inline_me_late (int argc) > +{ > + if (argc == -1) > + *bptr = (struct b *)(size_t)1; > +} > +void > +init() > +{ > + a.a=1; > + b.b=2; > +} >
> On Thu, 27 Jun 2019, Jan Hubicka wrote: > > > Hi, > > here is update patch I am re-testing. Ok if it suceeds? > > + if (!type_in_anonymous_namespace_p (t)) > + hash = htab_hash_string (IDENTIFIER_POINTER > + (DECL_ASSEMBLER_NAME > + (TYPE_NAME (t)))); > + else > + hash = TYPE_UID (t); > + num_canonical_type_hash_entries++; > + bool existed_p = canonical_type_hash_cache->put (prevail, hash); > > but you still cache the hash for prevail rather than t while we > are eventually asking for the hash of t again? I suppose not since > we just adjusted TYPE_CANONICAL and we're then supposed to pick > the hash from the canonical type? I think this deserves a comment. > And it should use 'prevail' then everywhere here for consistency, > otherwise it really looks odd. Yes, all ODR variants will get prevail as their TYPE_CANONICAL and therefore we need to cache its hash. I will add comment. > > > @@ -357,7 +367,7 @@ iterative_hash_canonical_type (tree type > > optimal order. To avoid quadratic behavior also register the > > type here. */ > > v = hash_canonical_type (type); > > - gimple_register_canonical_type_1 (type, v); > > + v = gimple_register_canonical_type_1 (type, v); > > } > > hstate.add_int (v); > > } > > Just noticed this should be > > hstate.merge_hash (v); > > results in the very same effect but is clearer IMHO. OK, will fix that too, re-test and commit. Thanks! Honza
No. Since this is LTO, it's very hard to simplify the big application. Sorry for that. I think Christophe is mentioning the case from g++.dg is reporting the similar issue like " type variant differs by TYPE_CXX_ODR_P ", right? Thanks, -Jiangning > -----Original Message----- > From: Jan Hubicka <hubicka@ucw.cz> > Sent: Thursday, June 27, 2019 2:29 PM > To: JiangNing OS <jiangning@os.amperecomputing.com> > Cc: Eric Botcazou <ebotcazou@adacore.com>; Christophe Lyon > <christophe.lyon@linaro.org>; gcc Patches <gcc-patches@gcc.gnu.org>; > Richard Biener <rguenther@suse.de>; d@dcepelik.cz; Martin Liška > <mliska@suse.cz> > Subject: Re: Use ODR for canonical types construction in LTO > > > Hi, > > > > This commit > > https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=272628 is > > breaking trunk LTO on some real benchmarks, so can it be fixed or > > reverted? For example, > > Do you have a testcase? > Honza > > > > lto1: error: type variant differs by TYPE_CXX_ODR_P <record_type > > 0xffff99943d08 __va_list BLK > > size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 > bitsizetype> constant 256> > > unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 > sizetype> constant 32> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0xffff99943d08 > > fields <field_decl 0xffff99960130 __stack > > type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void> > > public unsigned DI > > size <integer_cst 0xffff99890c00 constant 64> > > unit-size <integer_cst 0xffff99890c18 constant 8> > > align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality > > pointer_to_this <pointer_type 0xffff99b5d740>> > > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size > <integer_cst 0xffff99890c18 8> > > align:64 warn_if_not_align:0 offset_align 128 > > offset <integer_cst 0xffff99890c30 constant 0> > > bit-offset <integer_cst 0xffff99890c78 constant 0> context > <record_type 0xffff99943d08 __va_list> > > chain <field_decl 0xffff999601c8 __gr_top type <pointer_type > 0xffff99940fc0> > > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit- > size <integer_cst 0xffff99890c18 8> > > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst > 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context > <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 > __vr_top>>> > > reference_to_this <reference_type 0xffff99943fa8> chain <type_decl > > 0xffff99960098 __va_list>> <record_type 0xffff99c59cd8 va_list cxx-odr-p > BLK > > size <integer_cst 0xffff99890f60 type <integer_type 0xffff999400a8 > bitsizetype> constant 256> > > unit-size <integer_cst 0xffff99891050 type <integer_type 0xffff99940000 > sizetype> constant 32> > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type > 0xffff99943d08 > > fields <field_decl 0xffff99960130 __stack > > type <pointer_type 0xffff99940fc0 type <void_type 0xffff99940f18 void> > > public unsigned DI > > size <integer_cst 0xffff99890c00 constant 64> > > unit-size <integer_cst 0xffff99890c18 constant 8> > > align:64 warn_if_not_align:0 symtab:0 alias-set 7 structural-equality > > pointer_to_this <pointer_type 0xffff99b5d740>> > > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit-size > <integer_cst 0xffff99890c18 8> > > align:64 warn_if_not_align:0 offset_align 128 > > offset <integer_cst 0xffff99890c30 constant 0> > > bit-offset <integer_cst 0xffff99890c78 constant 0> context > <record_type 0xffff99943d08 __va_list> > > chain <field_decl 0xffff999601c8 __gr_top type <pointer_type > 0xffff99940fc0> > > unsigned DI <built-in>:0:0 size <integer_cst 0xffff99890c00 64> unit- > size <integer_cst 0xffff99890c18 8> > > align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst > 0xffff99890c30 0> bit-offset <integer_cst 0xffff99890c00 64> context > <record_type 0xffff99943d08 __va_list> chain <field_decl 0xffff99960260 > __vr_top>>> > > pointer_to_this <pointer_type 0xffff99c59e28> reference_to_this > > <reference_type 0xffff9a5b0d20>> > > lto1: internal compiler error: 'verify_type' failed > > 0xe33e93 verify_type(tree_node const*) > > ../../gcc/gcc/tree.c:14655 > > 0x5efc4b lto_fixup_state > > ../../gcc/gcc/lto/lto-common.c:2429 > > 0x5fc01b lto_fixup_decls > > ../../gcc/gcc/lto/lto-common.c:2460 > > 0x5fc01b read_cgraph_and_symbols(unsigned int, char const**) > > ../../gcc/gcc/lto/lto-common.c:2693 > > 0x5ded23 lto_main() > > ../../gcc/gcc/lto/lto.c:616 > > Please submit a full bug report, > > with preprocessed source if appropriate. > > Please include the complete backtrace with any bug report. > > See <https://gcc.gnu.org/bugs/> for instructions. > > lto-wrapper: fatal error: /home/amptest/gcc/install_last//bin/g++ > > returned 1 exit status compilation terminated. > > /usr/bin/ld: error: lto-wrapper failed > > collect2: error: ld returned 1 exit status > > > > Thanks, > > -Jiangning > > > > > -----Original Message----- > > > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> > > > On Behalf Of Christophe Lyon > > > Sent: Tuesday, June 25, 2019 8:30 PM > > > To: Jan Hubicka <hubicka@ucw.cz> > > > Cc: Eric Botcazou <ebotcazou@adacore.com>; gcc Patches <gcc- > > > patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>; > > > d@dcepelik.cz; Martin Liška <mliska@suse.cz> > > > Subject: Re: Use ODR for canonical types construction in LTO > > > > > > Hi, > > > > > > > > > On Tue, 25 Jun 2019 at 10:20, Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > > > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Check that > > > > > > type is array or integer prior checking string flag. > > > > > > > > > > The test for array is superfluous here. > > > > > > > > > > > * gcc-interface/gigi.h (gnat_signed_type_for, > > > > > > maybe_character_value): Likewise. > > > > > > > > > > Wrong ChangeLog, the first modified function is > maybe_character_type. > > > > > > > > > > I have installed the attached patchlet after testing it on x86-64/Linux. > > > > > > > > > > > > > > > * gcc-interface/decl.c (gnat_to_gnu_entity): Remove > > > > > superfluous test > > > in > > > > > previous change. > > > > > * gcc-interface/gigi.h (maybe_character_type): Fix formatting. > > > > > (maybe_character_value): Likewise. > > > > > > > > Thanks a lot. I was not quite sure if ARRAY_TYPEs can happen there > > > > and I should have added you to the CC. > > > > > > > > > > After the main commit (r272628), I have noticed regressions on arm > > > and > > > aarch64: > > > > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O0 -flto > > > -flto-partition=1to1 -fno-use-linker-plugin (internal compiler > > > error) > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O0 -flto -flto-partition=none -fuse-linker-plugin (internal > > > compiler > > > error) > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O0 -flto -fuse-linker-plugin -fno-fat-lto-objects (internal > > > compiler > > > error) > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O2 -flto > > > -flto-partition=1to1 -fno-use-linker-plugin (internal compiler > > > error) > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O2 -flto -flto-partition=none -fuse-linker-plugin > > > -fno-fat-lto-objects (internal compiler > > > error) > > > g++.dg/lto/pr60336 cp_lto_pr60336_0.o-cp_lto_pr60336_0.o link, > > > -O2 -flto -fuse-linker-plugin (internal compiler error) > > > g++.dg/torture/pr45843.C -O2 -flto -fno-use-linker-plugin > > > -flto-partition=none (internal compiler error) > > > g++.dg/torture/pr45843.C -O2 -flto -fuse-linker-plugin > > > -fno-fat-lto-objects (internal compiler error) > > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > > -fno-use-linker-plugin -flto-partition=none (internal compiler error) > > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > > -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler > > > error) > > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > > -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) > > > g++.dg/torture/stackalign/eh-vararg-1.C -O2 -flto > > > -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler > > > error) > > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > > -fno-use-linker-plugin -flto-partition=none (internal compiler error) > > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > > -fno-use-linker-plugin -flto-partition=none -fpic (internal compiler > > > error) > > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > > -fuse-linker-plugin -fno-fat-lto-objects (internal compiler error) > > > g++.dg/torture/stackalign/eh-vararg-2.C -O2 -flto > > > -fuse-linker-plugin -fno-fat-lto-objects -fpic (internal compiler > > > error) > > > > > > A sample ICE: > > > lto1: error: type variant differs by TYPE_CXX_ODR_P <record_type > > > 0x2b3d78275dc8 __va_list BLK > > > size <integer_cst 0x2b3d7825cf60 type <integer_type > > > 0x2b3d782690a8 > > > bitsizetype> constant 256> > > > unit-size <integer_cst 0x2b3d78270060 type <integer_type > > > 0x2b3d78269000 sizetype> constant 32> > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 > > > canonical-type > > > 0x2b3d78275dc8 > > > fields <field_decl 0x2b3d78273130 __stack > > > type <pointer_type 0x2b3d78271000 type <void_type > > > 0x2b3d78269f18 > > > void> > > > public unsigned DI > > > size <integer_cst 0x2b3d7825cc00 constant 64> > > > unit-size <integer_cst 0x2b3d7825cc18 constant 8> > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural- > equality > > > pointer_to_this <pointer_type 0x2b3d782769d8>> > > > unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 > > > 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > > align:64 warn_if_not_align:0 offset_align 128 > > > offset <integer_cst 0x2b3d7825cc30 constant 0> > > > bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context > > > <record_type 0x2b3d78275dc8 __va_list> > > > chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type > > > 0x2b3d78271000> > > > unsigned DI <built-in>:0:0 size <integer_cst > > > 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > > align:64 warn_if_not_align:0 offset_align 128 offset > > > <integer_cst > > > 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 > > > 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl > > > 0x2b3d78273260 __vr_top>>> > > > reference_to_this <reference_type 0x2b3d782760a8> chain > > > <type_decl > > > 0x2b3d78273098 __va_list>> > > > <record_type 0x2b3d783f6930 va_list cxx-odr-p BLK > > > size <integer_cst 0x2b3d7825cf60 type <integer_type > > > 0x2b3d782690a8 > > > bitsizetype> constant 256> > > > unit-size <integer_cst 0x2b3d78270060 type <integer_type > > > 0x2b3d78269000 sizetype> constant 32> > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 > > > canonical-type > > > 0x2b3d78275dc8 > > > fields <field_decl 0x2b3d78273130 __stack > > > type <pointer_type 0x2b3d78271000 type <void_type > > > 0x2b3d78269f18 > > > void> > > > public unsigned DI > > > size <integer_cst 0x2b3d7825cc00 constant 64> > > > unit-size <integer_cst 0x2b3d7825cc18 constant 8> > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 structural- > equality > > > pointer_to_this <pointer_type 0x2b3d782769d8>> > > > unsigned DI <built-in>:0:0 size <integer_cst 0x2b3d7825cc00 > > > 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > > align:64 warn_if_not_align:0 offset_align 128 > > > offset <integer_cst 0x2b3d7825cc30 constant 0> > > > bit-offset <integer_cst 0x2b3d7825cc78 constant 0> context > > > <record_type 0x2b3d78275dc8 __va_list> > > > chain <field_decl 0x2b3d782731c8 __gr_top type <pointer_type > > > 0x2b3d78271000> > > > unsigned DI <built-in>:0:0 size <integer_cst > > > 0x2b3d7825cc00 64> unit-size <integer_cst 0x2b3d7825cc18 8> > > > align:64 warn_if_not_align:0 offset_align 128 offset > > > <integer_cst > > > 0x2b3d7825cc30 0> bit-offset <integer_cst 0x2b3d7825cc00 > > > 64> context <record_type 0x2b3d78275dc8 __va_list> chain <field_decl > > > 0x2b3d78273260 __vr_top>>> > > > pointer_to_this <pointer_type 0x2b3d783f69d8>> > > > lto1: internal compiler error: 'verify_type' failed > > > 0xe667b0 verify_type(tree_node const*) > > > /gcc/tree.c:14650 > > > 0x632cd7 lto_fixup_state > > > /gcc/lto/lto-common.c:2429 > > > 0x63f459 lto_fixup_decls > > > /gcc/lto/lto-common.c:2460 > > > 0x63f459 read_cgraph_and_symbols(unsigned int, char const**) > > > /gcc/lto/lto-common.c:2693 > > > 0x620fa2 lto_main() > > > /gcc/lto/lto.c:616 > > > Please submit a full bug report, > > > > > > Christophe
On Mon, Jun 24, 2019 at 1:46 PM Jan Hubicka <hubicka@ucw.cz> wrote: > > > > I thought I remembered someone's recent-ish work to treat specially > > types containing a char array, but I'm not finding it now. > > > > > For dynamically allocated memory as well as for stack space after stack > > > slot sharing done in cfgexpand I see this is necessary since we do not > > > preserve any information about placement new. > > > > Yes, untyped memory is different, but I'd think that memory allocated > > through (non-placement) new should also be considered typed. > > I will try to return to this once the code is cleaned up. It would > be quite interesting to make this better defined. > > > > > > I think this is what Richard refers to the code generating clobber > > > statements that is only leaking as-base types to the middle-end visible > > > part of IL and the code in call.c copying base structures. > > > > Right. Is there a better way we could express copying/clobbering only > > part of the object without involving the as-base types? > > I think currently two variants was discussed > 1) use the trik Richard proposed for class a containing fake > subclass a_as_base that has reduced size (i.e. is the AS_BASE type > we have now) and adjust all component_refs accordingly introducing > the view_convert_exprs for outer decls. > > Then clobbers and copies in call.c could use the as_base type. This is the approach I talked about in 22488, which would also fix the issue of fields with TYPE_SIZE larger than DECL_SIZE. > 2) expose IS_FAKE_BASE_TYPE to middle-end and teach TBAA machinery > about the fact that these are actually same types for everything > it cares about. > > We have similar hacks for Fortran commons already, but of couse > it is not most pretty (I outlined plan in previous mail) > > I would personally lean towards 2 since it will keep all component_refs > in its current natural form and because I know how to implement it :) > I guess it is Richi and yours call to pick variant which is better... Both of these you mention still involve the as-base type. I was wondering if there was a way to avoid that. > > > So at this time basically every C++ type can inter-operate with non-C++. > > > I was thinking of relaxing this somewhat but wanted to see if C++ > > > standard says something here. Things that may be sensible include: > > > 1) perhaps non-POD types especially those with vptr pointers do > > > not need to be inter-operable. > > > > PODs were intended to be the C-compatible subset, yes. > > > > > 2) anonymous namespace types > > > 3) types in namespace > > > > As long as these types don't have explicit language linkage (e.g. > > extern "C"), sure. > > Great, I will add those to my TODOs. > Do we have any way to tell language linkage from middle-end? Not with a flag, currently, but you could look at the mangled name to recognize extern "C". Jason
Hi,
this patch fixes the ICE. Problem is that va_arg type is pre-streamed
and thus at stream-in time we have main variant constructed by LTO FE
which is !CXX_ODR_P while vairants are ones comming from C++ FE which
are ODR. It is safe to drop the flags here since we only care about
main variants anyway.
I am testing the patch on aarch64 now.
Honza
Index: lto/lto-common.c
===================================================================
--- lto/lto-common.c (revision 272846)
+++ lto/lto-common.c (working copy)
@@ -568,8 +568,17 @@ lto_register_canonical_types_for_odr_typ
/* Register all remaining types. */
FOR_EACH_VEC_ELT (*types_to_register, i, t)
- if (!TYPE_CANONICAL (t))
- gimple_register_canonical_type (t);
+ {
+ /* For pre-streamed types like va-arg it is possible that main variant
+ is !CXX_ODR_P while the variant (which is streamed) is.
+ Copy CXX_ODR_P to make type verifier happy. This is safe because
+ in canonical type calculation we only consider main variants.
+ However we can not change this flag before streaming is finished
+ to not affect tree merging. */
+ TYPE_CXX_ODR_P (t) = TYPE_CXX_ODR_P (TYPE_MAIN_VARIANT (t));
+ if (!TYPE_CANONICAL (t))
+ gimple_register_canonical_type (t);
+ }
}
Hi, FYI. This patch works for my application LTO build on aarch64. Thanks, -Jiangning > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> > On Behalf Of Jan Hubicka > Sent: Monday, July 1, 2019 8:22 PM > To: Christophe Lyon <christophe.lyon@linaro.org> > Cc: Eric Botcazou <ebotcazou@adacore.com>; gcc Patches <gcc- > patches@gcc.gnu.org>; Richard Biener <rguenther@suse.de>; d@dcepelik.cz; > Martin Liška <mliska@suse.cz> > Subject: Re: Use ODR for canonical types construction in LTO > > Hi, > this patch fixes the ICE. Problem is that va_arg type is pre-streamed and thus > at stream-in time we have main variant constructed by LTO FE which > is !CXX_ODR_P while vairants are ones comming from C++ FE which are ODR. > It is safe to drop the flags here since we only care about main variants > anyway. > > I am testing the patch on aarch64 now. > > Honza > > Index: lto/lto-common.c > ================================================================ > === > --- lto/lto-common.c (revision 272846) > +++ lto/lto-common.c (working copy) > @@ -568,8 +568,17 @@ lto_register_canonical_types_for_odr_typ > > /* Register all remaining types. */ > FOR_EACH_VEC_ELT (*types_to_register, i, t) > - if (!TYPE_CANONICAL (t)) > - gimple_register_canonical_type (t); > + { > + /* For pre-streamed types like va-arg it is possible that main variant > + is !CXX_ODR_P while the variant (which is streamed) is. > + Copy CXX_ODR_P to make type verifier happy. This is safe because > + in canonical type calculation we only consider main variants. > + However we can not change this flag before streaming is finished > + to not affect tree merging. */ > + TYPE_CXX_ODR_P (t) = TYPE_CXX_ODR_P (TYPE_MAIN_VARIANT (t)); > + if (!TYPE_CANONICAL (t)) > + gimple_register_canonical_type (t); > + } > } > >
Hi. Honza can you please fix this: gcc/ipa-devirt.c:1616:12: warning: implicit conversion of NULL constant to 'bool' [-Wnull-conversion] Thanks, Martin
Index: alias.c =================================================================== --- alias.c (revision 271843) +++ alias.c (working copy) @@ -1202,47 +1202,52 @@ record_component_aliases (tree type) case RECORD_TYPE: case UNION_TYPE: case QUAL_UNION_TYPE: - for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field)) - if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field)) - { - /* LTO type merging does not make any difference between - component pointer types. We may have - - struct foo {int *a;}; - - as TYPE_CANONICAL of - - struct bar {float *a;}; - - Because accesses to int * and float * do not alias, we would get - false negative when accessing the same memory location by - float ** and bar *. We thus record the canonical type as: - - struct {void *a;}; - - void * is special cased and works as a universal pointer type. - Accesses to it conflicts with accesses to any other pointer - type. */ - tree t = TREE_TYPE (field); - if (in_lto_p) - { - /* VECTOR_TYPE and ARRAY_TYPE share the alias set with their - element type and that type has to be normalized to void *, - too, in the case it is a pointer. */ - while (!canonical_type_used_p (t) && !POINTER_TYPE_P (t)) - { - gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t)); - t = TREE_TYPE (t); - } - if (POINTER_TYPE_P (t)) - t = ptr_type_node; - else if (flag_checking) - gcc_checking_assert (get_alias_set (t) - == get_alias_set (TREE_TYPE (field))); - } - - record_alias_subset (superset, get_alias_set (t)); - } + { + /* LTO non-ODR type merging does not make any difference between + component pointer types. We may have + + struct foo {int *a;}; + + as TYPE_CANONICAL of + + struct bar {float *a;}; + + Because accesses to int * and float * do not alias, we would get + false negative when accessing the same memory location by + float ** and bar *. We thus record the canonical type as: + + struct {void *a;}; + + void * is special cased and works as a universal pointer type. + Accesses to it conflicts with accesses to any other pointer + type. */ + bool void_pointers = in_lto_p + && (!odr_type_p (type) + || !odr_based_tbaa_p (type)); + for (field = TYPE_FIELDS (type); field != 0; field = DECL_CHAIN (field)) + if (TREE_CODE (field) == FIELD_DECL && !DECL_NONADDRESSABLE_P (field)) + { + tree t = TREE_TYPE (field); + if (void_pointers) + { + /* VECTOR_TYPE and ARRAY_TYPE share the alias set with their + element type and that type has to be normalized to void *, + too, in the case it is a pointer. */ + while (!canonical_type_used_p (t) && !POINTER_TYPE_P (t)) + { + gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t)); + t = TREE_TYPE (t); + } + if (POINTER_TYPE_P (t)) + t = ptr_type_node; + else if (flag_checking) + gcc_checking_assert (get_alias_set (t) + == get_alias_set (TREE_TYPE (field))); + } + + record_alias_subset (superset, get_alias_set (t)); + } + } break; case COMPLEX_TYPE: Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 271843) +++ ipa-devirt.c (working copy) @@ -213,6 +213,8 @@ struct GTY(()) odr_type_d bool odr_violated; /* Set when virtual table without RTTI previaled table with. */ bool rtti_broken; + /* Set when we need to give up on ODR based TBAA info. */ + bool tbaa_enabled; }; /* Return TRUE if all derived types of T are known and thus @@ -2045,6 +2047,9 @@ get_odr_type (tree type, bool insert) if (!in_lto_p && !TYPE_STRUCTURAL_EQUALITY_P (type)) type = TYPE_CANONICAL (type); + if (!odr_hash) + return NULL; + gcc_checking_assert (can_be_name_hashed_p (type) || can_be_vtable_hashed_p (type)); @@ -2182,6 +2187,45 @@ prevailing_odr_type (tree type) return t->type; } +/* Set tbaa_enabled flag for TYPE. */ + +void +enable_odr_based_tbaa (tree type) +{ + odr_type t = get_odr_type (type, true); + t->tbaa_enabled = true; +} + +/* Return type that in ODR type hash prevailed TYPE. Be careful and punt + on ODR violations. */ + +bool +odr_based_tbaa_p (const_tree type) +{ + odr_type t = get_odr_type (const_cast <tree> (type), false); + if (!t || !t->tbaa_enabled) + return false; + return true; +} + +/* Set TYPE_CANONICAL of type and all its variants and duplicates + to CANONICAL. */ + +void +set_type_canonical_for_odr_type (tree type, tree canonical) +{ + odr_type t = get_odr_type (type, false); + unsigned int i; + tree tt; + + for (tree t2 = t->type; t2; t2 = TYPE_NEXT_VARIANT (t2)) + TYPE_CANONICAL (t2) = canonical; + if (t->types) + FOR_EACH_VEC_ELT (*t->types, i, tt) + for (tree t2 = tt; t2; t2 = TYPE_NEXT_VARIANT (t2)) + TYPE_CANONICAL (t2) = canonical; +} + /* Return true if we reported some ODR violation on TYPE. */ bool Index: ipa-utils.h =================================================================== --- ipa-utils.h (revision 271843) +++ ipa-utils.h (working copy) @@ -93,6 +93,9 @@ bool odr_or_derived_type_p (const_tree t bool odr_types_equivalent_p (tree type1, tree type2); bool odr_type_violation_reported_p (tree type); tree prevailing_odr_type (tree type); +void enable_odr_based_tbaa (tree type); +bool odr_based_tbaa_p (const_tree type); +void set_type_canonical_for_odr_type (tree type, tree canonical); /* Return vector containing possible targets of polymorphic call E. If COMPLETEP is non-NULL, store true if the list is complete. Index: lto/lto-common.c =================================================================== --- lto/lto-common.c (revision 271843) +++ lto/lto-common.c (working copy) @@ -1,5 +1,5 @@ /* Top-level LTO routines. - Copyright (C) 2009-2018 Free Software Foundation, Inc. + Copyright (C) 2009-2019 Free Software Foundation, Inc. Contributed by CodeSourcery, Inc. This file is part of GCC. @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. #include "attribs.h" #include "builtins.h" #include "lto-common.h" +#include "tree-pretty-print.h" GTY(()) tree first_personality_decl; @@ -212,22 +213,26 @@ lto_read_in_decl_state (struct data_in * /* Global canonical type table. */ +static GTY(()) vec<tree, va_gc> *types_to_register = NULL; static htab_t gimple_canonical_types; static hash_map<const_tree, hashval_t> *canonical_type_hash_cache; static unsigned long num_canonical_type_hash_entries; static unsigned long num_canonical_type_hash_queries; -static void iterative_hash_canonical_type (tree type, inchash::hash &hstate); +static void iterative_hash_canonical_type (tree type, inchash::hash &hstate, bool); static hashval_t gimple_canonical_type_hash (const void *p); static void gimple_register_canonical_type_1 (tree t, hashval_t hash); /* Returning a hash value for gimple type TYPE. The hash value returned is equal for types considered compatible - by gimple_canonical_types_compatible_p. */ + by gimple_canonical_types_compatible_p. + + If INSERT is true, also populate canonical type hash by all + types TYPE is derived from. */ static hashval_t -hash_canonical_type (tree type) +hash_canonical_type (tree type, bool insert) { inchash::hash hstate; enum tree_code code; @@ -293,7 +298,7 @@ hash_canonical_type (tree type) if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == COMPLEX_TYPE || TREE_CODE (type) == VECTOR_TYPE) - iterative_hash_canonical_type (TREE_TYPE (type), hstate); + iterative_hash_canonical_type (TREE_TYPE (type), hstate, insert); /* Incorporate function return and argument types. */ if (TREE_CODE (type) == FUNCTION_TYPE || TREE_CODE (type) == METHOD_TYPE) @@ -301,11 +306,11 @@ hash_canonical_type (tree type) unsigned na; tree p; - iterative_hash_canonical_type (TREE_TYPE (type), hstate); + iterative_hash_canonical_type (TREE_TYPE (type), hstate, insert); for (p = TYPE_ARG_TYPES (type), na = 0; p; p = TREE_CHAIN (p)) { - iterative_hash_canonical_type (TREE_VALUE (p), hstate); + iterative_hash_canonical_type (TREE_VALUE (p), hstate, insert); na++; } @@ -322,7 +327,7 @@ hash_canonical_type (tree type) && (! DECL_SIZE (f) || ! integer_zerop (DECL_SIZE (f)))) { - iterative_hash_canonical_type (TREE_TYPE (f), hstate); + iterative_hash_canonical_type (TREE_TYPE (f), hstate, insert); nf++; } @@ -332,10 +337,11 @@ hash_canonical_type (tree type) return hstate.end(); } -/* Returning a hash value for gimple type TYPE combined with VAL. */ +/* Returning a hash value for gimple type TYPE combined with VAL. + If INSERT is true possibly insert TYPE to canonical type hash. */ static void -iterative_hash_canonical_type (tree type, inchash::hash &hstate) +iterative_hash_canonical_type (tree type, inchash::hash &hstate, bool insert) { hashval_t v; @@ -343,7 +349,7 @@ iterative_hash_canonical_type (tree type type = TYPE_MAIN_VARIANT (type); if (!canonical_type_used_p (type)) - v = hash_canonical_type (type); + v = hash_canonical_type (type, insert); /* An already processed type. */ else if (TYPE_CANONICAL (type)) { @@ -355,9 +361,11 @@ iterative_hash_canonical_type (tree type /* Canonical types should not be able to form SCCs by design, this recursion is just because we do not register canonical types in optimal order. To avoid quadratic behavior also register the - type here. */ - v = hash_canonical_type (type); - gimple_register_canonical_type_1 (type, v); + type here. Do not assign canonical types to ODR types - this + is done later using ODR name hash. */ + v = hash_canonical_type (type, insert); + if (insert) + gimple_register_canonical_type_1 (type, v); } hstate.add_int (v); } @@ -437,12 +445,26 @@ gimple_register_canonical_type (tree t) TYPE_CANONICAL (t) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); else { - hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t)); + hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t), true); gimple_register_canonical_type_1 (TYPE_MAIN_VARIANT (t), h); TYPE_CANONICAL (t) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); } } +/* See if structurally equivalent type to T is in the canonical type hash and + return it. Return NULL otherwise. + No changes to canonical hash are made during the lookup. */ + +static tree +lookup_canonical_type (tree t) +{ + hashval_t h = hash_canonical_type (TYPE_MAIN_VARIANT (t), false); + void **slot = htab_find_slot_with_hash (gimple_canonical_types, t, h, NO_INSERT); + if (!slot) + return NULL; + return (tree)(*slot); +} + /* Re-compute TYPE_CANONICAL for NODE and related types. */ static void @@ -464,6 +486,107 @@ lto_register_canonical_types (tree node, gimple_register_canonical_type (node); } +/* Finish canonical type calculation: after all units has been streamed in we + can check if given ODR type structurally conflicts with a non-ODR type. In + the first case we set type canonical according to the canonical type hash. + In the second case we use type names. */ + +static void +lto_register_canonical_types_for_odr_types () +{ + tree t; + unsigned int i; + + if (!types_to_register) + return; + + /* Be sure that no types derived from ODR types was + not inserted into the hash table. */ + if (flag_checking) + FOR_EACH_VEC_ELT (*types_to_register, i, t) + gcc_assert (!TYPE_CANONICAL (t)); + + FOR_EACH_VEC_ELT (*types_to_register, i, t) + if (odr_type_p (t) && !TYPE_CANONICAL (t)) + { + /* Punt on ODR violations - if there are structurally different + types of the same name we are better to not try too hard to + use TBAA. */ + if (odr_type_violation_reported_p (t)) + { + if (symtab->dump_file) + { + fprintf (symtab->dump_file, + "Disabling ODR TBAA because of ODR violation: "); + print_generic_expr (symtab->dump_file, t); + fprintf (symtab->dump_file, "\n"); + } + } + else + { + tree nonodr = type_in_anonymous_namespace_p (t) + ? NULL : lookup_canonical_type (t); + + /* If there is non-ODR type matching T, use its canonical + type. We can still propagate it to all variants of T + including incomplete ones. */ + if (nonodr) + { + gcc_checking_assert (!odr_type_p (nonodr)); + if (symtab->dump_file) + { + fprintf (symtab->dump_file, + "ODR and non-ODR type conflict: "); + print_generic_expr (symtab->dump_file, t); + fprintf (symtab->dump_file, " and "); + print_generic_expr (symtab->dump_file, nonodr); + fprintf (symtab->dump_file, "\n"); + } + set_type_canonical_for_odr_type (t, nonodr); + } + else + { + if (symtab->dump_file) + { + fprintf (symtab->dump_file, + "New canonical ODR type: "); + print_generic_expr (symtab->dump_file, t); + fprintf (symtab->dump_file, "\n"); + } + + tree prevail = prevailing_odr_type (t); + gcc_checking_assert (TYPE_CANONICAL (prevail) == NULL); + + /* Populate canonical type hash with type name. */ + bool existed = canonical_type_hash_cache->put + (prevail, + htab_hash_string + (IDENTIFIER_POINTER + (DECL_ASSEMBLER_NAME + (TYPE_NAME (prevail))))); + gcc_assert (!existed); + /* Set TYPE_CANONICAL for all variants and duplicates of T + including incompete ones. */ + set_type_canonical_for_odr_type (t, prevail); + /* And inform gimple_canonical_types_compatible_p that + it should rely on TYPE_CANONICAL compares only. */ + enable_odr_based_tbaa (t); + gcc_checking_assert (TYPE_CANONICAL (t) = prevail); + } + } + } + + /* Now compute canonical types for all ODR derived types. */ + FOR_EACH_VEC_ELT (*types_to_register, i, t) + if (!TYPE_CANONICAL (t)) + { + gimple_register_canonical_type (t); + /* We store only main variants; propagate info to all variants. */ + for (tree t2 = TYPE_NEXT_VARIANT (t); t2; t2 = TYPE_NEXT_VARIANT (t2)) + TYPE_CANONICAL (t2) = TYPE_CANONICAL (t); + } +} + /* Remember trees that contains references to declarations. */ vec <tree, va_gc> *tree_with_vars; @@ -1655,6 +1778,33 @@ unify_scc (struct data_in *data_in, unsi } +/* TYPE is expected to be record or union. Figure out if it is an ODR + type or derived from it. This is intended to be used only from + stream in process and rely on fact that all types with TYPE_CANONICAL + set are not ODR derived. This reducts most of recursive lookups. */ + +static bool +odr_or_derived_type_p (tree type) +{ + if (TYPE_CANONICAL (type)) + return false; + if (odr_type_p (TYPE_MAIN_VARIANT (type)) + || (TYPE_CONTEXT (type) + && TYPE_P (TYPE_CONTEXT (type)) + && odr_type_p (TYPE_CONTEXT (type)))) + return true; + for (tree f = TYPE_FIELDS (type); f; f = TREE_CHAIN (f)) + { + tree t = TREE_TYPE (f); + while (TREE_CODE (t) == ARRAY_TYPE) + t = TREE_TYPE (t); + if (RECORD_OR_UNION_TYPE_P (t) + && odr_or_derived_type_p (t)) + return true; + } + return false; +} + /* Read all the symbols from buffer DATA, using descriptors in DECL_DATA. RESOLUTIONS is the set of symbols picked by the linker (read from the resolution file when the linker plugin is being used). */ @@ -1747,12 +1897,24 @@ lto_read_decls (struct lto_file_decl_dat num_prevailing_types++; lto_fixup_prevailing_type (t); - /* Compute the canonical type of all types. + /* Compute the canonical type of all non-ODR types. + Delay ODR types for the end of merging process - the canonical + type for those can be computed using the (unique) name however + we want to do this only if units in other languages do not + contain structurally equivalent type. + Because SCC components are streamed in random (hash) order we may have encountered the type before while registering type canonical of a derived type in the same SCC. */ if (!TYPE_CANONICAL (t)) - gimple_register_canonical_type (t); + { + if (!RECORD_OR_UNION_TYPE_P (t) + || !odr_or_derived_type_p (t)) + gimple_register_canonical_type (t); + else if (COMPLETE_TYPE_P (t) + && TYPE_MAIN_VARIANT (t) == t) + vec_safe_push (types_to_register, t); + } if (TYPE_MAIN_VARIANT (t) == t && odr_type_p (t)) register_odr_type (t); } @@ -2602,6 +2764,8 @@ read_cgraph_and_symbols (unsigned nfiles ggc_free(decl_data); real_file_decl_data = NULL; + lto_register_canonical_types_for_odr_types (); + if (resolution_file_name) fclose (resolution); Index: testsuite/g++.dg/lto/alias-2_0.C =================================================================== --- testsuite/g++.dg/lto/alias-2_0.C (nonexistent) +++ testsuite/g++.dg/lto/alias-2_0.C (working copy) @@ -0,0 +1,31 @@ +/* { dg-lto-do run } */ +/* { dg-lto-options { { -O2 -flto } } } */ + +/* With LTO we consider all pointers to incomplete types to be possibly + aliasing. This makes *bptr to alias with aptr. + For C++ ODR types we however can work out that they are actually + different. */ + +#include <string.h> + +typedef int (*fnptr) (); + +__attribute__ ((used)) +struct a *aptr; + +__attribute__ ((used)) +struct b **bptr = (struct b**)&aptr; +extern void init (); +extern void inline_me_late (int); + + +int +main (int argc, char **argv) +{ + init (); + aptr = 0; + inline_me_late (argc); + if (!__builtin_constant_p (aptr == 0)) + __builtin_abort (); + return (size_t)aptr; +} Index: testsuite/g++.dg/lto/alias-2_1.C =================================================================== --- testsuite/g++.dg/lto/alias-2_1.C (nonexistent) +++ testsuite/g++.dg/lto/alias-2_1.C (working copy) @@ -0,0 +1,16 @@ +#include <string.h> +struct a {int a;} a; +struct b {int b;} b; +extern struct b **bptr; +void +inline_me_late (int argc) +{ + if (argc == -1) + *bptr = (struct b *)(size_t)1; +} +void +init() +{ + a.a=1; + b.b=2; +} Index: tree.c =================================================================== --- tree.c (revision 271843) +++ tree.c (working copy) @@ -14083,6 +14083,7 @@ gimple_canonical_types_compatible_p (con gcc_assert (!trust_type_canonical || (type_with_alias_set_p (t1) && type_with_alias_set_p (t2))); + /* If the types have been previously registered and found equal they still are. */ @@ -14100,6 +14101,14 @@ gimple_canonical_types_compatible_p (con return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); } + /* For types where we do ODR based TBAA the canonical type is always + set correctly, so we know that types are different if their + canonical types does not match. */ + if (trust_type_canonical + && (odr_type_p (t1) && odr_based_tbaa_p (t1)) + != (odr_type_p (t2) && odr_based_tbaa_p (t2))) + return false; + /* Can't be the same type if the types don't have the same code. */ enum tree_code code = tree_code_for_canonical_type_merging (TREE_CODE (t1)); if (code != tree_code_for_canonical_type_merging (TREE_CODE (t2)))
Hi, this patch makes LTO to use ODR names when building canonical types. Theoretically this is easy task because every ODR type is unique and it is enough to fill in the hash with the ODR names and compare them. In reality we want to be more careful and detect situation when non-ODR type is structurally equivalent to ODR type so C++ code and non-C++ code can co-exist. I have implemented it as follows 1) at stream in time populate canonical type hash by non-ODR types only. Also register all ODR types into so we detect ODR violations. 2) once all streaming is done we process all ODR types, lookup each of them in the canonical type hash and if structurally compatible type is present default to normal canonical type calculation. Otherwise canonical type of ODR type is the prevailing ODR variant. Now the problem is that in C++ some structures can be non-ODR. For example struct A {int a;} a; struct {int b;} b; Here the second structure has no name and we have no way to tell if it is C++ or non-C++ code at LTO time. I tought that perhaps such conflicts would be rare, but in reality most of types conflicts this way (C++ FE seems to generate non-ODR types of same strucutre as ODR types and they do get streamed, I am looking into this). So more careful approach is needed. I extende extend step 1 to also postpone types that reffers to ODR types (those are known to originate from C++ translation units and do not need to be unified by structurally equivalent ODR types). This is tested by odr_or_derived_type_p which is kind of hack and perhaps we should get around streaming this info somehow (but i would preffer to do that incrementally) Step 2 remain same. Additional step 3 registers all odr derived types into canonical type hash. This requires canonical type hash to play well with ODR types (i.e. not consider them equivalent based on structural equivalety). The decision on whether given type has ODR based canonical type is stored in odr_based_tbaa_p and is used 1) by gimple_canonical_types_compatible_p so the comparsion is behaving well after ODR types are inserted into the hash 2) by alias.c to be build more precise alias sets - for ODR types it is not necessary to glob pointers into void *. There is no need to update canonical type hash since I simply insert hash codes of ODR names into canonical type hash cache. LTO linking cc1plus there are 3317 ODR types that gets their own canonical type and 876 which have conflict. For example: ODR and non-ODR type conflict: union ssa_name_info_type and union { const void * cv; void * v; } ODR and non-ODR type conflict: struct ht_identifier and struct { union byte_fail_stack_elt_t * stack; unsigned int size; unsigned int avail; } ODR and non-ODR type conflict: struct lang_hooks_for_tree_dump and struct { struct demangle_component * left; struct demangle_component * right; } ODR and non-ODR type conflict: struct call_site_record_d and struct { struct demangle_component * sub; int num; } etc. So mostly random conflict induced by the C compiled liberty and other bits. Clearly most common issue is globing of pointers and lack of field names which is needed primarily for Fortran and is easy to disable for translation units not having Fortran modules in them. Canonical type hash stats for cc1plus build: [WPA] GIMPLE canonical type table: size 16381, 1590 elements, 6797 searches, 88 collisions (ratio: 0.012947) [WPA] GIMPLE canonical type pointer-map: 1590 elements, 15494 searches to: [WPA] GIMPLE canonical type table: size 16381, 755 elements, 6311 searches, 57 collisions (ratio: 0.009032) [WPA] GIMPLE canonical type pointer-map: 755 elements, 15553 searches So without patch there are 1590 distinct canonical types, with the patch there are 755 non-ODR and 3317 odr types, about 2.5 times more. And alias oracle stats for -flto-partition=none cc1plus build: Alias oracle query stats: refs_may_alias_p: 39227004 disambiguations, 47344870 queries ref_maybe_used_by_call_p: 57815 disambiguations, 39808081 queries call_may_clobber_ref_p: 5511 disambiguations, 8287 queries aliasing_component_ref_p: 90654 disambiguations, 269895 queries TBAA oracle: 11130153 disambiguations 34368560 queries 14199938 are in alias set 0 5193428 queries asked about the same object 147 queries asked about the same alias set 0 access volatile 1665979 are dependent in the DAG 2178915 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 464074 disambiguations, 7105649 queries pt_solutions_intersect: 355139 disambiguations, 6952492 queries Alias oracle query stats: refs_may_alias_p: 39682205 disambiguations, 47774305 queries ref_maybe_used_by_call_p: 58390 disambiguations, 40265537 queries call_may_clobber_ref_p: 5619 disambiguations, 8397 queries aliasing_component_ref_p: 61523 disambiguations, 240208 queries TBAA oracle: 11691755 disambiguations 34552651 queries 14233568 are in alias set 0 5230754 queries asked about the same object 147 queries asked about the same alias set 0 access volatile 2433081 are dependent in the DAG 963346 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 457871 disambiguations, 7044729 queries pt_solutions_intersect: 367701 disambiguations, 6859208 queries So about 455k new disambiguations or 5% more TBAA ones. It is about half of what PTA oracle stats shows, so it seems reasibale improvement to me. The patch results in about 1% cc1plus code size increase. (mostly because of less ICF) For tramp3d the stats are: refs_may_alias_p: 3252729 disambiguations, 3587196 queries ref_maybe_used_by_call_p: 7018 disambiguations, 3280028 queries call_may_clobber_ref_p: 883 disambiguations, 883 queries aliasing_component_ref_p: 549 disambiguations, 17885 queries TBAA oracle: 1644568 disambiguations 3261681 queries 555812 are in alias set 0 680883 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 248396 are dependent in the DAG 132022 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 641563 disambiguations, 921034 queries pt_solutions_intersect: 115861 disambiguations, 501205 queries Alias oracle query stats: refs_may_alias_p: 3016277 disambiguations, 3316301 queries ref_maybe_used_by_call_p: 7134 disambiguations, 3042055 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries aliasing_component_ref_p: 495 disambiguations, 14565 queries TBAA oracle: 1416803 disambiguations 2911585 queries 552489 are in alias set 0 567158 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 259923 are dependent in the DAG 115212 are aritificially in conflict with void * PTA query stats: pt_solution_includes: 668779 disambiguations, 949986 queries pt_solutions_intersect: 96232 disambiguations, 436617 queries About 7% difference. I did Firefox stats with earlier variant of this patch and difference was about 12%. It takes about a day for lto1-ltrans with one partition to converge (and two days for for gas) so I did not re-run it with final version. Code size difference is about 4%. Notice also that access path oracle now suceeds less often since we less often mix up unrealted types. In general with more precist TBAA lookups the oracle should be bit cheaper by avoiding more complex tests. lto-bootstrapped/regtested x86_64-linux with all languages and also tested on few extra C++ apps. * alias.c (record_component_aliases): Honor odr_based_tbaa_p. * ipa-devirt.c (odr_type_d): Add tbaa_enabled field. (get_odr_type): Return NULL when odr_type_hash is NULL. (enable_odr_based_tbaa): New function. (odr_based_tbaa_p): New function. (set_type_canonical_for_odr_type): New function. * ipa-utils.h (enable_odr_based_tbaa, odr_based_tbaa_p, set_type_canonical_for_odr_type): Declare. * lto-common.c: Update copyright; include tree-pretty-print.h. (types_to_register): New static var. (iterative_hash_canonical_type): Add new parameter INSERT. (hash_canonical_type): Likewise. (lookup_canonical_type): New function. (lto_register_canonical_types_for_odr_types): New. (odr_or_derived_type_p): New function. (lto_read_decls): Deffer ODR and ODR derived types to later canonical type calculation. * tree.c (gimple_canonical_types_compatible_p): Honnor odr_based_tbaa_p.