| Message ID | 20201117125409.GA72722@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Make ltrans type canonicals compatible with WPA ones | expand |
On Tue, 17 Nov 2020, Jan Hubicka wrote: > Hi, > this patch fixes profiledbootstrap failure with LTO enabled. > What happens is that alias_ptr_types_compatible_p relies on the > fact that alias sets are not refined from WPA to ltrans time: > > /* This function originally abstracts from simply comparing > get_deref_alias_set so that we are sure this still computes > the same result after LTO type merging is applied. > When in LTO type merging is done we can actually do this compare. > */ > if (in_lto_p) > return get_deref_alias_set (t1) == get_deref_alias_set (t2); > else > return (TYPE_MAIN_VARIANT (TREE_TYPE (t1)) > == TYPE_MAIN_VARIANT (TREE_TYPE (t2))); > > This conditional is confused - it pesimizes code with -fno-lto > for no good reason. I will fix that separately: we now have > lto_streaming_expected_p so I think it should read > > if (!lto_stremaing_expected_p () || flag_wpa) > use alias sets > else > use main varaiants as conservative estimate. > > (so if we ever get idea to ICF during incremental link or deduplicate in > early passes, things will work safely). I will send separate patch on > this. > > > Not refining alias sets from WPA to ltrans time is a good invariant to > maintain and the canonical type hash behaves this way. However I broke > this with the ODR logic. > > Normally we define canonical types for C++ ODR types according to their > type names. However to make ODR types compatible with C types we check > if structurally equivalent C type exists and if so, we ignore ODR > names giving up on the precision. > > This however is not stable between WPA and ltrans since at ltrans the > type merging does not see as many types as WPA does. To make this > consistent the patch makes WPA ODR_TYPE_P == 0 for ODR types that > conflicted with non-ODR type. > > I had to drop one sanity check in ipa-utils.h (that I think is not very > important - I added it while introducing CXX_ODR_P machinery) and also > it now may happen that we query odr_based_tbaa_p before registering > first ODR type so we do not want to ICE here. > ODR type registration happens early to produce ODR violation warings. > Those are not done at ltrans, so dropping the registration is safe. The > type will still be added while computing the type inheritance graph if > needed for devirtualization (and late devirtualization is not very > useful anyway since it won't enable inlining). > > Bootstrapped, regtested x86_64-linux, OK? OK. > I think this should go to release branches after some soaking in > mainline too, even if we don't have direct reproducers. > > Note that Martin implemented type checker to sanity check that alias > sets are never getting refined from compile to WPa and from WPA to > ltrans. I recovered the patch and will play with it more. > I think we should eventually establish this (if alias sets are refined > from copmile time to WPA it is either wrong code issue or frontend alias > sets are not as good as they should be), but of course there are fun > issues. My plan is to see if I can identify some wrong code bugs and > leave rest for early next stage1. So do we want to actually compute alias sets and stream them, "freeing up" TYPE_CANONICAL again? We're sort-of taking it away from FEs which use it before and recompute it possibly in different ways ... Richard. > PR bootstrap/97857 > * ipa-devirt.c (odr_based_tbaa_p): Do not ICE when > odr_hash is not initialized. > * ipa-utils.h (type_with_linkage_p): Do not sanity check > CXX_ODR_P. > * lto-common.c (gimple_register_canonical_type_1): Only > register types with TYPE_CXX_ODR_P flag; sanity check that no > conflict happens at ltrans time. > * tree-streamer-out.c (pack_ts_type_common_value_fields): Set > CXX_ODR_P according to the canonical type. > diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c > index 067ed5ba073..6e6df0b2af5 100644 > --- a/gcc/ipa-devirt.c > +++ b/gcc/ipa-devirt.c > @@ -2032,6 +2032,8 @@ odr_based_tbaa_p (const_tree type) > { > if (!RECORD_OR_UNION_TYPE_P (type)) > return false; > + if (!odr_hash) > + return false; > odr_type t = get_odr_type (const_cast <tree> (type), false); > if (!t || !t->tbaa_enabled) > return false; > diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h > index 880e527c590..91571d8e82a 100644 > --- a/gcc/ipa-utils.h > +++ b/gcc/ipa-utils.h > @@ -211,8 +211,6 @@ type_with_linkage_p (const_tree t) > if (!TYPE_CONTEXT (t)) > return false; > > - gcc_checking_assert (TREE_CODE (t) == ENUMERAL_TYPE || TYPE_CXX_ODR_P (t)); > - > return true; > } > > diff --git a/gcc/lto/lto-common.c b/gcc/lto/lto-common.c > index 6944c469f89..0a3033c3695 100644 > --- a/gcc/lto/lto-common.c > +++ b/gcc/lto/lto-common.c > @@ -415,8 +415,8 @@ gimple_register_canonical_type_1 (tree t, hashval_t hash) > that we can use to lookup structurally equivalent non-ODR type. > In case we decide to treat type as unique ODR type we recompute hash based > on name and let TBAA machinery know about our decision. */ > - if (RECORD_OR_UNION_TYPE_P (t) > - && odr_type_p (t) && !odr_type_violation_reported_p (t)) > + if (RECORD_OR_UNION_TYPE_P (t) && odr_type_p (t) > + && TYPE_CXX_ODR_P (t) && !odr_type_violation_reported_p (t)) > { > /* Anonymous namespace types never conflict with non-C++ types. */ > if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) > @@ -434,6 +434,7 @@ gimple_register_canonical_type_1 (tree t, hashval_t hash) > if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) > { > tree nonodr = *(tree *)slot; > + gcc_checking_assert (!flag_ltrans); > if (symtab->dump_file) > { > fprintf (symtab->dump_file, > diff --git a/gcc/tree-streamer-out.c b/gcc/tree-streamer-out.c > index d7a451cfef4..9383cc4b903 100644 > --- a/gcc/tree-streamer-out.c > +++ b/gcc/tree-streamer-out.c > @@ -343,7 +343,11 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr) > { > bp_pack_value (bp, TYPE_TRANSPARENT_AGGR (expr), 1); > bp_pack_value (bp, TYPE_FINAL_P (expr), 1); > - bp_pack_value (bp, TYPE_CXX_ODR_P (expr), 1); > + /* alias_ptr_types_compatible_p relies on fact that during LTO > + types do not get refined from WPA time to ltrans. */ > + bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr) > + ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr)) > + : TYPE_CXX_ODR_P (expr), 1); > } > else if (TREE_CODE (expr) == ARRAY_TYPE) > bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1); >
Hi, thanks! > > So do we want to actually compute alias sets and stream them, > "freeing up" TYPE_CANONICAL again? We're sort-of taking it away I am not sure what you mean by freeing up TYPE_CANONICAL again :) but I was playing with idea of streaming the alias sets from WPA to ltrans. It may simplify things especially if our canonical type logic gets more complex... In particular I would eventually like to avoid pesimizing C/C++ code just becuase we worry about compatibility with Fortran and such and had some patches for this direction but so far there was more pressing issues with TBAA than this. We do not really use TYPE_CANONICAL outside of tree-ssa-alias and alias.c Honza > from FEs which use it before and recompute it possibly in different > ways ... > > Richard.
On Tue, 17 Nov 2020, Jan Hubicka wrote: > Hi, > thanks! > > > > So do we want to actually compute alias sets and stream them, > > "freeing up" TYPE_CANONICAL again? We're sort-of taking it away > > I am not sure what you mean by freeing up TYPE_CANONICAL again :) but I > was playing with idea of streaming the alias sets from WPA to ltrans. It > may simplify things especially if our canonical type logic gets more > complex... In particular I would eventually like to avoid pesimizing > C/C++ code just becuase we worry about compatibility with Fortran and > such and had some patches for this direction but so far there was more > pressing issues with TBAA than this. > > We do not really use TYPE_CANONICAL outside of tree-ssa-alias and > alias.c GIMPLE type checking uses it heavily (aka useless_type_conversion_p). Richard. > Honza > > from FEs which use it before and recompute it possibly in different > > ways ... > > > > Richard. >
diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c index 067ed5ba073..6e6df0b2af5 100644 --- a/gcc/ipa-devirt.c +++ b/gcc/ipa-devirt.c @@ -2032,6 +2032,8 @@ odr_based_tbaa_p (const_tree type) { if (!RECORD_OR_UNION_TYPE_P (type)) return false; + if (!odr_hash) + return false; odr_type t = get_odr_type (const_cast <tree> (type), false); if (!t || !t->tbaa_enabled) return false; diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h index 880e527c590..91571d8e82a 100644 --- a/gcc/ipa-utils.h +++ b/gcc/ipa-utils.h @@ -211,8 +211,6 @@ type_with_linkage_p (const_tree t) if (!TYPE_CONTEXT (t)) return false; - gcc_checking_assert (TREE_CODE (t) == ENUMERAL_TYPE || TYPE_CXX_ODR_P (t)); - return true; } diff --git a/gcc/lto/lto-common.c b/gcc/lto/lto-common.c index 6944c469f89..0a3033c3695 100644 --- a/gcc/lto/lto-common.c +++ b/gcc/lto/lto-common.c @@ -415,8 +415,8 @@ gimple_register_canonical_type_1 (tree t, hashval_t hash) that we can use to lookup structurally equivalent non-ODR type. In case we decide to treat type as unique ODR type we recompute hash based on name and let TBAA machinery know about our decision. */ - if (RECORD_OR_UNION_TYPE_P (t) - && odr_type_p (t) && !odr_type_violation_reported_p (t)) + if (RECORD_OR_UNION_TYPE_P (t) && odr_type_p (t) + && TYPE_CXX_ODR_P (t) && !odr_type_violation_reported_p (t)) { /* Anonymous namespace types never conflict with non-C++ types. */ if (type_with_linkage_p (t) && type_in_anonymous_namespace_p (t)) @@ -434,6 +434,7 @@ gimple_register_canonical_type_1 (tree t, hashval_t hash) if (slot && !TYPE_CXX_ODR_P (*(tree *)slot)) { tree nonodr = *(tree *)slot; + gcc_checking_assert (!flag_ltrans); if (symtab->dump_file) { fprintf (symtab->dump_file, diff --git a/gcc/tree-streamer-out.c b/gcc/tree-streamer-out.c index d7a451cfef4..9383cc4b903 100644 --- a/gcc/tree-streamer-out.c +++ b/gcc/tree-streamer-out.c @@ -343,7 +343,11 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr) { bp_pack_value (bp, TYPE_TRANSPARENT_AGGR (expr), 1); bp_pack_value (bp, TYPE_FINAL_P (expr), 1); - bp_pack_value (bp, TYPE_CXX_ODR_P (expr), 1); + /* alias_ptr_types_compatible_p relies on fact that during LTO + types do not get refined from WPA time to ltrans. */ + bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr) + ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr)) + : TYPE_CXX_ODR_P (expr), 1); } else if (TREE_CODE (expr) == ARRAY_TYPE) bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
Hi, this patch fixes profiledbootstrap failure with LTO enabled. What happens is that alias_ptr_types_compatible_p relies on the fact that alias sets are not refined from WPA to ltrans time: /* This function originally abstracts from simply comparing get_deref_alias_set so that we are sure this still computes the same result after LTO type merging is applied. When in LTO type merging is done we can actually do this compare. */ if (in_lto_p) return get_deref_alias_set (t1) == get_deref_alias_set (t2); else return (TYPE_MAIN_VARIANT (TREE_TYPE (t1)) == TYPE_MAIN_VARIANT (TREE_TYPE (t2))); This conditional is confused - it pesimizes code with -fno-lto for no good reason. I will fix that separately: we now have lto_streaming_expected_p so I think it should read if (!lto_stremaing_expected_p () || flag_wpa) use alias sets else use main varaiants as conservative estimate. (so if we ever get idea to ICF during incremental link or deduplicate in early passes, things will work safely). I will send separate patch on this. Not refining alias sets from WPA to ltrans time is a good invariant to maintain and the canonical type hash behaves this way. However I broke this with the ODR logic. Normally we define canonical types for C++ ODR types according to their type names. However to make ODR types compatible with C types we check if structurally equivalent C type exists and if so, we ignore ODR names giving up on the precision. This however is not stable between WPA and ltrans since at ltrans the type merging does not see as many types as WPA does. To make this consistent the patch makes WPA ODR_TYPE_P == 0 for ODR types that conflicted with non-ODR type. I had to drop one sanity check in ipa-utils.h (that I think is not very important - I added it while introducing CXX_ODR_P machinery) and also it now may happen that we query odr_based_tbaa_p before registering first ODR type so we do not want to ICE here. ODR type registration happens early to produce ODR violation warings. Those are not done at ltrans, so dropping the registration is safe. The type will still be added while computing the type inheritance graph if needed for devirtualization (and late devirtualization is not very useful anyway since it won't enable inlining). Bootstrapped, regtested x86_64-linux, OK? I think this should go to release branches after some soaking in mainline too, even if we don't have direct reproducers. Note that Martin implemented type checker to sanity check that alias sets are never getting refined from compile to WPa and from WPA to ltrans. I recovered the patch and will play with it more. I think we should eventually establish this (if alias sets are refined from copmile time to WPA it is either wrong code issue or frontend alias sets are not as good as they should be), but of course there are fun issues. My plan is to see if I can identify some wrong code bugs and leave rest for early next stage1. PR bootstrap/97857 * ipa-devirt.c (odr_based_tbaa_p): Do not ICE when odr_hash is not initialized. * ipa-utils.h (type_with_linkage_p): Do not sanity check CXX_ODR_P. * lto-common.c (gimple_register_canonical_type_1): Only register types with TYPE_CXX_ODR_P flag; sanity check that no conflict happens at ltrans time. * tree-streamer-out.c (pack_ts_type_common_value_fields): Set CXX_ODR_P according to the canonical type.