From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3870 invoked by alias); 11 Nov 2015 09:21:59 -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 3861 invoked by uid 89); 11 Nov 2015 09:21:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_PASS autolearn=no 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; Wed, 11 Nov 2015 09:21:57 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1C652AABB; Wed, 11 Nov 2015 09:21:34 +0000 (UTC) Date: Wed, 11 Nov 2015 09:21:00 -0000 From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: Enable pointer TBAA for LTO In-Reply-To: <20151110181515.GB78110@kam.mff.cuni.cz> Message-ID: References: <20151108204618.GA68715@kam.mff.cuni.cz> <20151110181515.GB78110@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/msg01321.txt.bz2 On Tue, 10 Nov 2015, Jan Hubicka wrote: > > > Index: tree.c > > > =================================================================== > > > --- tree.c (revision 229968) > > > +++ tree.c (working copy) > > > @@ -13198,6 +13198,7 @@ gimple_canonical_types_compatible_p (con > > > /* If the types have been previously registered and found equal > > > they still are. */ > > > if (TYPE_CANONICAL (t1) && TYPE_CANONICAL (t2) > > > + && !POINTER_TYPE_P (t1) && !POINTER_TYPE_P (t2) > > > > But TYPE_CANONICAL (t1) should be NULL_TREE for POINTER_TYPE_P? > > The reason is that TYPE_CANONICAL is initialized in get_alias_set that may be > called before we finish all merging and then it is more fine grained than what > we need here (i.e. TYPE_CANONICAL of pointers to two differnt types will be > different, but here we want them to be equal so we can match: > > struct aa { void *ptr;}; > struct bb { int * ptr;}; > > Which is actually required for Fortran interoperability. > > Removing this hunk triggers false type incompatibility warning in one of the > interoperability testcases I added. Ok, I see. > Even if I drop the code bellow setting TYPE_CANOINCAL, I think I need to keep > this conditional: the types may be built in and those get TYPE_CANONICAL set as > they are constructed by build_pointer_type. I can gcc_checking_assert for this > scenario and see. Perhaps we never build LTO type from builtin type and this > won't happen. If we did, we would probably have a trouble with false negatives > in return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); on non-pointers anyway. Hmm, indeed. The various type builders might end up setting TYPE_CANONICAL if you ever run into a pre-defined pointer type (ptr_type_node for example). > > > > > && trust_type_canonical) > > > return TYPE_CANONICAL (t1) == TYPE_CANONICAL (t2); > > > > > > Index: alias.c > > > =================================================================== > > > --- alias.c (revision 229968) > > > +++ alias.c (working copy) > > > @@ -869,13 +874,19 @@ get_alias_set (tree t) > > > set = lang_hooks.get_alias_set (t); > > > if (set != -1) > > > return set; > > > - return 0; > > > + /* LTO frontend does not assign canonical types to pointers (which we > > > + ignore anyway) and we compute them. The following path may be > > > + probably enabled for non-LTO, too, and it may improve TBAA for > > > + pointers to types with structural equality. */ > > > + if (!in_lto_p || !POINTER_TYPE_P (t)) > > > + return 0; > > > > No new LTO paths please, do the suggested change immediately. > > OK, I originally tested the patch without if and there was no problems. > Just chickened out before preparing final version of the patch. > > > + p = TYPE_MAIN_VARIANT (p); > > > + /* Normally all pointer types are built by > > > + build_pointer_type_for_mode which ensures they have canonical > > > + type unless they point to type with structural equality. > > > + LTO frontend produce pointer types without TYPE_CANONICAL > > > + that are then added to TYPE_POINTER_TO lists and > > > + build_pointer_type_for_mode will end up picking one for us. > > > + Declare it the canonical one. This is the same as > > > + build_pointer_type_for_mode would do. */ > > > + if (!TYPE_CANONICAL (p)) > > > + { > > > + TYPE_CANONICAL (p) = p; > > > + gcc_checking_assert (in_lto_p); > > > + } > > > + else > > > + gcc_checking_assert (p == TYPE_CANONICAL (p)); > > > > The assert can trigger as > > build_pointer_type_for_mode builds SET_TYPE_STRUCTURAL_EQUALITY pointer > > types for SET_TYPE_STRUCTURAL_EQUALITY pointed-to types. Ah, > > looking up more context reveals > > > > if (TREE_CODE (p) == VOID_TYPE || TYPE_STRUCTURAL_EQUALITY_P (p)) > > set = get_alias_set (ptr_type_node); > > Yep, we don't get here. > > > > Not sure why you adjust TYPE_CANONICAL here at all either. > > You are right, I may probably just drop all the code and just do: > gcc_checking_assert (!TYPE_CANONICAL || p == TYPE_CANONICAL (p)); > I will test this and re-think the build_pointer_type code to be sure that we > won't get into a problem there. > > As I recall, the original code > p = TYPE_CANONICAL (p); > was there to permit frontends to glob two pointers by setting same canonical > type to them. Yes. > My original plan was to use this for LTO frotnend and make > gimple_compare_canonical_types to do the right thing for pointers and this would > follow gimple_compare_canonical_types globbing then. > > This idea was wrong: since pointer rules are not transitive (i.e. void > * alias them all), we can't model that by an equivalence produced by > gimple_compare_canonical_types. > > Since the assert does not trigger, seems no frontend is doing that and moreover > I do not see how that would be useful (well, perhaps for some kind of internal > bookeeping when build TYPE_CANONICAL of more complex types from pointer types, > like arrays, but for those we ignore TYPE_CANONICAL anyway). Grepping over > TYPE_CANONICAL sets in frotneds, I see no code that I would suspect from doing > something like this. Ok. Let's see what your experiment shows, otherwise the original patch is ok. Thanks, Richard. > Thank you! > Honza > > > > Otherwise looks ok. > > > > RIchard. > > > > > > > } > > > - gcc_checking_assert (TYPE_CANONICAL (p) == p); > > > > > > /* Assign the alias set to both p and t. > > > We can not call get_alias_set (p) here as that would trigger > > > @@ -1015,11 +1043,6 @@ get_alias_set (tree t) > > > } > > > } > > > } > > > - /* In LTO the rules above needs to be part of canonical type machinery. > > > - For now just punt. */ > > > - else if (POINTER_TYPE_P (t) > > > - && t != TYPE_CANONICAL (ptr_type_node) && in_lto_p) > > > - set = get_alias_set (TYPE_CANONICAL (ptr_type_node)); > > > > > > /* Otherwise make a new alias set for this type. */ > > > else > > > > > > > > > > -- > > Richard Biener > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)