Make ltrans type canonicals compatible with WPA ones

Richard Biener rguenther@suse.de
Tue Nov 17 13:13:59 GMT 2020


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


More information about the Gcc-patches mailing list