Fix verify_type ICE during Ada bootstrap

[Jan Hubicka]

> On Tue, 24 Nov 2015, Jan Hubicka wrote:
> 
> > Hi,
> > this patch fixes verify_type ICE while building Ada.  The problem is that
> > we end up with INTEGER_TYPE that has TYPE_ALIAS_SET buts its main variant
> > is integer_type_node that is built in LTO and has non-zero alias set.
> > 
> > This is will lead to wrong code if function using integer built with
> > -fno-strit-aliasing gets inlined, but I am not fixing this (dunno how).
> 
> Yeah, the usual issues with the pre-streamed nodes :/  (also for builtins
> which may vary with flags like -ffast-math)
> 
> Note that even with non-LTO using 
> attribute((optimize("no-strict-aliasing"))) has the same issue.  We might
> need to say that inlining no-strict-aliasing into strict-aliasing code
> is wrong.

Yeah, we may just rule out strict aliasing from optimization attribute if
we don't find better solution, becuase it is better than doing false promises.

However I hope we could do better...
> 
> Index: gcc/ipa-inline.c
> ===================================================================
> --- gcc/ipa-inline.c    (revision 230737)
> +++ gcc/ipa-inline.c    (working copy)
> @@ -410,7 +410,8 @@ can_inline_edge_p (struct cgraph_edge *e
>          Not even for always_inline declared functions.  */
>        /* Strictly speaking only when the callee contains signed integer
>           math where overflow is undefined.  */
> -     else if ((check_maybe_up (flag_strict_overflow)
> +     else if (((check_maybe_up (flag_strict_overflow)
> +               || check_maybe_down (flag_strict_aliasing))
>                /* this flag is set by optimize.  Allow inlining across
>                   optimize boundary.  */
>                && (!opt_for_fn (caller->decl, optimize)
> 
> maybe you can check the effect of this.

Sadly this has really bad effect on Firefox - here majority of code is built
with -fno-strict-aliasing and the benchmark sensitive part is -fstrict-aliasing.
Because we merge comdat inlines, we end up with non-strict-aliasing variants and
doing so prevents a lot of important inlining.

I know this from previous stage3 when we had this change and reverted it.
> 
> > This patch merely disables streaming of TYPE_ALIAS_SET==0 for non-main
> > variants, because it is consistently ignored by get_alias_set anyway and adds
> > corresponding testing. I also took liberty to optimize
> > pack_ts_type_common_value_fields by ordering the constant flags first and
> > turning TYPE_ALIAS_SET streaming to a bit instead of var_len_int that is either
> > 0 or -1.
> > 
> > I suppose -fno-strict-aliasing can be saved only by making all TYPE_ALIAS_SET==0
> > types variant and making get_alias_set honor it. When mixing -fstrict-aliasing
> > and -fno-strict-aliasing we will have different type variants on each code
> > which will stick. THe canonical type machinery then can preffer the non-0
> > variant.
> 
> Well, as we'll keep different main varinants and variants for the
> strict-aliasing vs. the no-strict-aliasing case we simply need to make
> sure to not look at TYPE_CANONICAL before testing TYPE_ALIAS_SET_KNOWN_P
> on the main variant.  The special-handling relies on that being the
> case and the alias-set being zero for all used types.

Yeah, that was what I had in mind.
> 
> > This seems like a deeper change - at least I do not see how to make 
> > prestreamed types to work with this sense easily. Another variant would 
> > be to put the info directly to MEM_REF that is probably better and 
> > easier and drop forcing TYPE_ALIAS_SET==0. Then however we will need to 
> > wrap all memory accesses into MEM_REF, even non-LTO.
> 
> We do already wrap all bases into MEM_REFs at streaming time, it would
> be easy to adjust it to make it effectively alias-set zero.  But of
> course the overhead and the downstream effects of having more MEM_REFs
> (we strip the unneeded ones at stream-in) are unknown (compared to
> the effect of disabling inlining).

Hmm, I can test in on Firefox (once I get it back to working condition).

An alternative I was thinking about was to keep duplicated bodies in the
callgraph for inlines that may get impossible after declmerging.  That
also needs works and the consequences on code size estimates and overall
binary size are bit hard to guess.

This may be good solution for the overall problem of mixing flags, but
my gut feeling is that we probably win most score by doing the memrefs.
> 
> The prestreamed types should work for all function bodies with
> optimize(no-strict-aliasing) via the flag_no_strict_aliasing check
> in get_alias_set.

Yeah, except for inlining that is important in this case and moreover
there is oposite problem too. When we stream in -fno-strict-aliasing code
first, we end up with types in alias set 0 as canonical types that leads
to bad TBAA, too.
> 
> > Bootstrapped/regtested x86_64-linux with and without LTO including ada.
> > Requires the earlier VECTOR_TYPE change to pass cleanly testsuite, otherwise it
> > fails on 2 vectorizer testcases.
> > 
> > OK?
> 
> Ok.
Thanks.

Honza
> 
> Thanks,
> Richard.
> 
> > Honza
> > 	* alias.c (get_alias_set): Before checking TYPE_ALIAS_SET_KNOWN_P
> > 	double check that type is main variant.
> > 	* tree.c (build_variant_type_copy): Clear TYPE_ALIAS_SET when producing
> > 	variant.
> > 	(verify_type_variant): Verify that variants have no
> > 	TYPE_ALIAS_SET_KNOWN_P set
> > 	* tree-streamer-out.c (pack_ts_type_common_value_fields): Reorder
> > 	streaming so constant fields come first; stream TYPE_ALIAS_SET==0
> > 	only for main variants; stream TYPE_ALIAS_SET as a bit.
> > 	* tree-streamer-in.c (unpack_ts_type_common_value_fields): Update
> > 	accordingly.
> > Index: alias.c
> > ===================================================================
> > --- alias.c	(revision 230783)
> > +++ alias.c	(working copy)
> > @@ -888,6 +888,7 @@ get_alias_set (tree t)
> >      }
> >  
> >    /* If this is a type with a known alias set, return it.  */
> > +  gcc_checking_assert (t == TYPE_MAIN_VARIANT (t));
> >    if (TYPE_ALIAS_SET_KNOWN_P (t))
> >      return TYPE_ALIAS_SET (t);
> >  
> > @@ -1025,6 +1026,7 @@ get_alias_set (tree t)
> >  	     We can not call get_alias_set (p) here as that would trigger
> >  	     infinite recursion when p == t.  In other cases it would just
> >  	     trigger unnecesary legwork of rebuilding the pointer again.  */
> > +	  gcc_checking_assert (p == TYPE_MAIN_VARIANT (p));
> >  	  if (TYPE_ALIAS_SET_KNOWN_P (p))
> >  	    set = TYPE_ALIAS_SET (p);
> >  	  else
> > Index: tree.c
> > ===================================================================
> > --- tree.c	(revision 230783)
> > +++ tree.c	(working copy)
> > @@ -6706,6 +6706,7 @@ build_variant_type_copy (tree type)
> >    /* Since we're building a variant, assume that it is a non-semantic
> >       variant. This also propagates TYPE_STRUCTURAL_EQUALITY_P. */
> >    TYPE_CANONICAL (t) = TYPE_CANONICAL (type);
> > +  /* Type variants have no alias set defined.  */
> > +  TYPE_ALIAS_SET (t) = -1;
> >  
> >    /* Add the new type to the chain of variants of TYPE.  */
> >    TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (m);
> > @@ -13056,8 +13058,12 @@ verify_type_variant (const_tree t, tree
> >    if ((!in_lto_p || !TYPE_FILE_SCOPE_P (t)) && 0)
> >      verify_variant_match (TYPE_CONTEXT);
> >    verify_variant_match (TYPE_STRING_FLAG);
> > -  if (TYPE_ALIAS_SET_KNOWN_P (t) && TYPE_ALIAS_SET_KNOWN_P (tv))
> > -    verify_variant_match (TYPE_ALIAS_SET);
> > +  if (TYPE_ALIAS_SET_KNOWN_P (t))
> > +    {
> > +      error ("type variant with TYPE_ALIAS_SET_KNOWN_P");
> > +      return false;
> > +    }
> >  
> >    /* tree_type_non_common checks.  */
> >  
> > Index: tree-streamer-out.c
> > ===================================================================
> > --- tree-streamer-out.c	(revision 230783)
> > +++ tree-streamer-out.c	(working copy)
> > @@ -313,6 +313,17 @@ pack_ts_type_common_value_fields (struct
> >    /* TYPE_NO_FORCE_BLK is private to stor-layout and need
> >       no streaming.  */
> >    bp_pack_value (bp, TYPE_NEEDS_CONSTRUCTING (expr), 1);
> > +  bp_pack_value (bp, TYPE_PACKED (expr), 1);
> > +  bp_pack_value (bp, TYPE_RESTRICT (expr), 1);
> > +  bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
> > +  bp_pack_value (bp, TYPE_READONLY (expr), 1);
> > +  /* Make sure to preserve the fact whether the frontend would assign
> > +     alias-set zero to this type.  Do that only for main variants, because
> > +     type variants alias sets are never computed.
> > +     FIXME:  This does not work for pre-streamed builtin types.  */
> > +  bp_pack_value (bp, (TYPE_ALIAS_SET (expr) == 0
> > +		      || (!in_lto_p && TYPE_MAIN_VARIANT (expr) == expr
> > +			  && get_alias_set (expr) == 0)), 1);
> >    if (RECORD_OR_UNION_TYPE_P (expr))
> >      {
> >        bp_pack_value (bp, TYPE_TRANSPARENT_AGGR (expr), 1);
> > @@ -320,17 +331,8 @@ pack_ts_type_common_value_fields (struct
> >      }
> >    else if (TREE_CODE (expr) == ARRAY_TYPE)
> >      bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> > -  bp_pack_value (bp, TYPE_PACKED (expr), 1);
> > -  bp_pack_value (bp, TYPE_RESTRICT (expr), 1);
> > -  bp_pack_value (bp, TYPE_USER_ALIGN (expr), 1);
> > -  bp_pack_value (bp, TYPE_READONLY (expr), 1);
> >    bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr));
> >    bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr));
> > -  /* Make sure to preserve the fact whether the frontend would assign
> > -     alias-set zero to this type.  */
> > -  bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0
> > -			    || (!in_lto_p
> > -				&& get_alias_set (expr) == 0)) ? 0 : -1);
> >  }
> >  
> >  
> > Index: tree-streamer-in.c
> > ===================================================================
> > --- tree-streamer-in.c	(revision 230783)
> > +++ tree-streamer-in.c	(working copy)
> > @@ -362,6 +362,11 @@ unpack_ts_type_common_value_fields (stru
> >    /* TYPE_NO_FORCE_BLK is private to stor-layout and need
> >       no streaming.  */
> >    TYPE_NEEDS_CONSTRUCTING (expr) = (unsigned) bp_unpack_value (bp, 1);
> > +  TYPE_PACKED (expr) = (unsigned) bp_unpack_value (bp, 1);
> > +  TYPE_RESTRICT (expr) = (unsigned) bp_unpack_value (bp, 1);
> > +  TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1);
> > +  TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1);
> > +  TYPE_ALIAS_SET (expr) = bp_unpack_value (bp, 1) ? 0 : -1;
> >    if (RECORD_OR_UNION_TYPE_P (expr))
> >      {
> >        TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> > @@ -369,17 +374,12 @@ unpack_ts_type_common_value_fields (stru
> >      }
> >    else if (TREE_CODE (expr) == ARRAY_TYPE)
> >      TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> > -  TYPE_PACKED (expr) = (unsigned) bp_unpack_value (bp, 1);
> > -  TYPE_RESTRICT (expr) = (unsigned) bp_unpack_value (bp, 1);
> > -  TYPE_USER_ALIGN (expr) = (unsigned) bp_unpack_value (bp, 1);
> > -  TYPE_READONLY (expr) = (unsigned) bp_unpack_value (bp, 1);
> >    TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp);
> >    TYPE_ALIGN (expr) = bp_unpack_var_len_unsigned (bp);
> >  #ifdef ACCEL_COMPILER
> >    if (TYPE_ALIGN (expr) > targetm.absolute_biggest_alignment)
> >      TYPE_ALIGN (expr) = targetm.absolute_biggest_alignment;
> >  #endif
> > -  TYPE_ALIAS_SET (expr) = bp_unpack_var_len_int (bp);
> >  }
> >  
> >  
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)