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.
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.
> 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.
> 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).
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.
> 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,
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 <[email protected]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
