From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89610 invoked by alias); 24 Nov 2015 08:58:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 89465 invoked by uid 89); 24 Nov 2015 08:58:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Tue, 24 Nov 2015 08:58:24 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 22913AC5F; Tue, 24 Nov 2015 08:57:00 +0000 (UTC) Date: Tue, 24 Nov 2015 09:00:00 -0000 From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: Fix verify_type ICE during Ada bootstrap In-Reply-To: <20151124082008.GA90197@kam.mff.cuni.cz> Message-ID: References: <20151124082008.GA90197@kam.mff.cuni.cz> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-11/txt/msg02844.txt.bz2 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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)