* Enable pointer TBAA for LTO @ 2015-11-08 20:46 Jan Hubicka 2015-11-10 12:27 ` Richard Biener 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-08 20:46 UTC (permalink / raw) To: gcc-patches, rguenther Hi, this patch adds basic TBAA for pointers to LTO. The basic scheme is simple; because TYPE_CANONICAL is not really needed by get_alias_set, we completely drop the caluclation of these (which also saves about 50% canonical type hash searches) and update get_alias_set to not punt on pointers with TYPE_STRUCTURAL_EQUALITY. The patch makes quite nice improvements (32%) on number of disambiguations on dealII (that is my random C++ testbed): Before: [WPA] GIMPLE canonical type table: size 16381, 817 elements, 35453 searches, 91 collisions (ratio: 0.002567) [WPA] GIMPLE canonical type pointer-map: 817 elements, 15570 searches after: [WPA] GIMPLE canonical type table: size 16381, 822 elements, 14863 searches, 114 collisions (ratio: 0.007670) [WPA] GIMPLE canonical type pointer-map: 822 elements, 12663 searches The number of disambiguations goes 1713472->2331078 (32%) and number of queries goes 3387753->3669698 (8%) We get code size growth 677527->701782 (3%) Also a query is disambiguated 63% of the time instead of 50% we had before. Clearly there are many areas for improvements (since functions are TYPE_STRUCTURAL_EQUALITY in LTO we ptr_type_node alias set on them), but that M can wait for next stage1. lto-bootstrapped/regtested x86_64-linux and also used it in my tree for quite a while, so the patch was tested on Firefox and other applications. OK? Honza * alias.c (get_alias_set): Do structural equality for pointer types; drop LTO specific path. * lto.c (iterative_hash_canonical_type): Do not compute TYPE_CANONICAL for pointer types. (gimple_register_canonical_type_1): Likewise. (gimple_register_canonical_type): Likewise. (lto_register_canonical_types): Do not clear canonical types of pointer types. Index: lto/lto.c =================================================================== --- lto/lto.c (revision 229968) +++ lto/lto.c (working copy) @@ -396,8 +396,13 @@ iterative_hash_canonical_type (tree type /* All type variants have same TYPE_CANONICAL. */ type = TYPE_MAIN_VARIANT (type); + + /* We do not compute TYPE_CANONICAl of POINTER_TYPE becuase the aliasing + code never use it anyway. */ + if (POINTER_TYPE_P (type)) + v = hash_canonical_type (type); /* An already processed type. */ - if (TYPE_CANONICAL (type)) + else if (TYPE_CANONICAL (type)) { type = TYPE_CANONICAL (type); v = gimple_canonical_type_hash (type); @@ -445,7 +450,9 @@ gimple_register_canonical_type_1 (tree t { void **slot; - gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t)); + gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) + && type_with_alias_set_p (t) + && TREE_CODE (t) != POINTER_TYPE); slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); if (*slot) @@ -478,7 +485,7 @@ gimple_register_canonical_type_1 (tree t static void gimple_register_canonical_type (tree t) { - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t)) + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) return; /* Canonical types are same among all complete variants. */ @@ -498,14 +505,13 @@ static void lto_register_canonical_types (tree node, bool first_p) { if (!node - || !TYPE_P (node)) + || !TYPE_P (node) || POINTER_TYPE_P (node)) return; if (first_p) TYPE_CANONICAL (node) = NULL_TREE; - if (POINTER_TYPE_P (node) - || TREE_CODE (node) == COMPLEX_TYPE + if (TREE_CODE (node) == COMPLEX_TYPE || TREE_CODE (node) == ARRAY_TYPE) lto_register_canonical_types (TREE_TYPE (node), first_p); Index: tree.c =================================================================== --- tree.c (revision 229968) +++ tree.c (working copy) @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con /* If the types have been previously registered and found equal they still are. */ if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) + && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2) && trust_type_canonical) return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); Index: alias.c =================================================================== --- alias.c (revision 229968) +++ alias.c (working copy) @@ -869,13 +874,19 @@ get_alias_set (tree t) set = lang_hooks.get_alias_set (t); if (set != -1) return set; - return 0; + /* LTO frontend does not assign canonical types to pointers (which we + ignore anyway) and we compute them. The following path may be + probably enabled for non-LTO, too, and it may improve TBAA for + pointers to types with structural equality. */ + if (!in_lto_p || !POINTER_TYPE_P (t)) + return 0; + } + else + { + t = TYPE_CANONICAL (t); + /* The canonical type should not require structural equality checks. */ + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); } - - t = TYPE_CANONICAL (t); - - /* The canonical type should not require structural equality checks. */ - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); /* If this is a type with a known alias set, return it. */ if (TYPE_ALIAS_SET_KNOWN_P (t)) @@ -952,20 +963,23 @@ get_alias_set (tree t) ptr_type_node but that is a bad idea, because it prevents disabiguations in between pointers. For Firefox this accounts about 20% of all disambiguations in the program. */ - else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p) + else if (POINTER_TYPE_P (t) && t != ptr_type_node) { tree p; auto_vec <bool, 8> reference; /* Unnest all pointers and references. - We also want to make pointer to array equivalent to pointer to its - element. So skip all array types, too. */ + We also want to make pointer to array/vector equivalent to pointer to + its element (see the reasoning above). Skip all those types, too. */ for (p = t; POINTER_TYPE_P (p) - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)); + || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) + || TREE_CODE (p) == VECTOR_TYPE; p = TREE_TYPE (p)) { if (TREE_CODE (p) == REFERENCE_TYPE) - reference.safe_push (true); + /* In LTO we want languages that use references to be compatible + with languages that use pointers. */ + reference.safe_push (true && !in_lto_p); if (TREE_CODE (p) == POINTER_TYPE) reference.safe_push (false); } @@ -998,9 +1012,23 @@ get_alias_set (tree t) p = build_reference_type (p); else p = build_pointer_type (p); - p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p)); + p = TYPE_MAIN_VARIANT (p); + /* Normally all pointer types are built by + build_pointer_type_for_mode which ensures they have canonical + type unless they point to type with structural equality. + LTO frontend produce pointer types without TYPE_CANONICAL + that are then added to TYPE_POINTER_TO lists and + build_pointer_type_for_mode will end up picking one for us. + Declare it the canonical one. This is the same as + build_pointer_type_for_mode would do. */ + if (!TYPE_CANONICAL (p)) + { + TYPE_CANONICAL (p) = p; + gcc_checking_assert (in_lto_p); + } + else + gcc_checking_assert (p == TYPE_CANONICAL (p)); } - gcc_checking_assert (TYPE_CANONICAL (p) == p); /* Assign the alias set to both p and t. We can not call get_alias_set (p) here as that would trigger @@ -1015,11 +1043,6 @@ get_alias_set (tree t) } } } - /* In LTO the rules above needs to be part of canonical type machinery. - For now just punt. */ - else if (POINTER_TYPE_P (t) - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); /* Otherwise make a new alias set for this type. */ else ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-08 20:46 Enable pointer TBAA for LTO Jan Hubicka @ 2015-11-10 12:27 ` Richard Biener 2015-11-10 18:15 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Richard Biener @ 2015-11-10 12:27 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Sun, 8 Nov 2015, Jan Hubicka wrote: > Hi, > this patch adds basic TBAA for pointers to LTO. The basic scheme is simple; > because TYPE_CANONICAL is not really needed by get_alias_set, we completely > drop the caluclation of these (which also saves about 50% canonical type hash > searches) and update get_alias_set to not punt on pointers with > TYPE_STRUCTURAL_EQUALITY. > > The patch makes quite nice improvements (32%) on number of disambiguations on > dealII (that is my random C++ testbed): > > Before: > [WPA] GIMPLE canonical type table: size 16381, 817 elements, 35453 searches, 91 collisions (ratio: 0.002567) > [WPA] GIMPLE canonical type pointer-map: 817 elements, 15570 searches > > after: > [WPA] GIMPLE canonical type table: size 16381, 822 elements, 14863 searches, 114 collisions (ratio: 0.007670) > [WPA] GIMPLE canonical type pointer-map: 822 elements, 12663 searches > > The number of disambiguations goes 1713472->2331078 (32%) > and number of queries goes 3387753->3669698 (8%) > We get code size growth 677527->701782 (3%) > > Also a query is disambiguated 63% of the time instead of 50% we had before. > > Clearly there are many areas for improvements (since functions are > TYPE_STRUCTURAL_EQUALITY in LTO we ptr_type_node alias set on them), but that > M > can wait for next stage1. > > lto-bootstrapped/regtested x86_64-linux and also used it in my tree for quite > a while, so the patch was tested on Firefox and other applications. > > OK? > Honza > > * alias.c (get_alias_set): Do structural equality for pointer types; > drop LTO specific path. > * lto.c (iterative_hash_canonical_type): Do not compute TYPE_CANONICAL > for pointer types. > (gimple_register_canonical_type_1): Likewise. > (gimple_register_canonical_type): Likewise. > (lto_register_canonical_types): Do not clear canonical types of pointer > types. > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 229968) > +++ lto/lto.c (working copy) > @@ -396,8 +396,13 @@ iterative_hash_canonical_type (tree type > > /* All type variants have same TYPE_CANONICAL. */ > type = TYPE_MAIN_VARIANT (type); > + > + /* We do not compute TYPE_CANONICAl of POINTER_TYPE becuase the aliasing > + code never use it anyway. */ > + if (POINTER_TYPE_P (type)) > + v = hash_canonical_type (type); > /* An already processed type. */ > - if (TYPE_CANONICAL (type)) > + else if (TYPE_CANONICAL (type)) > { > type = TYPE_CANONICAL (type); > v = gimple_canonical_type_hash (type); > @@ -445,7 +450,9 @@ gimple_register_canonical_type_1 (tree t > { > void **slot; > > - gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t)); > + gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) > + && type_with_alias_set_p (t) > + && TREE_CODE (t) != POINTER_TYPE); POINTER_TYPE_P > > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > @@ -478,7 +485,7 @@ gimple_register_canonical_type_1 (tree t > static void > gimple_register_canonical_type (tree t) > { > - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t)) > + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) > return; > > /* Canonical types are same among all complete variants. */ > @@ -498,14 +505,13 @@ static void > lto_register_canonical_types (tree node, bool first_p) > { > if (!node > - || !TYPE_P (node)) > + || !TYPE_P (node) || POINTER_TYPE_P (node)) > return; > > if (first_p) > TYPE_CANONICAL (node) = NULL_TREE; > > - if (POINTER_TYPE_P (node) > - || TREE_CODE (node) == COMPLEX_TYPE > + if (TREE_CODE (node) == COMPLEX_TYPE > || TREE_CODE (node) == ARRAY_TYPE) > lto_register_canonical_types (TREE_TYPE (node), first_p); Hmm, here we'll miss registering canoncial types for the pointed-to type. I believe this will miss FILE for example but also some va_list types? Please drop this change. > Index: tree.c > =================================================================== > --- tree.c (revision 229968) > +++ tree.c (working copy) > @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con > /* If the types have been previously registered and found equal > they still are. */ > if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) > + && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2) But TYPE_CANONICAL (t1) should be NULL_TREE for POINTER_TYPE_P? > && trust_type_canonical) > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > > Index: alias.c > =================================================================== > --- alias.c (revision 229968) > +++ alias.c (working copy) > @@ -869,13 +874,19 @@ get_alias_set (tree t) > set = lang_hooks.get_alias_set (t); > if (set != -1) > return set; > - return 0; > + /* LTO frontend does not assign canonical types to pointers (which we > + ignore anyway) and we compute them. The following path may be > + probably enabled for non-LTO, too, and it may improve TBAA for > + pointers to types with structural equality. */ > + if (!in_lto_p || !POINTER_TYPE_P (t)) > + return 0; No new LTO paths please, do the suggested change immediately. > + } > + else > + { > + t = TYPE_CANONICAL (t); > + /* The canonical type should not require structural equality checks. */ > + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > } > - > - t = TYPE_CANONICAL (t); > - > - /* The canonical type should not require structural equality checks. */ > - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > > /* If this is a type with a known alias set, return it. */ > if (TYPE_ALIAS_SET_KNOWN_P (t)) > @@ -952,20 +963,23 @@ get_alias_set (tree t) > ptr_type_node but that is a bad idea, because it prevents disabiguations > in between pointers. For Firefox this accounts about 20% of all > disambiguations in the program. */ > - else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p) > + else if (POINTER_TYPE_P (t) && t != ptr_type_node) > { > tree p; > auto_vec <bool, 8> reference; > > /* Unnest all pointers and references. > - We also want to make pointer to array equivalent to pointer to its > - element. So skip all array types, too. */ > + We also want to make pointer to array/vector equivalent to pointer to > + its element (see the reasoning above). Skip all those types, too. */ > for (p = t; POINTER_TYPE_P (p) > - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)); > + || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) > + || TREE_CODE (p) == VECTOR_TYPE; > p = TREE_TYPE (p)) > { > if (TREE_CODE (p) == REFERENCE_TYPE) > - reference.safe_push (true); > + /* In LTO we want languages that use references to be compatible > + with languages that use pointers. */ > + reference.safe_push (true && !in_lto_p); > if (TREE_CODE (p) == POINTER_TYPE) > reference.safe_push (false); > } > @@ -998,9 +1012,23 @@ get_alias_set (tree t) > p = build_reference_type (p); > else > p = build_pointer_type (p); > - p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p)); > + p = TYPE_MAIN_VARIANT (p); > + /* Normally all pointer types are built by > + build_pointer_type_for_mode which ensures they have canonical > + type unless they point to type with structural equality. > + LTO frontend produce pointer types without TYPE_CANONICAL > + that are then added to TYPE_POINTER_TO lists and > + build_pointer_type_for_mode will end up picking one for us. > + Declare it the canonical one. This is the same as > + build_pointer_type_for_mode would do. */ > + if (!TYPE_CANONICAL (p)) > + { > + TYPE_CANONICAL (p) = p; > + gcc_checking_assert (in_lto_p); > + } > + else > + gcc_checking_assert (p == TYPE_CANONICAL (p)); The assert can trigger as build_pointer_type_for_mode builds SET_TYPE_STRUCTURAL_EQUALITY pointer types for SET_TYPE_STRUCTURAL_EQUALITY pointed-to types. Ah, looking up more context reveals if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p)) set = get_alias_set (ptr_type_node); Not sure why you adjust TYPE_CANONICAL here at all either. Otherwise looks ok. RIchard. > } > - gcc_checking_assert (TYPE_CANONICAL (p) == p); > > /* Assign the alias set to both p and t. > We can not call get_alias_set (p) here as that would trigger > @@ -1015,11 +1043,6 @@ get_alias_set (tree t) > } > } > } > - /* In LTO the rules above needs to be part of canonical type machinery. > - For now just punt. */ > - else if (POINTER_TYPE_P (t) > - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) > - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); > > /* Otherwise make a new alias set for this type. */ > else > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-10 12:27 ` Richard Biener @ 2015-11-10 18:15 ` Jan Hubicka 2015-11-11 9:21 ` Richard Biener 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-10 18:15 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches > > Index: tree.c > > =================================================================== > > --- tree.c (revision 229968) > > +++ tree.c (working copy) > > @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con > > /* If the types have been previously registered and found equal > > they still are. */ > > if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) > > + && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2) > > But TYPE_CANONICAL (t1) should be NULL_TREE for POINTER_TYPE_P? The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be called before we finish all merging and then it is more fine grained than what we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be different, but here we want them to be equal so we can match: struct aa { void *ptr;}; struct bb { int * ptr;}; Which is actually required for Fortran interoperability. Removing this hunk triggers false type incompatibility warning in one of the interoperability testcases I added. Even if I drop the code bellow setting TYPE_CANOINCAL, I think I need to keep this conditional: the types may be built in and those get TYPE_CANONICAL set as they are constructed by build_pointer_type. I can gcc_checking_assert for this scenario and see. Perhaps we never build LTO type from builtin type and this won't happen. If we did, we would probably have a trouble with false negatives in return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); on non-pointers anyway. > > > && trust_type_canonical) > > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > > > > Index: alias.c > > =================================================================== > > --- alias.c (revision 229968) > > +++ alias.c (working copy) > > @@ -869,13 +874,19 @@ get_alias_set (tree t) > > set = lang_hooks.get_alias_set (t); > > if (set != -1) > > return set; > > - return 0; > > + /* LTO frontend does not assign canonical types to pointers (which we > > + ignore anyway) and we compute them. The following path may be > > + probably enabled for non-LTO, too, and it may improve TBAA for > > + pointers to types with structural equality. */ > > + if (!in_lto_p || !POINTER_TYPE_P (t)) > > + return 0; > > No new LTO paths please, do the suggested change immediately. OK, I originally tested the patch without if and there was no problems. Just chickened out before preparing final version of the patch. > > + p = TYPE_MAIN_VARIANT (p); > > + /* Normally all pointer types are built by > > + build_pointer_type_for_mode which ensures they have canonical > > + type unless they point to type with structural equality. > > + LTO frontend produce pointer types without TYPE_CANONICAL > > + that are then added to TYPE_POINTER_TO lists and > > + build_pointer_type_for_mode will end up picking one for us. > > + Declare it the canonical one. This is the same as > > + build_pointer_type_for_mode would do. */ > > + if (!TYPE_CANONICAL (p)) > > + { > > + TYPE_CANONICAL (p) = p; > > + gcc_checking_assert (in_lto_p); > > + } > > + else > > + gcc_checking_assert (p == TYPE_CANONICAL (p)); > > The assert can trigger as > build_pointer_type_for_mode builds SET_TYPE_STRUCTURAL_EQUALITY pointer > types for SET_TYPE_STRUCTURAL_EQUALITY pointed-to types. Ah, > looking up more context reveals > > if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p)) > set = get_alias_set (ptr_type_node); Yep, we don't get here. > > Not sure why you adjust TYPE_CANONICAL here at all either. You are right, I may probably just drop all the code and just do: gcc_checking_assert (!TYPE_CANONICAL || p == TYPE_CANONICAL (p)); I will test this and re-think the build_pointer_type code to be sure that we won't get into a problem there. As I recall, the original code p = TYPE_CANONICAL (p); was there to permit frontends to glob two pointers by setting same canonical type to them. My original plan was to use this for LTO frotnend and make gimple_compare_canonical_types to do the right thing for pointers and this would follow gimple_compare_canonical_types globbing then. This idea was wrong: since pointer rules are not transitive (i.e. void * alias them all), we can't model that by an equivalence produced by gimple_compare_canonical_types. Since the assert does not trigger, seems no frontend is doing that and moreover I do not see how that would be useful (well, perhaps for some kind of internal bookeeping when build TYPE_CANONICAL of more complex types from pointer types, like arrays, but for those we ignore TYPE_CANONICAL anyway). Grepping over TYPE_CANONICAL sets in frotneds, I see no code that I would suspect from doing something like this. Thank you! Honza > > Otherwise looks ok. > > RIchard. > > > > } > > - gcc_checking_assert (TYPE_CANONICAL (p) == p); > > > > /* Assign the alias set to both p and t. > > We can not call get_alias_set (p) here as that would trigger > > @@ -1015,11 +1043,6 @@ get_alias_set (tree t) > > } > > } > > } > > - /* In LTO the rules above needs to be part of canonical type machinery. > > - For now just punt. */ > > - else if (POINTER_TYPE_P (t) > > - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) > > - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); > > > > /* Otherwise make a new alias set for this type. */ > > else > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-10 18:15 ` Jan Hubicka @ 2015-11-11 9:21 ` Richard Biener 2015-11-11 12:43 ` Bernd Schmidt 0 siblings, 1 reply; 28+ messages in thread From: Richard Biener @ 2015-11-11 9:21 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Tue, 10 Nov 2015, Jan Hubicka wrote: > > > Index: tree.c > > > =================================================================== > > > --- tree.c (revision 229968) > > > +++ tree.c (working copy) > > > @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con > > > /* If the types have been previously registered and found equal > > > they still are. */ > > > if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) > > > + && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2) > > > > But TYPE_CANONICAL (t1) should be NULL_TREE for POINTER_TYPE_P? > > The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be > called before we finish all merging and then it is more fine grained than what > we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be > different, but here we want them to be equal so we can match: > > struct aa { void *ptr;}; > struct bb { int * ptr;}; > > Which is actually required for Fortran interoperability. > > Removing this hunk triggers false type incompatibility warning in one of the > interoperability testcases I added. Ok, I see. > Even if I drop the code bellow setting TYPE_CANOINCAL, I think I need to keep > this conditional: the types may be built in and those get TYPE_CANONICAL set as > they are constructed by build_pointer_type. I can gcc_checking_assert for this > scenario and see. Perhaps we never build LTO type from builtin type and this > won't happen. If we did, we would probably have a trouble with false negatives > in return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); on non-pointers anyway. Hmm, indeed. The various type builders might end up setting TYPE_CANONICAL if you ever run into a pre-defined pointer type (ptr_type_node for example). > > > > > && trust_type_canonical) > > > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > > > > > > Index: alias.c > > > =================================================================== > > > --- alias.c (revision 229968) > > > +++ alias.c (working copy) > > > @@ -869,13 +874,19 @@ get_alias_set (tree t) > > > set = lang_hooks.get_alias_set (t); > > > if (set != -1) > > > return set; > > > - return 0; > > > + /* LTO frontend does not assign canonical types to pointers (which we > > > + ignore anyway) and we compute them. The following path may be > > > + probably enabled for non-LTO, too, and it may improve TBAA for > > > + pointers to types with structural equality. */ > > > + if (!in_lto_p || !POINTER_TYPE_P (t)) > > > + return 0; > > > > No new LTO paths please, do the suggested change immediately. > > OK, I originally tested the patch without if and there was no problems. > Just chickened out before preparing final version of the patch. > > > + p = TYPE_MAIN_VARIANT (p); > > > + /* Normally all pointer types are built by > > > + build_pointer_type_for_mode which ensures they have canonical > > > + type unless they point to type with structural equality. > > > + LTO frontend produce pointer types without TYPE_CANONICAL > > > + that are then added to TYPE_POINTER_TO lists and > > > + build_pointer_type_for_mode will end up picking one for us. > > > + Declare it the canonical one. This is the same as > > > + build_pointer_type_for_mode would do. */ > > > + if (!TYPE_CANONICAL (p)) > > > + { > > > + TYPE_CANONICAL (p) = p; > > > + gcc_checking_assert (in_lto_p); > > > + } > > > + else > > > + gcc_checking_assert (p == TYPE_CANONICAL (p)); > > > > The assert can trigger as > > build_pointer_type_for_mode builds SET_TYPE_STRUCTURAL_EQUALITY pointer > > types for SET_TYPE_STRUCTURAL_EQUALITY pointed-to types. Ah, > > looking up more context reveals > > > > if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p)) > > set = get_alias_set (ptr_type_node); > > Yep, we don't get here. > > > > Not sure why you adjust TYPE_CANONICAL here at all either. > > You are right, I may probably just drop all the code and just do: > gcc_checking_assert (!TYPE_CANONICAL || p == TYPE_CANONICAL (p)); > I will test this and re-think the build_pointer_type code to be sure that we > won't get into a problem there. > > As I recall, the original code > p = TYPE_CANONICAL (p); > was there to permit frontends to glob two pointers by setting same canonical > type to them. Yes. > My original plan was to use this for LTO frotnend and make > gimple_compare_canonical_types to do the right thing for pointers and this would > follow gimple_compare_canonical_types globbing then. > > This idea was wrong: since pointer rules are not transitive (i.e. void > * alias them all), we can't model that by an equivalence produced by > gimple_compare_canonical_types. > > Since the assert does not trigger, seems no frontend is doing that and moreover > I do not see how that would be useful (well, perhaps for some kind of internal > bookeeping when build TYPE_CANONICAL of more complex types from pointer types, > like arrays, but for those we ignore TYPE_CANONICAL anyway). Grepping over > TYPE_CANONICAL sets in frotneds, I see no code that I would suspect from doing > something like this. Ok. Let's see what your experiment shows, otherwise the original patch is ok. Thanks, Richard. > Thank you! > Honza > > > > Otherwise looks ok. > > > > RIchard. > > > > > > > } > > > - gcc_checking_assert (TYPE_CANONICAL (p) == p); > > > > > > /* Assign the alias set to both p and t. > > > We can not call get_alias_set (p) here as that would trigger > > > @@ -1015,11 +1043,6 @@ get_alias_set (tree t) > > > } > > > } > > > } > > > - /* In LTO the rules above needs to be part of canonical type machinery. > > > - For now just punt. */ > > > - else if (POINTER_TYPE_P (t) > > > - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) > > > - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); > > > > > > /* Otherwise make a new alias set for this type. */ > > > else > > > > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-11 9:21 ` Richard Biener @ 2015-11-11 12:43 ` Bernd Schmidt 2015-11-11 22:14 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Bernd Schmidt @ 2015-11-11 12:43 UTC (permalink / raw) To: Richard Biener, Jan Hubicka; +Cc: gcc-patches On 11/11/2015 10:21 AM, Richard Biener wrote: > On Tue, 10 Nov 2015, Jan Hubicka wrote: >> The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be >> called before we finish all merging and then it is more fine grained than what >> we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be >> different, but here we want them to be equal so we can match: >> >> struct aa { void *ptr;}; >> struct bb { int * ptr;}; >> >> Which is actually required for Fortran interoperability. Just curious, is this sort of thing documented anywhere? Bernd ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-11 12:43 ` Bernd Schmidt @ 2015-11-11 22:14 ` Jan Hubicka 2015-11-11 22:31 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-11 22:14 UTC (permalink / raw) To: Bernd Schmidt; +Cc: Richard Biener, Jan Hubicka, gcc-patches > On 11/11/2015 10:21 AM, Richard Biener wrote: > >On Tue, 10 Nov 2015, Jan Hubicka wrote: > >>The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be > >>called before we finish all merging and then it is more fine grained than what > >>we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be > >>different, but here we want them to be equal so we can match: > >> > >>struct aa { void *ptr;}; > >>struct bb { int * ptr;}; > >> > >>Which is actually required for Fortran interoperability. > > Just curious, is this sort of thing documented anywhere? See http://www.j3-fortran.org/doc/year/10/10-007.pdf, section 15 (interoperability with C). It defines that C_PTR is compatible with any C non-function pointer. Honza > > > Bernd ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-11 22:14 ` Jan Hubicka @ 2015-11-11 22:31 ` Jan Hubicka 2015-11-22 23:19 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-11 22:31 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, Richard Biener, gcc-patches > > On 11/11/2015 10:21 AM, Richard Biener wrote: > > >On Tue, 10 Nov 2015, Jan Hubicka wrote: > > >>The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be > > >>called before we finish all merging and then it is more fine grained than what > > >>we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be > > >>different, but here we want them to be equal so we can match: > > >> > > >>struct aa { void *ptr;}; > > >>struct bb { int * ptr;}; > > >> > > >>Which is actually required for Fortran interoperability. > > > > Just curious, is this sort of thing documented anywhere? > > See http://www.j3-fortran.org/doc/year/10/10-007.pdf, section 15 (interoperability with C). > It defines that C_PTR is compatible with any C non-function pointer. .. and if you ask about GCC side documentation, I added testcases that should trigger if the Fortran interoperability breaks. I do not think we want to document the above compatibility explicitly, because in future we may want to have more fine grained TBAA for types that are not shared across fortran and C code (= most types in practice) Honza > > Honza > > > > > > Bernd ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-11 22:31 ` Jan Hubicka @ 2015-11-22 23:19 ` Jan Hubicka 2015-11-22 23:22 ` Eric Botcazou ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Jan Hubicka @ 2015-11-22 23:19 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, Richard Biener, gcc-patches Hi, here is updated patch which I finally comitted today. It addresses all the comments and also fixes one nasty bug that really cost me a lot of time to understand. + /* 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. */ This problem manifested itself only as a lto-bootstrap miscompare on 32bit build and I spent a lot of time localizing the wrong code since it reproduces only in quite large programs where we get conficts in canonical type merging like this. The patch thus updates record_component_aliases to substitute void_ptr_type for all pointer types. I re-did the stats. Now the improvement on dealII is 14% that is quite a bit lower than earlier, but still substantial. Since we have voidptr globing counter, I know that the number of disambiguations would go at least 19% up if we did not do it. THere is a lot of low hanging fruit in that area now, but the real solution is to track types that needs to be merge by fortran rules and don't do all this fancy globing for C/C++ types. I will open branch for IPA work and try to prepare this for next stage 1. bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested on i386-linux and also on some bigger apps, committed Note that we still have bootstrap miscompare with LTO build and --disable-checking, I am looking for that now. Additoinally after fixing the ICEs preventing us to build the gnat1 binary, gnat1 aborts. Both these are independent of the patch. Honza * lto.c (iterative_hash_canonical_type): Always recurse for pointers. (gimple_register_canonical_type_1): Check that pointers do not get canonical types. (gimple_register_canonical_type): Do not register pointers. * tree.c (build_pointer_type_for_mode,build_reference_type_for_mode): In LTO we do not compute TYPE_CANONICAL of pointers. (gimple_canonical_types_compatible_p): Improve coments; sanity check that pointers do not have canonical type that would make us believe they are different. * alias.c (get_alias_set): Do structural type equality on pointers; enable pointer path for LTO; also glob pointer to vector with pointer to vector element; glob pointers and references for LTO; do more strict sanity checking about build_pointer_type returning the canonical type which is also the main variant. (record_component_aliases): When component type is pointer and we do LTO; record void_type_node alias set. Index: tree.c =================================================================== --- tree.c (revision 230714) +++ tree.c (working copy) @@ -7919,7 +7919,8 @@ build_pointer_type_for_mode (tree to_typ TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (to_type); TYPE_POINTER_TO (to_type) = t; - if (TYPE_STRUCTURAL_EQUALITY_P (to_type)) + /* During LTO we do not set TYPE_CANONICAL of pointers and references. */ + if (TYPE_STRUCTURAL_EQUALITY_P (to_type) || in_lto_p) SET_TYPE_STRUCTURAL_EQUALITY (t); else if (TYPE_CANONICAL (to_type) != to_type || could_alias) TYPE_CANONICAL (t) @@ -7987,7 +7988,8 @@ build_reference_type_for_mode (tree to_t TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (to_type); TYPE_REFERENCE_TO (to_type) = t; - if (TYPE_STRUCTURAL_EQUALITY_P (to_type)) + /* During LTO we do not set TYPE_CANONICAL of pointers and references. */ + if (TYPE_STRUCTURAL_EQUALITY_P (to_type) || in_lto_p) SET_TYPE_STRUCTURAL_EQUALITY (t); else if (TYPE_CANONICAL (to_type) != to_type || could_alias) TYPE_CANONICAL (t) @@ -13224,7 +13226,9 @@ type_with_interoperable_signedness (cons TBAA is concerned. This function is used both by lto.c canonical type merging and by the verifier. If TRUST_TYPE_CANONICAL we do not look into structure of types - that have TYPE_CANONICAL defined and assume them equivalent. */ + that have TYPE_CANONICAL defined and assume them equivalent. This is useful + only for LTO because only in these cases TYPE_CANONICAL equivalence + correspond to one defined by gimple_canonical_types_compatible_p. */ bool gimple_canonical_types_compatible_p (const_tree t1, const_tree t2, @@ -13265,9 +13269,19 @@ gimple_canonical_types_compatible_p (con || (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. */ + if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) && trust_type_canonical) - return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); + { + /* Do not use TYPE_CANONICAL of pointer types. For LTO streamed types + they are always NULL, but they are set to non-NULL for types + constructed by build_pointer_type and variants. In this case the + TYPE_CANONICAL is more fine grained than the equivalnce we test (where + all pointers are considered equal. Be sure to not return false + negatives. */ + gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)); + return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); + } /* 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)); Index: lto/lto.c =================================================================== --- lto/lto.c (revision 230714) +++ lto/lto.c (working copy) @@ -388,8 +388,13 @@ iterative_hash_canonical_type (tree type /* All type variants have same TYPE_CANONICAL. */ type = TYPE_MAIN_VARIANT (type); + + /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing + code never use it anyway. */ + if (POINTER_TYPE_P (type)) + v = hash_canonical_type (type); /* An already processed type. */ - if (TYPE_CANONICAL (type)) + else if (TYPE_CANONICAL (type)) { type = TYPE_CANONICAL (type); v = gimple_canonical_type_hash (type); @@ -437,7 +442,9 @@ gimple_register_canonical_type_1 (tree t { void **slot; - gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t)); + gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) + && type_with_alias_set_p (t) + && !POINTER_TYPE_P (t)); slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); if (*slot) @@ -470,7 +477,7 @@ gimple_register_canonical_type_1 (tree t static void gimple_register_canonical_type (tree t) { - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t)) + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) return; /* Canonical types are same among all complete variants. */ Index: alias.c =================================================================== --- alias.c (revision 230714) +++ alias.c (working copy) @@ -869,13 +869,23 @@ get_alias_set (tree t) set = lang_hooks.get_alias_set (t); if (set != -1) return set; - return 0; + /* Handle structure type equality for pointer types. This is easy + to do, because the code bellow ignore canonical types on these anyway. + This is important for LTO, where TYPE_CANONICAL for pointers can not + be meaningfuly computed by the frotnend. */ + if (!POINTER_TYPE_P (t)) + { + /* In LTO we set canonical types for all types where it makes + sense to do so. Double check we did not miss some type. */ + gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); + return 0; + } + } + else + { + t = TYPE_CANONICAL (t); + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); } - - t = TYPE_CANONICAL (t); - - /* The canonical type should not require structural equality checks. */ - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); /* If this is a type with a known alias set, return it. */ if (TYPE_ALIAS_SET_KNOWN_P (t)) @@ -952,20 +962,23 @@ get_alias_set (tree t) ptr_type_node but that is a bad idea, because it prevents disabiguations in between pointers. For Firefox this accounts about 20% of all disambiguations in the program. */ - else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p) + else if (POINTER_TYPE_P (t) && t != ptr_type_node) { tree p; auto_vec <bool, 8> reference; /* Unnest all pointers and references. - We also want to make pointer to array equivalent to pointer to its - element. So skip all array types, too. */ + We also want to make pointer to array/vector equivalent to pointer to + its element (see the reasoning above). Skip all those types, too. */ for (p = t; POINTER_TYPE_P (p) - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)); + || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) + || TREE_CODE (p) == VECTOR_TYPE; p = TREE_TYPE (p)) { if (TREE_CODE (p) == REFERENCE_TYPE) - reference.safe_push (true); + /* In LTO we want languages that use references to be compatible + with languages that use pointers. */ + reference.safe_push (true && !in_lto_p); if (TREE_CODE (p) == POINTER_TYPE) reference.safe_push (false); } @@ -981,7 +994,7 @@ get_alias_set (tree t) set = get_alias_set (ptr_type_node); else { - /* Rebuild pointer type from starting from canonical types using + /* Rebuild pointer type starting from canonical types using unqualified pointers and references only. This way all such pointers will have the same alias set and will conflict with each other. @@ -998,9 +1011,15 @@ get_alias_set (tree t) p = build_reference_type (p); else p = build_pointer_type (p); - p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p)); + gcc_checking_assert (p == TYPE_MAIN_VARIANT (p)); + /* build_pointer_type should always return the canonical type. + For LTO TYPE_CANOINCAL may be NULL, because we do not compute + them. Be sure that frontends do not glob canonical types of + pointers in unexpected way and that p == TYPE_CANONICAL (p) + in all other cases. */ + gcc_checking_assert (!TYPE_CANONICAL (p) + || p == TYPE_CANONICAL (p)); } - gcc_checking_assert (TYPE_CANONICAL (p) == p); /* Assign the alias set to both p and t. We can not call get_alias_set (p) here as that would trigger @@ -1015,11 +1034,12 @@ get_alias_set (tree t) } } } - /* In LTO the rules above needs to be part of canonical type machinery. - For now just punt. */ - else if (POINTER_TYPE_P (t) - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); + /* Alias set of ptr_type_node is special and serve as universal pointer which + is TBAA compatible with every other pointer type. Be sure we have the + alias set built even for LTO which otherwise keeps all TYPE_CANONICAL + of pointer types NULL. */ + else if (t == ptr_type_node) + set = new_alias_set (); /* Otherwise make a new alias set for this type. */ else @@ -1155,7 +1175,42 @@ record_component_aliases (tree 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)) - record_alias_subset (superset, get_alias_set (TREE_TYPE (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 ((TREE_CODE (t) == ARRAY_TYPE + && (!COMPLETE_TYPE_P (t) + || TYPE_NONALIASED_COMPONENT (t))) + || TREE_CODE (t) == VECTOR_TYPE) + t = TREE_TYPE (t); + if (POINTER_TYPE_P (t)) + t = ptr_type_node; + } + + record_alias_subset (superset, get_alias_set (t)); + } break; case COMPLEX_TYPE: ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-22 23:19 ` Jan Hubicka @ 2015-11-22 23:22 ` Eric Botcazou 2015-11-23 1:20 ` Jan Hubicka 2015-11-23 10:39 ` Richard Biener 2015-11-23 13:52 ` Martin Jambor 2 siblings, 1 reply; 28+ messages in thread From: Eric Botcazou @ 2015-11-22 23:22 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Bernd Schmidt, Richard Biener > + 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 ((TREE_CODE (t) == ARRAY_TYPE > + && (!COMPLETE_TYPE_P (t) > + || TYPE_NONALIASED_COMPONENT (t))) > + || TREE_CODE (t) == VECTOR_TYPE) > + t = TREE_TYPE (t); > + if (POINTER_TYPE_P (t)) > + t = ptr_type_node; > + } > + > + record_alias_subset (superset, get_alias_set (t)); > + } > break; Are you sure that it's not !TYPE_NONALIASED_COMPONENT (t) here? -- Eric Botcazou ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-22 23:22 ` Eric Botcazou @ 2015-11-23 1:20 ` Jan Hubicka 2015-11-23 8:34 ` Eric Botcazou 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-23 1:20 UTC (permalink / raw) To: Eric Botcazou; +Cc: Jan Hubicka, gcc-patches, Bernd Schmidt, Richard Biener > > + 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 ((TREE_CODE (t) == ARRAY_TYPE > > + && (!COMPLETE_TYPE_P (t) > > + || TYPE_NONALIASED_COMPONENT (t))) > > + || TREE_CODE (t) == VECTOR_TYPE) > > + t = TREE_TYPE (t); > > + if (POINTER_TYPE_P (t)) > > + t = ptr_type_node; > > + } > > + > > + record_alias_subset (superset, get_alias_set (t)); > > + } > > break; > > Are you sure that it's not !TYPE_NONALIASED_COMPONENT (t) here? You are right, TYPE_NONALIASED_COMPONENT is the wrong way. I will fix it and try to come up with a testcase (TYPE_NONALIASED_COMPONENT is quite rarely used beast) Honza > > -- > Eric Botcazou ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 1:20 ` Jan Hubicka @ 2015-11-23 8:34 ` Eric Botcazou 2015-11-23 17:26 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Eric Botcazou @ 2015-11-23 8:34 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, Bernd Schmidt, Richard Biener > You are right, TYPE_NONALIASED_COMPONENT is the wrong way. I will fix it > and try to come up with a testcase (TYPE_NONALIASED_COMPONENT is quite > rarely used beast) It's only used in Ada as far as I know, but is quite sensitive and quickly leads to wrong code if not handled properly in my experience, so this could well be responsible for the gnat1 miscompilation. -- Eric Botcazou ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 8:34 ` Eric Botcazou @ 2015-11-23 17:26 ` Jan Hubicka 0 siblings, 0 replies; 28+ messages in thread From: Jan Hubicka @ 2015-11-23 17:26 UTC (permalink / raw) To: Eric Botcazou; +Cc: Jan Hubicka, gcc-patches, Bernd Schmidt, Richard Biener > > You are right, TYPE_NONALIASED_COMPONENT is the wrong way. I will fix it > > and try to come up with a testcase (TYPE_NONALIASED_COMPONENT is quite > > rarely used beast) > > It's only used in Ada as far as I know, but is quite sensitive and quickly > leads to wrong code if not handled properly in my experience, so this could > well be responsible for the gnat1 miscompilation. Build fialed same way before my patch. Moreover the problem can only happen on array of pointers that are type punned (i.e. using store like (void *[r])array = [NULL, NULL, NULL] and then accessing it as (int *[r]) array. I do not think C or Ada can produce code like that. I am re-testing with the fix to TYPE_NONALIASED_COMPONENT arrays I explained in other email. Perhaps that helps, or perhaps it is one of those Ada/C type puning glues getting miscompiled? Other Ada binaries (gnatbind) seems to work fine. I will post backtrace once my testing gets to the ICE again. Honza > > -- > Eric Botcazou ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-22 23:19 ` Jan Hubicka 2015-11-22 23:22 ` Eric Botcazou @ 2015-11-23 10:39 ` Richard Biener 2015-11-23 17:08 ` Jan Hubicka 2015-11-24 5:01 ` Jan Hubicka 2015-11-23 13:52 ` Martin Jambor 2 siblings, 2 replies; 28+ messages in thread From: Richard Biener @ 2015-11-23 10:39 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, gcc-patches On Mon, 23 Nov 2015, Jan Hubicka wrote: > Hi, > here is updated patch which I finally comitted today. It addresses all the comments > and also fixes one nasty bug that really cost me a lot of time to understand. > > + /* 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. */ > > This problem manifested itself only as a lto-bootstrap miscompare on 32bit > build and I spent a lot of time localizing the wrong code since it reproduces > only in quite large programs where we get conficts in canonical type merging > like this. > > The patch thus updates record_component_aliases to substitute > void_ptr_type for all pointer types. I re-did the stats. Now the > improvement on dealII is 14% that is quite a bit lower than earlier, but > still substantial. Since we have voidptr globing counter, I know that > the number of disambiguations would go at least 19% up if we did not do > it. Please in future leave patches for review again if you do such big changes before committing... I don't understand why we need this (testcase?) because get_alias_set () is supposed to do the alias-set of pointer globbing for us. Thanks, Richard. > THere is a lot of low hanging fruit in that area now, but the real > solution is to track types that needs to be merge by fortran rules and > don't do all this fancy globing for C/C++ types. I will open branch for > IPA work and try to prepare this for next stage 1. > > bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested on i386-linux > and also on some bigger apps, committed > > Note that we still have bootstrap miscompare with LTO build and --disable-checking, > I am looking for that now. Additoinally after fixing the ICEs preventing us to build > the gnat1 binary, gnat1 aborts. Both these are independent of the patch. > > Honza > * lto.c (iterative_hash_canonical_type): Always recurse for pointers. > (gimple_register_canonical_type_1): Check that pointers do not get > canonical types. > (gimple_register_canonical_type): Do not register pointers. > > * tree.c (build_pointer_type_for_mode,build_reference_type_for_mode): > In LTO we do not compute TYPE_CANONICAL of pointers. > (gimple_canonical_types_compatible_p): Improve coments; sanity check > that pointers do not have canonical type that would make us believe > they are different. > * alias.c (get_alias_set): Do structural type equality on pointers; > enable pointer path for LTO; also glob pointer to vector with pointer > to vector element; glob pointers and references for LTO; do more strict > sanity checking about build_pointer_type returning the canonical type > which is also the main variant. > (record_component_aliases): When component type is pointer and we > do LTO; record void_type_node alias set. > Index: tree.c > =================================================================== > --- tree.c (revision 230714) > +++ tree.c (working copy) > @@ -7919,7 +7919,8 @@ build_pointer_type_for_mode (tree to_typ > TYPE_NEXT_PTR_TO (t) = TYPE_POINTER_TO (to_type); > TYPE_POINTER_TO (to_type) = t; > > - if (TYPE_STRUCTURAL_EQUALITY_P (to_type)) > + /* During LTO we do not set TYPE_CANONICAL of pointers and references. */ > + if (TYPE_STRUCTURAL_EQUALITY_P (to_type) || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if (TYPE_CANONICAL (to_type) != to_type || could_alias) > TYPE_CANONICAL (t) > @@ -7987,7 +7988,8 @@ build_reference_type_for_mode (tree to_t > TYPE_NEXT_REF_TO (t) = TYPE_REFERENCE_TO (to_type); > TYPE_REFERENCE_TO (to_type) = t; > > - if (TYPE_STRUCTURAL_EQUALITY_P (to_type)) > + /* During LTO we do not set TYPE_CANONICAL of pointers and references. */ > + if (TYPE_STRUCTURAL_EQUALITY_P (to_type) || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if (TYPE_CANONICAL (to_type) != to_type || could_alias) > TYPE_CANONICAL (t) > @@ -13224,7 +13226,9 @@ type_with_interoperable_signedness (cons > TBAA is concerned. > This function is used both by lto.c canonical type merging and by the > verifier. If TRUST_TYPE_CANONICAL we do not look into structure of types > - that have TYPE_CANONICAL defined and assume them equivalent. */ > + that have TYPE_CANONICAL defined and assume them equivalent. This is useful > + only for LTO because only in these cases TYPE_CANONICAL equivalence > + correspond to one defined by gimple_canonical_types_compatible_p. */ > > bool > gimple_canonical_types_compatible_p (const_tree t1, const_tree t2, > @@ -13265,9 +13269,19 @@ gimple_canonical_types_compatible_p (con > || (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. */ > + > if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) > && trust_type_canonical) > - return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > + { > + /* Do not use TYPE_CANONICAL of pointer types. For LTO streamed types > + they are always NULL, but they are set to non-NULL for types > + constructed by build_pointer_type and variants. In this case the > + TYPE_CANONICAL is more fine grained than the equivalnce we test (where > + all pointers are considered equal. Be sure to not return false > + negatives. */ > + gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)); > + return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > + } > > /* 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)); > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 230714) > +++ lto/lto.c (working copy) > @@ -388,8 +388,13 @@ iterative_hash_canonical_type (tree type > > /* All type variants have same TYPE_CANONICAL. */ > type = TYPE_MAIN_VARIANT (type); > + > + /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing > + code never use it anyway. */ > + if (POINTER_TYPE_P (type)) > + v = hash_canonical_type (type); > /* An already processed type. */ > - if (TYPE_CANONICAL (type)) > + else if (TYPE_CANONICAL (type)) > { > type = TYPE_CANONICAL (type); > v = gimple_canonical_type_hash (type); > @@ -437,7 +442,9 @@ gimple_register_canonical_type_1 (tree t > { > void **slot; > > - gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t)); > + gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) > + && type_with_alias_set_p (t) > + && !POINTER_TYPE_P (t)); > > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > @@ -470,7 +477,7 @@ gimple_register_canonical_type_1 (tree t > static void > gimple_register_canonical_type (tree t) > { > - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t)) > + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) > return; > > /* Canonical types are same among all complete variants. */ > Index: alias.c > =================================================================== > --- alias.c (revision 230714) > +++ alias.c (working copy) > @@ -869,13 +869,23 @@ get_alias_set (tree t) > set = lang_hooks.get_alias_set (t); > if (set != -1) > return set; > - return 0; > + /* Handle structure type equality for pointer types. This is easy > + to do, because the code bellow ignore canonical types on these anyway. > + This is important for LTO, where TYPE_CANONICAL for pointers can not > + be meaningfuly computed by the frotnend. */ > + if (!POINTER_TYPE_P (t)) > + { > + /* In LTO we set canonical types for all types where it makes > + sense to do so. Double check we did not miss some type. */ > + gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > + return 0; > + } > + } > + else > + { > + t = TYPE_CANONICAL (t); > + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > } > - > - t = TYPE_CANONICAL (t); > - > - /* The canonical type should not require structural equality checks. */ > - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > > /* If this is a type with a known alias set, return it. */ > if (TYPE_ALIAS_SET_KNOWN_P (t)) > @@ -952,20 +962,23 @@ get_alias_set (tree t) > ptr_type_node but that is a bad idea, because it prevents disabiguations > in between pointers. For Firefox this accounts about 20% of all > disambiguations in the program. */ > - else if (POINTER_TYPE_P (t) && t != ptr_type_node && !in_lto_p) > + else if (POINTER_TYPE_P (t) && t != ptr_type_node) > { > tree p; > auto_vec <bool, 8> reference; > > /* Unnest all pointers and references. > - We also want to make pointer to array equivalent to pointer to its > - element. So skip all array types, too. */ > + We also want to make pointer to array/vector equivalent to pointer to > + its element (see the reasoning above). Skip all those types, too. */ > for (p = t; POINTER_TYPE_P (p) > - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)); > + || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) > + || TREE_CODE (p) == VECTOR_TYPE; > p = TREE_TYPE (p)) > { > if (TREE_CODE (p) == REFERENCE_TYPE) > - reference.safe_push (true); > + /* In LTO we want languages that use references to be compatible > + with languages that use pointers. */ > + reference.safe_push (true && !in_lto_p); > if (TREE_CODE (p) == POINTER_TYPE) > reference.safe_push (false); > } > @@ -981,7 +994,7 @@ get_alias_set (tree t) > set = get_alias_set (ptr_type_node); > else > { > - /* Rebuild pointer type from starting from canonical types using > + /* Rebuild pointer type starting from canonical types using > unqualified pointers and references only. This way all such > pointers will have the same alias set and will conflict with > each other. > @@ -998,9 +1011,15 @@ get_alias_set (tree t) > p = build_reference_type (p); > else > p = build_pointer_type (p); > - p = TYPE_CANONICAL (TYPE_MAIN_VARIANT (p)); > + gcc_checking_assert (p == TYPE_MAIN_VARIANT (p)); > + /* build_pointer_type should always return the canonical type. > + For LTO TYPE_CANOINCAL may be NULL, because we do not compute > + them. Be sure that frontends do not glob canonical types of > + pointers in unexpected way and that p == TYPE_CANONICAL (p) > + in all other cases. */ > + gcc_checking_assert (!TYPE_CANONICAL (p) > + || p == TYPE_CANONICAL (p)); > } > - gcc_checking_assert (TYPE_CANONICAL (p) == p); > > /* Assign the alias set to both p and t. > We can not call get_alias_set (p) here as that would trigger > @@ -1015,11 +1034,12 @@ get_alias_set (tree t) > } > } > } > - /* In LTO the rules above needs to be part of canonical type machinery. > - For now just punt. */ > - else if (POINTER_TYPE_P (t) > - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) > - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); > + /* Alias set of ptr_type_node is special and serve as universal pointer which > + is TBAA compatible with every other pointer type. Be sure we have the > + alias set built even for LTO which otherwise keeps all TYPE_CANONICAL > + of pointer types NULL. */ > + else if (t == ptr_type_node) > + set = new_alias_set (); > > /* Otherwise make a new alias set for this type. */ > else > @@ -1155,7 +1175,42 @@ record_component_aliases (tree 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)) > - record_alias_subset (superset, get_alias_set (TREE_TYPE (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 ((TREE_CODE (t) == ARRAY_TYPE > + && (!COMPLETE_TYPE_P (t) > + || TYPE_NONALIASED_COMPONENT (t))) > + || TREE_CODE (t) == VECTOR_TYPE) > + t = TREE_TYPE (t); > + if (POINTER_TYPE_P (t)) > + t = ptr_type_node; > + } > + > + record_alias_subset (superset, get_alias_set (t)); > + } > break; > > case COMPLEX_TYPE: > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 10:39 ` Richard Biener @ 2015-11-23 17:08 ` Jan Hubicka 2015-11-24 8:34 ` Richard Biener 2015-11-24 5:01 ` Jan Hubicka 1 sibling, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-23 17:08 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, Bernd Schmidt, gcc-patches > > Please in future leave patches for review again if you do such > big changes before committing... Uhm, sorry, next time I will be more cureful. It seemed rather easy after debugging it but indeed it is somewhat surprising issue. > > I don't understand why we need this (testcase?) because get_alias_set () > is supposed to do the alias-set of pointer globbing for us. The situation is as follows. You can have struct a { int *ptr; } struct b { float *ptr; } Now, if becase type is ignored by TYPE_CANONICAL, we have TYPE_CANONICAL (sturct b) = struct a. and while building alias set of "struct a" we record compoents as: struct a ^ | int * Now do struct b b = {NULL}; float *p=&b->ptr; *p=nonnull; return b.ptr; Now alias set of the initialization is alias set of "struct a". The alias set of of the pointer store is "float *". We ask alias oracle if "struct a" can conflict with "float *" and answer is no, because "int *" (component of struct b) and "float *" are not conflicting. With the change we record component alias as follows: struct a ^ | void * which makes the answer to be correct, because "int *" conflicts with all pointers, so all such queries about a structure gimple_canonical_types_compatible with "struct a" will be handled correctly. I will add aritifical testcase for this after double checking that the code above miscompiles without that conditional. I found that having warning about TBAA incompatibility when doing merigng in lto-symtab is great to catch issues like this. I had this patch for quite some time, but it was producing obviously wrong positives (in Fortran, Ada and also sometimes in Firefox on array initializes of style int a[]={1,2,3}). I wrongly assumed tha the bug is because we compute TYPE_CANONICAL sensitively to array size and there are permited transitions of array sizes between units. THis is not the case. Yesterday I found that the problem is with interaction of get_alias_set globbing and gimple_canonical_types_compatible globbing. We can't have get_alias_set globbing more or less coarse than gimple_canonical_types_compatible does. Here the situation is reverse to the case above : because array type is inherited from element type, we can't have TYPE_CANONICAL more globbed for array than we have get_alias_set. In this case the problem appeared when TYPE_NONALIASED_COMPONENT array previaled in type merging other arrays. The situation got worse with pointer, becuase array of pointers of one type could prevail array of pointers of other type and then the array type gets different alias sets in different units. This seems to work for C programs, but is wrong. I will send patch and separate explanation after re-testing final version shortly. Honza ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 17:08 ` Jan Hubicka @ 2015-11-24 8:34 ` Richard Biener 2015-11-24 19:05 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Richard Biener @ 2015-11-24 8:34 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, gcc-patches On Mon, 23 Nov 2015, Jan Hubicka wrote: > > > > Please in future leave patches for review again if you do such > > big changes before committing... > > Uhm, sorry, next time I will be more cureful. It seemed rather easy after > debugging it but indeed it is somewhat surprising issue. > > > > I don't understand why we need this (testcase?) because get_alias_set () > > is supposed to do the alias-set of pointer globbing for us. > > The situation is as follows. You can have > > struct a { > int *ptr; > } > > struct b { > float *ptr; > } > > Now, if becase type is ignored by TYPE_CANONICAL, we have > > TYPE_CANONICAL (sturct b) = struct a. > > and while building alias set of "struct a" we record compoents as: > > struct a > ^ > | > int * > > Now do > > struct b b = {NULL}; > float *p=&b->ptr; > *p=nonnull; > return b.ptr; > > Now alias set of the initialization is alias set of "struct a". The alias set > of of the pointer store is "float *". We ask alias oracle if "struct a" can > conflict with "float *" and answer is no, because "int *" (component of struct > b) and "float *" are not conflicting. With the change we record component > alias as follows: Ah, with my original pointer globbing I side-stepped this. So are you _really_ sure that we want to handle int * different from float *? Because that makes the situation much more complicated in these cases. > > struct a > ^ > | > void * > > which makes the answer to be correct, because "int *" conflicts with all > pointers, so all such queries about a structure > gimple_canonical_types_compatible with "struct a" will be handled > correctly. > > I will add aritifical testcase for this after double checking that the code above > miscompiles without that conditional. > > I found that having warning about TBAA incompatibility when doing merigng in > lto-symtab is great to catch issues like this. > > I had this patch for quite some time, but it was producing obviously wrong > positives (in Fortran, Ada and also sometimes in Firefox on array initializes > of style int a[]={1,2,3}). I wrongly assumed tha the bug is because we compute > TYPE_CANONICAL sensitively to array size and there are permited transitions > of array sizes between units. THis is not the case. > > Yesterday I found that the problem is with interaction of get_alias_set > globbing and gimple_canonical_types_compatible globbing. We can't have > get_alias_set globbing more or less coarse than > gimple_canonical_types_compatible does. > > Here the situation is reverse to the case above : because array type is > inherited from element type, we can't have TYPE_CANONICAL more globbed for > array than we have get_alias_set. In this case the problem appeared when > TYPE_NONALIASED_COMPONENT array previaled in type merging other arrays. The > situation got worse with pointer, becuase array of pointers of one type could > prevail array of pointers of other type and then the array type gets different > alias sets in different units. This seems to work for C programs, but is > wrong. I will send patch and separate explanation after re-testing final > version shortly. I feel we need to document all this somewhere. Richard. > Honza > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-24 8:34 ` Richard Biener @ 2015-11-24 19:05 ` Jan Hubicka 2015-11-25 10:49 ` Richard Biener 0 siblings, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-24 19:05 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, Bernd Schmidt, gcc-patches > > Now alias set of the initialization is alias set of "struct a". The alias set > > of of the pointer store is "float *". We ask alias oracle if "struct a" can > > conflict with "float *" and answer is no, because "int *" (component of struct > > b) and "float *" are not conflicting. With the change we record component > > alias as follows: > > Ah, with my original pointer globbing I side-stepped this. So are > you _really_ sure that we want to handle int * different from float *? > Because that makes the situation much more complicated in these cases. Yep, keeping track of different pointer types in C++ is really important and moreover the old globbing was not able to deal with C/Fortran compatibility fun anyway. Average higher level C++ code has dozen of differently typed containers for random stuff and shuffles the pointers around them in memory. Without being able to track down pointers stored in memory there is no hope to optimize the code. This very basic pointer LTO reduced rodata cc1 binary by 7% which I think are from 99% the removed sanity checking __FUNCTION__ locators. Globing all pointers to void * is just part of the problem anyway. The previous code did not implemented Fortran/C interoperability correctly anyway. I hope that with these changes we have infrastructure robust and flexible enough to model more precisely what we really want. > > Here the situation is reverse to the case above : because array type is > > inherited from element type, we can't have TYPE_CANONICAL more globbed for > > array than we have get_alias_set. In this case the problem appeared when > > TYPE_NONALIASED_COMPONENT array previaled in type merging other arrays. The > > situation got worse with pointer, becuase array of pointers of one type could > > prevail array of pointers of other type and then the array type gets different > > alias sets in different units. This seems to work for C programs, but is > > wrong. I will send patch and separate explanation after re-testing final > > version shortly. > > I feel we need to document all this somewhere. Yeah, that would be a good idea. Probably comment at beggining of alias.c? I was thinking of moving the code to one place, perhaps moving gimple_canonical_type from tree.c to alias.c would make sense, or inventing new place for this. Moreover I was thinking of extending type verifier to check that all components of a type alias with the whole tihng, so we do not run into bugs like this again. Together with the TBAA checking in lto-symtab.c it should make it pretty hard to make nasty bugs like this. Thanks, Honza > > Richard. > > > Honza > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-24 19:05 ` Jan Hubicka @ 2015-11-25 10:49 ` Richard Biener 2015-11-25 18:35 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Richard Biener @ 2015-11-25 10:49 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, gcc-patches On Tue, 24 Nov 2015, Jan Hubicka wrote: > > > Now alias set of the initialization is alias set of "struct a". The alias set > > > of of the pointer store is "float *". We ask alias oracle if "struct a" can > > > conflict with "float *" and answer is no, because "int *" (component of struct > > > b) and "float *" are not conflicting. With the change we record component > > > alias as follows: > > > > Ah, with my original pointer globbing I side-stepped this. So are > > you _really_ sure that we want to handle int * different from float *? > > Because that makes the situation much more complicated in these cases. > > Yep, keeping track of different pointer types in C++ is really important > and moreover the old globbing was not able to deal with C/Fortran compatibility > fun anyway. > > Average higher level C++ code has dozen of differently typed containers for > random stuff and shuffles the pointers around them in memory. Without being > able to track down pointers stored in memory there is no hope to optimize the > code. > > This very basic pointer LTO reduced rodata cc1 binary by 7% which I think are > from 99% the removed sanity checking __FUNCTION__ locators. > > Globing all pointers to void * is just part of the problem anyway. The previous > code did not implemented Fortran/C interoperability correctly anyway. I hope > that with these changes we have infrastructure robust and flexible enough > to model more precisely what we really want. > > > > Here the situation is reverse to the case above : because array type is > > > inherited from element type, we can't have TYPE_CANONICAL more globbed for > > > array than we have get_alias_set. In this case the problem appeared when > > > TYPE_NONALIASED_COMPONENT array previaled in type merging other arrays. The > > > situation got worse with pointer, becuase array of pointers of one type could > > > prevail array of pointers of other type and then the array type gets different > > > alias sets in different units. This seems to work for C programs, but is > > > wrong. I will send patch and separate explanation after re-testing final > > > version shortly. > > > > I feel we need to document all this somewhere. > > Yeah, that would be a good idea. Probably comment at beggining of alias.c? Yes, that sounds good. > I was thinking of moving the code to one place, perhaps moving > gimple_canonical_type from tree.c to alias.c would make sense, or inventing > new place for this. Didn't like tree.c for this anyway and yes, alias.c sounds better given its connection to alias. > Moreover I was thinking of extending type verifier to check that all > components of a type alias with the whole tihng, so we do not run into > bugs like this again. Together with the TBAA checking in lto-symtab.c it > should make it pretty hard to make nasty bugs like this. Might be a bit expensive though. It also reminds me of all the ICEs we have PRs for WRT the type verifier. Remember we are in stage3 now so rather than introducing new issues you should spend some time fixing the existing ones you caused ;)) (just look for bugs CCed to hubicka@gcc.gnu.org) Thanks, Richard. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-25 10:49 ` Richard Biener @ 2015-11-25 18:35 ` Jan Hubicka 0 siblings, 0 replies; 28+ messages in thread From: Jan Hubicka @ 2015-11-25 18:35 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, Bernd Schmidt, gcc-patches > > Might be a bit expensive though. It also reminds me of all the > ICEs we have PRs for WRT the type verifier. Remember we are in stage3 > now so rather than introducing new issues you should spend some time > fixing the existing ones you caused ;)) (just look for bugs > CCed to hubicka@gcc.gnu.org) Yep, I already fixed some of these, will push out patches today. Honza > > Thanks, > Richard. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 10:39 ` Richard Biener 2015-11-23 17:08 ` Jan Hubicka @ 2015-11-24 5:01 ` Jan Hubicka 1 sibling, 0 replies; 28+ messages in thread From: Jan Hubicka @ 2015-11-24 5:01 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, Bernd Schmidt, gcc-patches > I don't understand why we need this (testcase?) because get_alias_set () > is supposed to do the alias-set of pointer globbing for us. Hi, After some experimentation I managed to derive self contained testcase (other than GCC itself). struct a/b/c gets the same TYPE_CANONICAL which is different from struct b. Without that change in recording component aliases then the alias_set of struct b has component float * instead of int *. Eventually in main we optimize out the store to int * because we do not see the conflict. One needs to add the extra garbage around to be sure that things are streamed in proper order and also in proper order shipped to ltrans and finally the gimple optimizers are not smart enough to get the propagation without TBAA oracle help. Comitted. * gcc.c-torture/execute/lto-tbaa-1.c: New testcase. Index: gcc.c-torture/execute/lto-tbaa-1.c =================================================================== --- gcc.c-torture/execute/lto-tbaa-1.c (revision 0) +++ gcc.c-torture/execute/lto-tbaa-1.c (revision 0) @@ -0,0 +1,42 @@ +/* { dg-additional-options "-fno-early-inlining -fno-ipa-cp" } */ +struct a { + float *b; +} *a; +struct b { + int *b; +} b; +struct c { + float *b; +} *c; +int d; +use_a (struct a *a) +{ +} +set_b (int **a) +{ + *a=&d; +} +use_c (struct c *a) +{ +} +__attribute__ ((noinline)) int **retme(int **val) +{ + return val; +} +int e; +struct b b= {&e}; +struct b b2; +struct b b3; +int **ptr = &b2.b; +main () +{ + a= (void *)0; + b.b=&e; + ptr =retme ( &b.b); + set_b (ptr); + b3=b; + if (b3.b != &d) + __builtin_abort (); + c= (void *)0; + return 0; +} ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-22 23:19 ` Jan Hubicka 2015-11-22 23:22 ` Eric Botcazou 2015-11-23 10:39 ` Richard Biener @ 2015-11-23 13:52 ` Martin Jambor 2015-11-23 15:33 ` Richard Biener 2 siblings, 1 reply; 28+ messages in thread From: Martin Jambor @ 2015-11-23 13:52 UTC (permalink / raw) To: Jan Hubicka; +Cc: Bernd Schmidt, Richard Biener, gcc-patches Hi, On Mon, Nov 23, 2015 at 12:00:25AM +0100, Jan Hubicka wrote: > Hi, > here is updated patch which I finally comitted today. It addresses all the comments > and also fixes one nasty bug that really cost me a lot of time to understand. > > + /* 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. */ > > This problem manifested itself only as a lto-bootstrap miscompare on 32bit > build and I spent a lot of time localizing the wrong code since it reproduces > only in quite large programs where we get conficts in canonical type merging > like this. > > The patch thus updates record_component_aliases to substitute void_ptr_type for > all pointer types. I re-did the stats. Now the improvement on dealII is 14% > that is quite a bit lower than earlier, but still substantial. Since we have > voidptr globing counter, I know that the number of disambiguations would go at > least 19% up if we did not do it. > > THere is a lot of low hanging fruit in that area now, but the real solution is to > track types that needs to be merge by fortran rules and don't do all this fancy > globing for C/C++ types. I will open branch for IPA work and try to prepare this > for next stage 1. > > bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested on i386-linux > and also on some bigger apps, committed > > Note that we still have bootstrap miscompare with LTO build and --disable-checking, > I am looking for that now. Additoinally after fixing the ICEs preventing us to build > the gnat1 binary, gnat1 aborts. Both these are independent of the patch. > > Honza > * lto.c (iterative_hash_canonical_type): Always recurse for pointers. > (gimple_register_canonical_type_1): Check that pointers do not get > canonical types. > (gimple_register_canonical_type): Do not register pointers. > > * tree.c (build_pointer_type_for_mode,build_reference_type_for_mode): > In LTO we do not compute TYPE_CANONICAL of pointers. > (gimple_canonical_types_compatible_p): Improve coments; sanity check > that pointers do not have canonical type that would make us believe > they are different. > * alias.c (get_alias_set): Do structural type equality on pointers; > enable pointer path for LTO; also glob pointer to vector with pointer > to vector element; glob pointers and references for LTO; do more strict > sanity checking about build_pointer_type returning the canonical type > which is also the main variant. > (record_component_aliases): When component type is pointer and we > do LTO; record void_type_node alias set. ... > Index: alias.c > =================================================================== > --- alias.c (revision 230714) > +++ alias.c (working copy) > @@ -869,13 +869,23 @@ get_alias_set (tree t) > set = lang_hooks.get_alias_set (t); > if (set != -1) > return set; > - return 0; > + /* Handle structure type equality for pointer types. This is easy > + to do, because the code bellow ignore canonical types on these anyway. > + This is important for LTO, where TYPE_CANONICAL for pointers can not > + be meaningfuly computed by the frotnend. */ > + if (!POINTER_TYPE_P (t)) > + { > + /* In LTO we set canonical types for all types where it makes > + sense to do so. Double check we did not miss some type. */ > + gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > + return 0; I have hit this assert on our LTO tests when doing a merge from trunk to the HSA branch. On the branch, we generate very simple static constructors/destructors which just call libgomp (un)registration routines to which we pass data in static variables of artificial types. The assert happens inside varpool_node::finalize_decl calls on those variables, e.g.: lto1: internal compiler error: in get_alias_set, at alias.c:880 0x613650 get_alias_set(tree_node*) /home/mjambor/gcc/branch/src/gcc/alias.c:880 0x71d2c7 set_mem_attributes_minus_bitpos(rtx_def*, tree_node*, int, long) /home/mjambor/gcc/branch/src/gcc/emit-rtl.c:1772 0xd2d2f0 make_decl_rtl(tree_node*) /home/mjambor/gcc/branch/src/gcc/varasm.c:1473 0xd310c7 assemble_variable(tree_node*, int, int, int) /home/mjambor/gcc/branch/src/gcc/varasm.c:2144 0xd37b32 varpool_node::assemble_decl() /home/mjambor/gcc/branch/src/gcc/varpool.c:580 0x6896aa varpool_node::finalize_decl(tree_node*) /home/mjambor/gcc/branch/src/gcc/cgraphunit.c:820 0x844da1 hsa_output_kernels /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:1954 0x849f04 hsa_output_libgomp_mapping /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2116 0x849f04 hsa_output_brig() /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2356 (hsa_output_brig is called from compile_file in toplev.c) I am going to commit the following hunk to the branch so that I can get on with the merge. If you think it is wrong in any way, please let me know so that we can find a proper solution. Thanks, Martin --- /home/mjambor/gcc/trunk/src/gcc/alias.c 2015-11-23 11:45:27.850846512 +0100 +++ gcc/alias.c 2015-11-23 14:37:32.098133073 +0100 @@ -877,7 +877,9 @@ get_alias_set (tree t) { /* In LTO we set canonical types for all types where it makes sense to do so. Double check we did not miss some type. */ - gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); + gcc_checking_assert (!in_lto_p + || !type_with_alias_set_p (t) + || TYPE_ARTIFICIAL (t)); return 0; } } > + } > + } > + else > + { > + t = TYPE_CANONICAL (t); > + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > } > - > - t = TYPE_CANONICAL (t); > - > - /* The canonical type should not require structural equality checks. */ > - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t)); > > /* If this is a type with a known alias set, return it. */ > if (TYPE_ALIAS_SET_KNOWN_P (t)) ... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 13:52 ` Martin Jambor @ 2015-11-23 15:33 ` Richard Biener 2015-11-23 15:35 ` Ilya Verbin 2015-11-23 16:52 ` Jan Hubicka 0 siblings, 2 replies; 28+ messages in thread From: Richard Biener @ 2015-11-23 15:33 UTC (permalink / raw) To: Martin Jambor; +Cc: Jan Hubicka, Bernd Schmidt, gcc-patches On Mon, 23 Nov 2015, Martin Jambor wrote: > Hi, > > On Mon, Nov 23, 2015 at 12:00:25AM +0100, Jan Hubicka wrote: > > Hi, > > here is updated patch which I finally comitted today. It addresses all the comments > > and also fixes one nasty bug that really cost me a lot of time to understand. > > > > + /* 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. */ > > > > This problem manifested itself only as a lto-bootstrap miscompare on 32bit > > build and I spent a lot of time localizing the wrong code since it reproduces > > only in quite large programs where we get conficts in canonical type merging > > like this. > > > > The patch thus updates record_component_aliases to substitute void_ptr_type for > > all pointer types. I re-did the stats. Now the improvement on dealII is 14% > > that is quite a bit lower than earlier, but still substantial. Since we have > > voidptr globing counter, I know that the number of disambiguations would go at > > least 19% up if we did not do it. > > > > THere is a lot of low hanging fruit in that area now, but the real solution is to > > track types that needs to be merge by fortran rules and don't do all this fancy > > globing for C/C++ types. I will open branch for IPA work and try to prepare this > > for next stage 1. > > > > bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested on i386-linux > > and also on some bigger apps, committed > > > > Note that we still have bootstrap miscompare with LTO build and --disable-checking, > > I am looking for that now. Additoinally after fixing the ICEs preventing us to build > > the gnat1 binary, gnat1 aborts. Both these are independent of the patch. > > > > Honza > > * lto.c (iterative_hash_canonical_type): Always recurse for pointers. > > (gimple_register_canonical_type_1): Check that pointers do not get > > canonical types. > > (gimple_register_canonical_type): Do not register pointers. > > > > * tree.c (build_pointer_type_for_mode,build_reference_type_for_mode): > > In LTO we do not compute TYPE_CANONICAL of pointers. > > (gimple_canonical_types_compatible_p): Improve coments; sanity check > > that pointers do not have canonical type that would make us believe > > they are different. > > * alias.c (get_alias_set): Do structural type equality on pointers; > > enable pointer path for LTO; also glob pointer to vector with pointer > > to vector element; glob pointers and references for LTO; do more strict > > sanity checking about build_pointer_type returning the canonical type > > which is also the main variant. > > (record_component_aliases): When component type is pointer and we > > do LTO; record void_type_node alias set. > > ... > > > Index: alias.c > > =================================================================== > > --- alias.c (revision 230714) > > +++ alias.c (working copy) > > @@ -869,13 +869,23 @@ get_alias_set (tree t) > > set = lang_hooks.get_alias_set (t); > > if (set != -1) > > return set; > > - return 0; > > + /* Handle structure type equality for pointer types. This is easy > > + to do, because the code bellow ignore canonical types on these anyway. > > + This is important for LTO, where TYPE_CANONICAL for pointers can not > > + be meaningfuly computed by the frotnend. */ > > + if (!POINTER_TYPE_P (t)) > > + { > > + /* In LTO we set canonical types for all types where it makes > > + sense to do so. Double check we did not miss some type. */ > > + gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > > + return 0; > > I have hit this assert on our LTO tests when doing a merge from trunk > to the HSA branch. On the branch, we generate very simple static > constructors/destructors which just call libgomp (un)registration > routines to which we pass data in static variables of artificial > types. The assert happens inside varpool_node::finalize_decl calls on > those variables, e.g.: > > lto1: internal compiler error: in get_alias_set, at alias.c:880 > 0x613650 get_alias_set(tree_node*) > /home/mjambor/gcc/branch/src/gcc/alias.c:880 > 0x71d2c7 set_mem_attributes_minus_bitpos(rtx_def*, tree_node*, int, long) > /home/mjambor/gcc/branch/src/gcc/emit-rtl.c:1772 > 0xd2d2f0 make_decl_rtl(tree_node*) > /home/mjambor/gcc/branch/src/gcc/varasm.c:1473 > 0xd310c7 assemble_variable(tree_node*, int, int, int) > /home/mjambor/gcc/branch/src/gcc/varasm.c:2144 > 0xd37b32 varpool_node::assemble_decl() > /home/mjambor/gcc/branch/src/gcc/varpool.c:580 > 0x6896aa varpool_node::finalize_decl(tree_node*) > /home/mjambor/gcc/branch/src/gcc/cgraphunit.c:820 > 0x844da1 hsa_output_kernels > /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:1954 > 0x849f04 hsa_output_libgomp_mapping > /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2116 > 0x849f04 hsa_output_brig() > /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2356 > (hsa_output_brig is called from compile_file in toplev.c) I think it also causes the following and one related ICE FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal compiler error) /space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: internal compiler error: in get_alias_set, at alias.c:880^M 0x7528a7 get_alias_set(tree_node*)^M /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M 0x751ce5 component_uses_parent_alias_set_from(tree_node const*)^M /space/rguenther/src/svn/trunk3/gcc/alias.c:635^M 0x7522ad reference_alias_ptr_type_1^M /space/rguenther/src/svn/trunk3/gcc/alias.c:747^M 0x752683 get_alias_set(tree_node*)^M ... Richard. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 15:33 ` Richard Biener @ 2015-11-23 15:35 ` Ilya Verbin 2015-11-27 8:24 ` Thomas Schwinge 2015-11-23 16:52 ` Jan Hubicka 1 sibling, 1 reply; 28+ messages in thread From: Ilya Verbin @ 2015-11-23 15:35 UTC (permalink / raw) To: Richard Biener, Jan Hubicka Cc: Martin Jambor, Bernd Schmidt, gcc-patches, Kirill Yukhin, Thomas Schwinge On Mon, Nov 23, 2015 at 16:31:42 +0100, Richard Biener wrote: > I think it also causes the following and one related ICE > > FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal compiler > error) > > /space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > internal compiler error: in get_alias_set, at alias.c:880^M > 0x7528a7 get_alias_set(tree_node*)^M > /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > 0x751ce5 component_uses_parent_alias_set_from(tree_node const*)^M > /space/rguenther/src/svn/trunk3/gcc/alias.c:635^M > 0x7522ad reference_alias_ptr_type_1^M > /space/rguenther/src/svn/trunk3/gcc/alias.c:747^M > 0x752683 get_alias_set(tree_node*)^M > ... And an ICE in intelmicemul offloading compiler: FAIL: libgomp.c++/for-11.C (internal compiler error) FAIL: libgomp.c++/for-13.C (internal compiler error) FAIL: libgomp.c++/for-14.C (internal compiler error) FAIL: libgomp.c/for-3.c (internal compiler error) FAIL: libgomp.c/for-5.c (internal compiler error) FAIL: libgomp.c/for-6.c (internal compiler error) libgomp/testsuite/libgomp.c/for-2.h:201:9: internal compiler error: in get_alias_set, at alias.c:880 0x710eef get_alias_set(tree_node*) gcc/alias.c:880 0x71032d component_uses_parent_alias_set_from(tree_node const*) gcc/alias.c:635 0x7108f5 reference_alias_ptr_type_1 gcc/alias.c:747 0x710ccb get_alias_set(tree_node*) gcc/alias.c:843 0x89d208 expand_assignment(tree_node*, tree_node*, bool) gcc/expr.c:5020 0x768ff7 expand_gimple_stmt_1 gcc/cfgexpand.c:3592 0x7693e2 expand_gimple_stmt gcc/cfgexpand.c:3688 0x7704ed expand_gimple_basic_block gcc/cfgexpand.c:5694 0x771ff1 execute gcc/cfgexpand.c:6309 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <http://gcc.gnu.org/bugs.html> for instructions. -- Ilya ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 15:35 ` Ilya Verbin @ 2015-11-27 8:24 ` Thomas Schwinge 0 siblings, 0 replies; 28+ messages in thread From: Thomas Schwinge @ 2015-11-27 8:24 UTC (permalink / raw) To: Ilya Verbin, Richard Biener, Jan Hubicka Cc: Martin Jambor, Bernd Schmidt, gcc-patches, Kirill Yukhin Hi! On Mon, 23 Nov 2015 18:34:30 +0300, Ilya Verbin <iverbin@gmail.com> wrote: > On Mon, Nov 23, 2015 at 16:31:42 +0100, Richard Biener wrote: > > I think it also causes the following and one related ICE > > > > FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal compiler > > error) > > > > /space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > > internal compiler error: in get_alias_set, at alias.c:880^M > > 0x7528a7 get_alias_set(tree_node*)^M > > /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > > 0x751ce5 component_uses_parent_alias_set_from(tree_node const*)^M > > /space/rguenther/src/svn/trunk3/gcc/alias.c:635^M > > 0x7522ad reference_alias_ptr_type_1^M > > /space/rguenther/src/svn/trunk3/gcc/alias.c:747^M > > 0x752683 get_alias_set(tree_node*)^M > > ... > > And an ICE in intelmicemul offloading compiler: > > FAIL: libgomp.c++/for-11.C (internal compiler error) > FAIL: libgomp.c++/for-13.C (internal compiler error) > FAIL: libgomp.c++/for-14.C (internal compiler error) > FAIL: libgomp.c/for-3.c (internal compiler error) > FAIL: libgomp.c/for-5.c (internal compiler error) > FAIL: libgomp.c/for-6.c (internal compiler error) > > libgomp/testsuite/libgomp.c/for-2.h:201:9: internal compiler error: in get_alias_set, at alias.c:880 > 0x710eef get_alias_set(tree_node*) > gcc/alias.c:880 > 0x71032d component_uses_parent_alias_set_from(tree_node const*) > gcc/alias.c:635 > 0x7108f5 reference_alias_ptr_type_1 > gcc/alias.c:747 > 0x710ccb get_alias_set(tree_node*) > gcc/alias.c:843 > 0x89d208 expand_assignment(tree_node*, tree_node*, bool) > gcc/expr.c:5020 > 0x768ff7 expand_gimple_stmt_1 > gcc/cfgexpand.c:3592 > 0x7693e2 expand_gimple_stmt > gcc/cfgexpand.c:3688 > 0x7704ed expand_gimple_basic_block > gcc/cfgexpand.c:5694 > 0x771ff1 execute > gcc/cfgexpand.c:6309 Belatedly confirmed, and also confirmed fixed with trunk r230835, <http://news.gmane.org/find-root.php?message_id=%3C20151124061000.GA4494%40kam.mff.cuni.cz%3E>. Grüße Thomas ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 15:33 ` Richard Biener 2015-11-23 15:35 ` Ilya Verbin @ 2015-11-23 16:52 ` Jan Hubicka 2015-11-23 17:18 ` Richard Biener 1 sibling, 1 reply; 28+ messages in thread From: Jan Hubicka @ 2015-11-23 16:52 UTC (permalink / raw) To: Richard Biener; +Cc: Martin Jambor, Jan Hubicka, Bernd Schmidt, gcc-patches > > I think it also causes the following and one related ICE > > FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal compiler > error) > > /space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > internal compiler error: in get_alias_set, at alias.c:880^M > 0x7528a7 get_alias_set(tree_node*)^M > /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M Does this one reproduce with mainline? All thee ICEs are on the sanity check: gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); which check that in LTO all types that ought to have CANONICAL_TYPE gets CANONICAL_TYPE computed. ICE here probalby means that the type somehow bypassed LTO canonical type merging as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c Honza ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 16:52 ` Jan Hubicka @ 2015-11-23 17:18 ` Richard Biener 2015-11-24 6:23 ` Jan Hubicka 0 siblings, 1 reply; 28+ messages in thread From: Richard Biener @ 2015-11-23 17:18 UTC (permalink / raw) To: Jan Hubicka, Richard Biener; +Cc: Martin Jambor, Bernd Schmidt, gcc-patches On November 23, 2015 5:50:25 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> I think it also causes the following and one related ICE >> >> FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal >compiler >> error) >> >> >/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > >> internal compiler error: in get_alias_set, at alias.c:880^M >> 0x7528a7 get_alias_set(tree_node*)^M >> /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > >Does this one reproduce with mainline? Yes, testsuite run on x86_64 All thee ICEs are on the sanity >check: >gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); >which check that in LTO all types that ought to have CANONICAL_TYPE >gets CANONICAL_TYPE >computed. ICE here probalby means that the type somehow bypassed LTO >canonical type merging >as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c > >Honza ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-23 17:18 ` Richard Biener @ 2015-11-24 6:23 ` Jan Hubicka 2015-11-24 8:38 ` Richard Biener 2015-11-25 10:10 ` James Greenhalgh 0 siblings, 2 replies; 28+ messages in thread From: Jan Hubicka @ 2015-11-24 6:23 UTC (permalink / raw) To: Richard Biener Cc: Jan Hubicka, Richard Biener, Martin Jambor, Bernd Schmidt, gcc-patches > On November 23, 2015 5:50:25 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: > >> > >> I think it also causes the following and one related ICE > >> > >> FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal > >compiler > >> error) > >> > >> > >/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > > > >> internal compiler error: in get_alias_set, at alias.c:880^M > >> 0x7528a7 get_alias_set(tree_node*)^M > >> /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > > > >Does this one reproduce with mainline? > > Yes, testsuite run on x86_64 > > All thee ICEs are on the sanity > >check: > >gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > >which check that in LTO all types that ought to have CANONICAL_TYPE > >gets CANONICAL_TYPE > >computed. ICE here probalby means that the type somehow bypassed LTO > >canonical type merging > >as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c Hi, all the ICE seems to be caused by fact that the simd streaming code creates array of pointers at ltrans time. Because pointers are now TYPE_STRUCTURAL_EQUALITY_P, we get array with TYPE_STRUCTURAL_EQUALITY_P and that sanity check is there to verify that this doesn't happen (because we lose optimization then) This patch makes arrays and vectors to be handled same way as pointers: the canonical type is not computed because it is unused by get_alias_set anyway. I implemented it for different reason - my TBAA checking in lto-symtab got false positives on arrays during the LTO bootstrap. THe problem was again the overly generous globbing of TYPE_CANONICAL. get_alias_set first does t = TYPE_CANONICAL (t) and then for arrays recurses set = get_alias_set (TREE_TYPE (t)); Now we can have type int *[10]; with TYPE_CANONICAL float *[10]; and then get_alias_set (int *[10]) will return get_alias_set (float *) which is wrong, because then the array does not conflict with its elements. I did not managed to get any wrong code out of this, but it is an obvious bug. This problem reproduced prior the pointer patch, too, on other types which are globed at TYPE_CANONICAL computation. This also fixeds semi-latent bug with Ada where TYPE_STRUCTURAL_EQUALITY_P type may win the merging and turn all interoperable arrays into TYPE_STRUCTURAL_EQUALITY_P. For next stage1 I think I can move frontends away from computing TYPE_CANONICAL for those types. At least my initial test for C and C++ seemed to work just fine. Bootstrapped/regtested x86_64-linux, OK? * lto-streamer-in.c (lto_read_body_or_constructor): Set TYPE_CANONICAL only for types where LTO sets them. * tree.c (build_array_type_1): Do ont set TYPE_CANONICAL for LTO. (make_vector_type): Likewise. (gimple_canonical_types_compatible_p): Use canonical_type_used_p. * tree.h (canonical_type_used_p): New inline. * alias.c (get_alias_set): Handle structural equality for all types that pass canonical_type_used_p. (record_component_aliases): Look through all types with record_component_aliases for possible pointers; sanity check that the alias sets match. * lto.c (iterative_hash_canonical_type): Recruse for all types which pass !canonical_type_used_p. (gimple_register_canonical_type_1): Sanity check we do not compute canonical type of anything with !canonical_type_used_p. (gimple_register_canonical_type): Skip all types that are !canonical_type_used_p Index: lto-streamer-in.c =================================================================== --- lto-streamer-in.c (revision 230718) +++ lto-streamer-in.c (working copy) @@ -1231,7 +1231,9 @@ lto_read_body_or_constructor (struct lto if (TYPE_P (t)) { gcc_assert (TYPE_CANONICAL (t) == NULL_TREE); - TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); + if (type_with_alias_set_p (t) + && canonical_type_used_p (t)) + TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); if (TYPE_MAIN_VARIANT (t) != t) { gcc_assert (TYPE_NEXT_VARIANT (t) == NULL_TREE); Index: tree.c =================================================================== --- tree.c (revision 230718) +++ tree.c (working copy) @@ -8236,7 +8243,8 @@ build_array_type_1 (tree elt_type, tree if (TYPE_CANONICAL (t) == t) { if (TYPE_STRUCTURAL_EQUALITY_P (elt_type) - || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))) + || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)) + || in_lto_p) SET_TYPE_STRUCTURAL_EQUALITY (t); else if (TYPE_CANONICAL (elt_type) != elt_type || (index_type && TYPE_CANONICAL (index_type) != index_type)) @@ -9849,13 +9857,13 @@ make_vector_type (tree innertype, int nu SET_TYPE_VECTOR_SUBPARTS (t, nunits); SET_TYPE_MODE (t, mode); - if (TYPE_STRUCTURAL_EQUALITY_P (innertype)) + if (TYPE_STRUCTURAL_EQUALITY_P (innertype) || in_lto_p) SET_TYPE_STRUCTURAL_EQUALITY (t); else if ((TYPE_CANONICAL (innertype) != innertype || mode != VOIDmode) && !VECTOR_BOOLEAN_TYPE_P (t)) TYPE_CANONICAL (t) - = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); + = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); layout_type (t); @@ -13279,7 +13292,8 @@ gimple_canonical_types_compatible_p (con TYPE_CANONICAL is more fine grained than the equivalnce we test (where all pointers are considered equal. Be sure to not return false negatives. */ - gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)); + gcc_checking_assert (canonical_type_used_p (t1) + && canonical_type_used_p (t2)); return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); } Index: tree.h =================================================================== --- tree.h (revision 230718) +++ tree.h (working copy) @@ -4807,7 +4807,9 @@ extern void DEBUG_FUNCTION verify_type ( extern bool gimple_canonical_types_compatible_p (const_tree, const_tree, bool trust_type_canonical = true); extern bool type_with_interoperable_signedness (const_tree); -/* Return simplified tree code of type that is used for canonical type merging. */ + +/* Return simplified tree code of type that is used for canonical type + merging. */ inline enum tree_code tree_code_for_canonical_type_merging (enum tree_code code) { @@ -4829,6 +4831,23 @@ tree_code_for_canonical_type_merging (en return code; } +/* Return ture if get_alias_set care about TYPE_CANONICAL of given type. + We don't define the types for pointers, arrays and vectors. The reason is + that pointers are handled specially: ptr_type_node accesses conflict with + accesses to all other pointers. This is done by alias.c. + Because alias sets of arrays and vectors are the same as types of their + elements, we can't compute canonical type either. Otherwise we could go + form void *[10] to int *[10] (because they are equivalent for canonical type + machinery) and get wrong TBAA. */ + +inline bool +canonical_type_used_p (const_tree t) +{ + return !(POINTER_TYPE_P (t) + || TREE_CODE (t) == ARRAY_TYPE + || TREE_CODE (t) == VECTOR_TYPE); +} + #define tree_map_eq tree_map_base_eq extern unsigned int tree_map_hash (const void *); #define tree_map_marked_p tree_map_base_marked_p Index: alias.c =================================================================== --- alias.c (revision 230718) +++ alias.c (working copy) @@ -869,11 +870,11 @@ get_alias_set (tree t) set = lang_hooks.get_alias_set (t); if (set != -1) return set; - /* Handle structure type equality for pointer types. This is easy - to do, because the code bellow ignore canonical types on these anyway. - This is important for LTO, where TYPE_CANONICAL for pointers can not - be meaningfuly computed by the frotnend. */ - if (!POINTER_TYPE_P (t)) + /* Handle structure type equality for pointer types, arrays and vectors. + This is easy to do, because the code bellow ignore canonical types on + these anyway. This is important for LTO, where TYPE_CANONICAL for + pointers can not be meaningfuly computed by the frotnend. */ + if (canonical_type_used_p (t)) { /* In LTO we set canonical types for all types where it makes sense to do so. Double check we did not miss some type. */ @@ -929,7 +931,9 @@ get_alias_set (tree t) integer(kind=4)[4] the same alias set or not. Just be pragmatic here and make sure the array and its element type get the same alias set assigned. */ - else if (TREE_CODE (t) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (t)) + else if (TREE_CODE (t) == ARRAY_TYPE + && (!TYPE_NONALIASED_COMPONENT (t) + || TYPE_STRUCTURAL_EQUALITY_P (t))) set = get_alias_set (TREE_TYPE (t)); /* From the former common C and C++ langhook implementation: @@ -971,7 +975,10 @@ get_alias_set (tree t) We also want to make pointer to array/vector equivalent to pointer to its element (see the reasoning above). Skip all those types, too. */ for (p = t; POINTER_TYPE_P (p) - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) + || (TREE_CODE (p) == ARRAY_TYPE + && (!TYPE_NONALIASED_COMPONENT (p) + || !COMPLETE_TYPE_P (p) + || TYPE_STRUCTURAL_EQUALITY_P (p))) || TREE_CODE (p) == VECTOR_TYPE; p = TREE_TYPE (p)) { @@ -1195,20 +1203,23 @@ record_component_aliases (tree type) Accesses to it conflicts with accesses to any other pointer type. */ tree t = TREE_TYPE (field); - if (in_lto_p) + 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 ((TREE_CODE (t) == ARRAY_TYPE - && (!COMPLETE_TYPE_P (t) - || TYPE_NONALIASED_COMPONENT (t))) - || TREE_CODE (t) == VECTOR_TYPE) - t = TREE_TYPE (t); + 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; Index: lto/lto.c =================================================================== --- lto/lto.c (revision 230718) +++ lto/lto.c (working copy) @@ -389,9 +389,7 @@ iterative_hash_canonical_type (tree type /* All type variants have same TYPE_CANONICAL. */ type = TYPE_MAIN_VARIANT (type); - /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing - code never use it anyway. */ - if (POINTER_TYPE_P (type)) + if (!canonical_type_used_p (type)) v = hash_canonical_type (type); /* An already processed type. */ else if (TYPE_CANONICAL (type)) @@ -444,7 +442,7 @@ gimple_register_canonical_type_1 (tree t gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) && type_with_alias_set_p (t) - && !POINTER_TYPE_P (t)); + && canonical_type_used_p (t)); slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); if (*slot) @@ -477,7 +475,8 @@ gimple_register_canonical_type_1 (tree t static void gimple_register_canonical_type (tree t) { - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) + || !canonical_type_used_p (t)) return; /* Canonical types are same among all complete variants. */ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-24 6:23 ` Jan Hubicka @ 2015-11-24 8:38 ` Richard Biener 2015-11-25 10:10 ` James Greenhalgh 1 sibling, 0 replies; 28+ messages in thread From: Richard Biener @ 2015-11-24 8:38 UTC (permalink / raw) To: Jan Hubicka; +Cc: Richard Biener, Martin Jambor, Bernd Schmidt, gcc-patches On Tue, 24 Nov 2015, Jan Hubicka wrote: > > On November 23, 2015 5:50:25 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: > > >> > > >> I think it also causes the following and one related ICE > > >> > > >> FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal > > >compiler > > >> error) > > >> > > >> > > >/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > > > > > >> internal compiler error: in get_alias_set, at alias.c:880^M > > >> 0x7528a7 get_alias_set(tree_node*)^M > > >> /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > > > > > >Does this one reproduce with mainline? > > > > Yes, testsuite run on x86_64 > > > > All thee ICEs are on the sanity > > >check: > > >gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > > >which check that in LTO all types that ought to have CANONICAL_TYPE > > >gets CANONICAL_TYPE > > >computed. ICE here probalby means that the type somehow bypassed LTO > > >canonical type merging > > >as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c > > Hi, > all the ICE seems to be caused by fact that the simd streaming code creates > array of pointers at ltrans time. Because pointers are now > TYPE_STRUCTURAL_EQUALITY_P, we get array with TYPE_STRUCTURAL_EQUALITY_P > and that sanity check is there to verify that this doesn't happen (because > we lose optimization then) > > This patch makes arrays and vectors to be handled same way as pointers: > the canonical type is not computed because it is unused by get_alias_set > anyway. > > I implemented it for different reason - my TBAA checking in lto-symtab got > false positives on arrays during the LTO bootstrap. THe problem was again > the overly generous globbing of TYPE_CANONICAL. > > get_alias_set first does > > t = TYPE_CANONICAL (t) > > and then for arrays recurses > > set = get_alias_set (TREE_TYPE (t)); > > Now we can have type > > int *[10]; > > with TYPE_CANONICAL > > float *[10]; > > and then get_alias_set (int *[10]) will return get_alias_set (float *) > which is wrong, because then the array does not conflict with its elements. > I did not managed to get any wrong code out of this, but it is an obvious bug. > This problem reproduced prior the pointer patch, too, on other types which are > globed at TYPE_CANONICAL computation. > > This also fixeds semi-latent bug with Ada where TYPE_STRUCTURAL_EQUALITY_P type > may win the merging and turn all interoperable arrays into > TYPE_STRUCTURAL_EQUALITY_P. > > For next stage1 I think I can move frontends away from computing TYPE_CANONICAL > for those types. At least my initial test for C and C++ seemed to work just fine. > > Bootstrapped/regtested x86_64-linux, OK? I don't like the in_lto_p checks in tree.c but as you say they'll be removed next stage1 ok. Thanks, Richard. > * lto-streamer-in.c (lto_read_body_or_constructor): Set TYPE_CANONICAL > only for types where LTO sets them. > * tree.c (build_array_type_1): Do ont set TYPE_CANONICAL for LTO. > (make_vector_type): Likewise. > (gimple_canonical_types_compatible_p): Use canonical_type_used_p. > * tree.h (canonical_type_used_p): New inline. > * alias.c (get_alias_set): Handle structural equality for all > types that pass canonical_type_used_p. > (record_component_aliases): Look through all types with > record_component_aliases for possible pointers; sanity check that > the alias sets match. > > * lto.c (iterative_hash_canonical_type): Recruse for all types > which pass !canonical_type_used_p. > (gimple_register_canonical_type_1): Sanity check we do not compute > canonical type of anything with !canonical_type_used_p. > (gimple_register_canonical_type): Skip all types that are > !canonical_type_used_p > > Index: lto-streamer-in.c > =================================================================== > --- lto-streamer-in.c (revision 230718) > +++ lto-streamer-in.c (working copy) > @@ -1231,7 +1231,9 @@ lto_read_body_or_constructor (struct lto > if (TYPE_P (t)) > { > gcc_assert (TYPE_CANONICAL (t) == NULL_TREE); > - TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); > + if (type_with_alias_set_p (t) > + && canonical_type_used_p (t)) > + TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); > if (TYPE_MAIN_VARIANT (t) != t) > { > gcc_assert (TYPE_NEXT_VARIANT (t) == NULL_TREE); > Index: tree.c > =================================================================== > --- tree.c (revision 230718) > +++ tree.c (working copy) > @@ -8236,7 +8243,8 @@ build_array_type_1 (tree elt_type, tree > if (TYPE_CANONICAL (t) == t) > { > if (TYPE_STRUCTURAL_EQUALITY_P (elt_type) > - || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))) > + || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)) > + || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if (TYPE_CANONICAL (elt_type) != elt_type > || (index_type && TYPE_CANONICAL (index_type) != index_type)) > @@ -9849,13 +9857,13 @@ make_vector_type (tree innertype, int nu > SET_TYPE_VECTOR_SUBPARTS (t, nunits); > SET_TYPE_MODE (t, mode); > > - if (TYPE_STRUCTURAL_EQUALITY_P (innertype)) > + if (TYPE_STRUCTURAL_EQUALITY_P (innertype) || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if ((TYPE_CANONICAL (innertype) != innertype > || mode != VOIDmode) > && !VECTOR_BOOLEAN_TYPE_P (t)) > TYPE_CANONICAL (t) > - = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); > + = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); > > layout_type (t); > > @@ -13279,7 +13292,8 @@ gimple_canonical_types_compatible_p (con > TYPE_CANONICAL is more fine grained than the equivalnce we test (where > all pointers are considered equal. Be sure to not return false > negatives. */ > - gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)); > + gcc_checking_assert (canonical_type_used_p (t1) > + && canonical_type_used_p (t2)); > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > } > > Index: tree.h > =================================================================== > --- tree.h (revision 230718) > +++ tree.h (working copy) > @@ -4807,7 +4807,9 @@ extern void DEBUG_FUNCTION verify_type ( > extern bool gimple_canonical_types_compatible_p (const_tree, const_tree, > bool trust_type_canonical = true); > extern bool type_with_interoperable_signedness (const_tree); > -/* Return simplified tree code of type that is used for canonical type merging. */ > + > +/* Return simplified tree code of type that is used for canonical type > + merging. */ > inline enum tree_code > tree_code_for_canonical_type_merging (enum tree_code code) > { > @@ -4829,6 +4831,23 @@ tree_code_for_canonical_type_merging (en > return code; > } > > +/* Return ture if get_alias_set care about TYPE_CANONICAL of given type. > + We don't define the types for pointers, arrays and vectors. The reason is > + that pointers are handled specially: ptr_type_node accesses conflict with > + accesses to all other pointers. This is done by alias.c. > + Because alias sets of arrays and vectors are the same as types of their > + elements, we can't compute canonical type either. Otherwise we could go > + form void *[10] to int *[10] (because they are equivalent for canonical type > + machinery) and get wrong TBAA. */ > + > +inline bool > +canonical_type_used_p (const_tree t) > +{ > + return !(POINTER_TYPE_P (t) > + || TREE_CODE (t) == ARRAY_TYPE > + || TREE_CODE (t) == VECTOR_TYPE); > +} > + > #define tree_map_eq tree_map_base_eq > extern unsigned int tree_map_hash (const void *); > #define tree_map_marked_p tree_map_base_marked_p > Index: alias.c > =================================================================== > --- alias.c (revision 230718) > +++ alias.c (working copy) > @@ -869,11 +870,11 @@ get_alias_set (tree t) > set = lang_hooks.get_alias_set (t); > if (set != -1) > return set; > - /* Handle structure type equality for pointer types. This is easy > - to do, because the code bellow ignore canonical types on these anyway. > - This is important for LTO, where TYPE_CANONICAL for pointers can not > - be meaningfuly computed by the frotnend. */ > - if (!POINTER_TYPE_P (t)) > + /* Handle structure type equality for pointer types, arrays and vectors. > + This is easy to do, because the code bellow ignore canonical types on > + these anyway. This is important for LTO, where TYPE_CANONICAL for > + pointers can not be meaningfuly computed by the frotnend. */ > + if (canonical_type_used_p (t)) > { > /* In LTO we set canonical types for all types where it makes > sense to do so. Double check we did not miss some type. */ > @@ -929,7 +931,9 @@ get_alias_set (tree t) > integer(kind=4)[4] the same alias set or not. > Just be pragmatic here and make sure the array and its element > type get the same alias set assigned. */ > - else if (TREE_CODE (t) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (t)) > + else if (TREE_CODE (t) == ARRAY_TYPE > + && (!TYPE_NONALIASED_COMPONENT (t) > + || TYPE_STRUCTURAL_EQUALITY_P (t))) > set = get_alias_set (TREE_TYPE (t)); > > /* From the former common C and C++ langhook implementation: > @@ -971,7 +975,10 @@ get_alias_set (tree t) > We also want to make pointer to array/vector equivalent to pointer to > its element (see the reasoning above). Skip all those types, too. */ > for (p = t; POINTER_TYPE_P (p) > - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) > + || (TREE_CODE (p) == ARRAY_TYPE > + && (!TYPE_NONALIASED_COMPONENT (p) > + || !COMPLETE_TYPE_P (p) > + || TYPE_STRUCTURAL_EQUALITY_P (p))) > || TREE_CODE (p) == VECTOR_TYPE; > p = TREE_TYPE (p)) > { > @@ -1195,20 +1203,23 @@ record_component_aliases (tree type) > Accesses to it conflicts with accesses to any other pointer > type. */ > tree t = TREE_TYPE (field); > - if (in_lto_p) > + 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 ((TREE_CODE (t) == ARRAY_TYPE > - && (!COMPLETE_TYPE_P (t) > - || TYPE_NONALIASED_COMPONENT (t))) > - || TREE_CODE (t) == VECTOR_TYPE) > - t = TREE_TYPE (t); > + 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; > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 230718) > +++ lto/lto.c (working copy) > @@ -389,9 +389,7 @@ iterative_hash_canonical_type (tree type > /* All type variants have same TYPE_CANONICAL. */ > type = TYPE_MAIN_VARIANT (type); > > - /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing > - code never use it anyway. */ > - if (POINTER_TYPE_P (type)) > + if (!canonical_type_used_p (type)) > v = hash_canonical_type (type); > /* An already processed type. */ > else if (TYPE_CANONICAL (type)) > @@ -444,7 +442,7 @@ gimple_register_canonical_type_1 (tree t > > gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) > && type_with_alias_set_p (t) > - && !POINTER_TYPE_P (t)); > + && canonical_type_used_p (t)); > > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > @@ -477,7 +475,8 @@ gimple_register_canonical_type_1 (tree t > static void > gimple_register_canonical_type (tree t) > { > - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) > + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) > + || !canonical_type_used_p (t)) > return; > > /* Canonical types are same among all complete variants. */ > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Enable pointer TBAA for LTO 2015-11-24 6:23 ` Jan Hubicka 2015-11-24 8:38 ` Richard Biener @ 2015-11-25 10:10 ` James Greenhalgh 1 sibling, 0 replies; 28+ messages in thread From: James Greenhalgh @ 2015-11-25 10:10 UTC (permalink / raw) To: Jan Hubicka Cc: Richard Biener, Richard Biener, Martin Jambor, Bernd Schmidt, gcc-patches On Tue, Nov 24, 2015 at 07:10:01AM +0100, Jan Hubicka wrote: > > On November 23, 2015 5:50:25 PM GMT+01:00, Jan Hubicka <hubicka@ucw.cz> wrote: > > >> > > >> I think it also causes the following and one related ICE > > >> > > >> FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal > > >compiler > > >> error) > > >> > > >> > > >/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1: > > > > > >> internal compiler error: in get_alias_set, at alias.c:880^M > > >> 0x7528a7 get_alias_set(tree_node*)^M > > >> /space/rguenther/src/svn/trunk3/gcc/alias.c:880^M > > > > > >Does this one reproduce with mainline? > > > > Yes, testsuite run on x86_64 > > > > All thee ICEs are on the sanity > > >check: > > >gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t)); > > >which check that in LTO all types that ought to have CANONICAL_TYPE > > >gets CANONICAL_TYPE > > >computed. ICE here probalby means that the type somehow bypassed LTO > > >canonical type merging > > >as well as the TYPE_CANONICAL=MAIN_VARIANT set in lto-streamer.c > > > * lto-streamer-in.c (lto_read_body_or_constructor): Set TYPE_CANONICAL > only for types where LTO sets them. > * tree.c (build_array_type_1): Do ont set TYPE_CANONICAL for LTO. > (make_vector_type): Likewise. > (gimple_canonical_types_compatible_p): Use canonical_type_used_p. > * tree.h (canonical_type_used_p): New inline. > * alias.c (get_alias_set): Handle structural equality for all > types that pass canonical_type_used_p. > (record_component_aliases): Look through all types with > record_component_aliases for possible pointers; sanity check that > the alias sets match. > > * lto.c (iterative_hash_canonical_type): Recruse for all types > which pass !canonical_type_used_p. > (gimple_register_canonical_type_1): Sanity check we do not compute > canonical type of anything with !canonical_type_used_p. > (gimple_register_canonical_type): Skip all types that are > !canonical_type_used_p Hi, This patch introduces an ICE for an AArch64 testcase: FAIL: gcc.target/aarch64/advsimd-intrinsics/vstX_lane.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) Excess errors: ...../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vstX_lane.c:260:6: internal compiler error: in record_component_aliases, at alias.c:1219 0x5ae2a5 record_component_aliases(tree_node*) .../gcc/alias.c:1218 0x5aed46 get_alias_set(tree_node*) .../gcc/alias.c:1068 0x5afe2a get_deref_alias_set(tree_node*) .../gcc/alias.c:697 0x5ae408 get_alias_set(tree_node*) .../gcc/alias.c:845 0xb6bfd7 vn_reference_lookup(tree_node*, tree_node*, vn_lookup_kind, vn_reference_s**) .../gcc/tree-ssa-sccvn.c:2250 0xb7026f visit_reference_op_store .../gcc/tree-ssa-sccvn.c:3310 0xb7026f visit_use .../gcc/tree-ssa-sccvn.c:3558 0xb716b4 process_scc .../gcc/tree-ssa-sccvn.c:3800 0xb716b4 extract_and_process_scc_for_name .../gcc/tree-ssa-sccvn.c:3887 0xb716b4 DFS .../gcc/tree-ssa-sccvn.c:3939 0xb729db sccvn_dom_walker::before_dom_children(basic_block_def*) .../gcc/tree-ssa-sccvn.c:4427 0xec017f dom_walker::walk(basic_block_def*) .../gcc/domwalk.c:176 0xb73aaa run_scc_vn(vn_lookup_kind) .../gcc/tree-ssa-sccvn.c:4550 0xb48278 execute .../gcc/tree-ssa-pre.c:4891 Thanks, James > > Index: lto-streamer-in.c > =================================================================== > --- lto-streamer-in.c (revision 230718) > +++ lto-streamer-in.c (working copy) > @@ -1231,7 +1231,9 @@ lto_read_body_or_constructor (struct lto > if (TYPE_P (t)) > { > gcc_assert (TYPE_CANONICAL (t) == NULL_TREE); > - TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); > + if (type_with_alias_set_p (t) > + && canonical_type_used_p (t)) > + TYPE_CANONICAL (t) = TYPE_MAIN_VARIANT (t); > if (TYPE_MAIN_VARIANT (t) != t) > { > gcc_assert (TYPE_NEXT_VARIANT (t) == NULL_TREE); > Index: tree.c > =================================================================== > --- tree.c (revision 230718) > +++ tree.c (working copy) > @@ -8236,7 +8243,8 @@ build_array_type_1 (tree elt_type, tree > if (TYPE_CANONICAL (t) == t) > { > if (TYPE_STRUCTURAL_EQUALITY_P (elt_type) > - || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type))) > + || (index_type && TYPE_STRUCTURAL_EQUALITY_P (index_type)) > + || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if (TYPE_CANONICAL (elt_type) != elt_type > || (index_type && TYPE_CANONICAL (index_type) != index_type)) > @@ -9849,13 +9857,13 @@ make_vector_type (tree innertype, int nu > SET_TYPE_VECTOR_SUBPARTS (t, nunits); > SET_TYPE_MODE (t, mode); > > - if (TYPE_STRUCTURAL_EQUALITY_P (innertype)) > + if (TYPE_STRUCTURAL_EQUALITY_P (innertype) || in_lto_p) > SET_TYPE_STRUCTURAL_EQUALITY (t); > else if ((TYPE_CANONICAL (innertype) != innertype > || mode != VOIDmode) > && !VECTOR_BOOLEAN_TYPE_P (t)) > TYPE_CANONICAL (t) > - = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); > + = make_vector_type (TYPE_CANONICAL (innertype), nunits, VOIDmode); > > layout_type (t); > > @@ -13279,7 +13292,8 @@ gimple_canonical_types_compatible_p (con > TYPE_CANONICAL is more fine grained than the equivalnce we test (where > all pointers are considered equal. Be sure to not return false > negatives. */ > - gcc_checking_assert (!POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2)); > + gcc_checking_assert (canonical_type_used_p (t1) > + && canonical_type_used_p (t2)); > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > } > > Index: tree.h > =================================================================== > --- tree.h (revision 230718) > +++ tree.h (working copy) > @@ -4807,7 +4807,9 @@ extern void DEBUG_FUNCTION verify_type ( > extern bool gimple_canonical_types_compatible_p (const_tree, const_tree, > bool trust_type_canonical = true); > extern bool type_with_interoperable_signedness (const_tree); > -/* Return simplified tree code of type that is used for canonical type merging. */ > + > +/* Return simplified tree code of type that is used for canonical type > + merging. */ > inline enum tree_code > tree_code_for_canonical_type_merging (enum tree_code code) > { > @@ -4829,6 +4831,23 @@ tree_code_for_canonical_type_merging (en > return code; > } > > +/* Return ture if get_alias_set care about TYPE_CANONICAL of given type. > + We don't define the types for pointers, arrays and vectors. The reason is > + that pointers are handled specially: ptr_type_node accesses conflict with > + accesses to all other pointers. This is done by alias.c. > + Because alias sets of arrays and vectors are the same as types of their > + elements, we can't compute canonical type either. Otherwise we could go > + form void *[10] to int *[10] (because they are equivalent for canonical type > + machinery) and get wrong TBAA. */ > + > +inline bool > +canonical_type_used_p (const_tree t) > +{ > + return !(POINTER_TYPE_P (t) > + || TREE_CODE (t) == ARRAY_TYPE > + || TREE_CODE (t) == VECTOR_TYPE); > +} > + > #define tree_map_eq tree_map_base_eq > extern unsigned int tree_map_hash (const void *); > #define tree_map_marked_p tree_map_base_marked_p > Index: alias.c > =================================================================== > --- alias.c (revision 230718) > +++ alias.c (working copy) > @@ -869,11 +870,11 @@ get_alias_set (tree t) > set = lang_hooks.get_alias_set (t); > if (set != -1) > return set; > - /* Handle structure type equality for pointer types. This is easy > - to do, because the code bellow ignore canonical types on these anyway. > - This is important for LTO, where TYPE_CANONICAL for pointers can not > - be meaningfuly computed by the frotnend. */ > - if (!POINTER_TYPE_P (t)) > + /* Handle structure type equality for pointer types, arrays and vectors. > + This is easy to do, because the code bellow ignore canonical types on > + these anyway. This is important for LTO, where TYPE_CANONICAL for > + pointers can not be meaningfuly computed by the frotnend. */ > + if (canonical_type_used_p (t)) > { > /* In LTO we set canonical types for all types where it makes > sense to do so. Double check we did not miss some type. */ > @@ -929,7 +931,9 @@ get_alias_set (tree t) > integer(kind=4)[4] the same alias set or not. > Just be pragmatic here and make sure the array and its element > type get the same alias set assigned. */ > - else if (TREE_CODE (t) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (t)) > + else if (TREE_CODE (t) == ARRAY_TYPE > + && (!TYPE_NONALIASED_COMPONENT (t) > + || TYPE_STRUCTURAL_EQUALITY_P (t))) > set = get_alias_set (TREE_TYPE (t)); > > /* From the former common C and C++ langhook implementation: > @@ -971,7 +975,10 @@ get_alias_set (tree t) > We also want to make pointer to array/vector equivalent to pointer to > its element (see the reasoning above). Skip all those types, too. */ > for (p = t; POINTER_TYPE_P (p) > - || (TREE_CODE (p) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (p)) > + || (TREE_CODE (p) == ARRAY_TYPE > + && (!TYPE_NONALIASED_COMPONENT (p) > + || !COMPLETE_TYPE_P (p) > + || TYPE_STRUCTURAL_EQUALITY_P (p))) > || TREE_CODE (p) == VECTOR_TYPE; > p = TREE_TYPE (p)) > { > @@ -1195,20 +1203,23 @@ record_component_aliases (tree type) > Accesses to it conflicts with accesses to any other pointer > type. */ > tree t = TREE_TYPE (field); > - if (in_lto_p) > + 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 ((TREE_CODE (t) == ARRAY_TYPE > - && (!COMPLETE_TYPE_P (t) > - || TYPE_NONALIASED_COMPONENT (t))) > - || TREE_CODE (t) == VECTOR_TYPE) > - t = TREE_TYPE (t); > + 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; > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 230718) > +++ lto/lto.c (working copy) > @@ -389,9 +389,7 @@ iterative_hash_canonical_type (tree type > /* All type variants have same TYPE_CANONICAL. */ > type = TYPE_MAIN_VARIANT (type); > > - /* We do not compute TYPE_CANONICAl of POINTER_TYPE because the aliasing > - code never use it anyway. */ > - if (POINTER_TYPE_P (type)) > + if (!canonical_type_used_p (type)) > v = hash_canonical_type (type); > /* An already processed type. */ > else if (TYPE_CANONICAL (type)) > @@ -444,7 +442,7 @@ gimple_register_canonical_type_1 (tree t > > gcc_checking_assert (TYPE_P (t) && !TYPE_CANONICAL (t) > && type_with_alias_set_p (t) > - && !POINTER_TYPE_P (t)); > + && canonical_type_used_p (t)); > > slot = htab_find_slot_with_hash (gimple_canonical_types, t, hash, INSERT); > if (*slot) > @@ -477,7 +475,8 @@ gimple_register_canonical_type_1 (tree t > static void > gimple_register_canonical_type (tree t) > { > - if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) || POINTER_TYPE_P (t)) > + if (TYPE_CANONICAL (t) || !type_with_alias_set_p (t) > + || !canonical_type_used_p (t)) > return; > > /* Canonical types are same among all complete variants. */ > ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-11-27 7:31 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-08 20:46 Enable pointer TBAA for LTO Jan Hubicka 2015-11-10 12:27 ` Richard Biener 2015-11-10 18:15 ` Jan Hubicka 2015-11-11 9:21 ` Richard Biener 2015-11-11 12:43 ` Bernd Schmidt 2015-11-11 22:14 ` Jan Hubicka 2015-11-11 22:31 ` Jan Hubicka 2015-11-22 23:19 ` Jan Hubicka 2015-11-22 23:22 ` Eric Botcazou 2015-11-23 1:20 ` Jan Hubicka 2015-11-23 8:34 ` Eric Botcazou 2015-11-23 17:26 ` Jan Hubicka 2015-11-23 10:39 ` Richard Biener 2015-11-23 17:08 ` Jan Hubicka 2015-11-24 8:34 ` Richard Biener 2015-11-24 19:05 ` Jan Hubicka 2015-11-25 10:49 ` Richard Biener 2015-11-25 18:35 ` Jan Hubicka 2015-11-24 5:01 ` Jan Hubicka 2015-11-23 13:52 ` Martin Jambor 2015-11-23 15:33 ` Richard Biener 2015-11-23 15:35 ` Ilya Verbin 2015-11-27 8:24 ` Thomas Schwinge 2015-11-23 16:52 ` Jan Hubicka 2015-11-23 17:18 ` Richard Biener 2015-11-24 6:23 ` Jan Hubicka 2015-11-24 8:38 ` Richard Biener 2015-11-25 10:10 ` James Greenhalgh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).