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);
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
