From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103699 invoked by alias); 22 May 2015 08:12:25 -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 103628 invoked by uid 89); 22 May 2015 08:12:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.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; Fri, 22 May 2015 08:12:23 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9B806AB5F; Fri, 22 May 2015 08:12:19 +0000 (UTC) Date: Fri, 22 May 2015 08:28:00 -0000 From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: Calculate TYPE_CANONICAL only for types that can be accessed in memory In-Reply-To: <20150521181437.GB8821@kam.mff.cuni.cz> Message-ID: References: <20150521181437.GB8821@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-05/txt/msg02064.txt.bz2 On Thu, 21 May 2015, Jan Hubicka wrote: > Hi, > this is next part of the series. It disables canonical type calculation for > incomplete types with exception of arrays based on claim that we do not have > good notion of those. > > I can botostrap this with additional checks in alias.c that canonical types are > always present with LTO but I need fix to ICF that compare alias sets of types > it does not need to and trips incomplete types otherwise. I will push out > these fixes separately and incrementally add the fix. The purpose of those > checks is to avoid alias.c degenerating to structural equality path for no good > reason. > > I tried the alternative to disable it on ARRAY_TYPES too and add avoid > recursion to those for fields. THis does not fly because we can have > ARRAY_REFS of incomplete types: > type type size > unit size > align 8 symtab -158253232 alias set 0 canonical type 0x7ffff6adb498 precision 8 min max > pointer_to_this > > readonly unsigned DI > size > unit size > align 64 symtab 0 alias set -1 canonical type 0x7ffff6af37e0 > pointer_to_this > > readonly > arg 0 type > BLK > align 64 symtab 0 alias set -1 structural equality > pointer_to_this > > > arg 0 > constant arg 0 > > arg 1 > > arg 1 type size > unit size > align 32 symtab -158421968 alias set 3 canonical type 0x7ffff6adb690 precision 32 min max > pointer_to_this reference_to_this > > visiteddef_stmt _103 = (int) _101; > > version 103 > ptr-info 0x7ffff69f04a0> > ../../gcc/print-rtl.c:173:4> > > and we compute alias set for it via: > #0 internal_error (gmsgid=0x1b86c8f "in %s, at %s:%d") at ../../gcc/diagnostic.c:1271 > #1 0x00000000015e2416 in fancy_abort (file=0x167ea2a "../../gcc/alias.c", line=823, function=0x167f7d6 "get_alias_set") > at ../../gcc/diagnostic.c:1341 > #2 0x00000000007109b9 in get_alias_set (t=0x7ffff694b2a0) at ../../gcc/alias.c:823 > #3 0x000000000070fecf in component_uses_parent_alias_set_from (t=0x7ffff69c2968) at ../../gcc/alias.c:607 > #4 0x0000000000710497 in reference_alias_ptr_type_1 (t=0x7fffffffe068) at ../../gcc/alias.c:719 > #5 0x00000000007107e8 in get_alias_set (t=0x7ffff69c2968) at ../../gcc/alias.c:799 > #6 0x0000000000ebca97 in vn_reference_lookup (op=0x7ffff69c2968, vuse=0x7ffff69ca798, kind=VN_WALKREWRITE, vnresult=0x0) at ../../gcc/tree-ssa-sccvn.c:2217 > #7 0x0000000000ebea99 in visit_reference_op_load (lhs=0x7ffff69c5678, op=0x7ffff69c2968, stmt=0x7ffff69cf730) at ../../gcc/tree-ssa-sccvn.c:3030 > #8 0x0000000000ec05ec in visit_use (use=0x7ffff69c5678) at ../../gcc/tree-ssa-sccvn.c:3685 > #9 0x0000000000ec1047 in process_scc (scc=...) at ../../gcc/tree-ssa-sccvn.c:3927 > #10 0x0000000000ec1679 in extract_and_process_scc_for_name (name=0x7ffff69c5678) at ../../gcc/tree-ssa-sccvn.c:4013 > #11 0x0000000000ec1848 in DFS (name=0x7ffff69c5678) at ../../gcc/tree-ssa-sccvn.c:4065 > #12 0x0000000000ec26d1 in cond_dom_walker::before_dom_children (this=0x7fffffffe5a0, bb=0x7ffff69b9888) at ../../gcc/tree-ssa-sccvn.c:4345 > #13 0x00000000014c05c0 in dom_walker::walk (this=0x7fffffffe5a0, bb=0x7ffff69b9888) at ../../gcc/domwalk.c:188 > #14 0x0000000000ec2b0e in run_scc_vn (default_vn_walk_kind_=VN_WALKREWRITE) at ../../gcc/tree-ssa-sccvn.c:4436 > #15 0x0000000000e98d59 in (anonymous namespace)::pass_fre::execute (this=0x1f621b0, fun=0x7ffff698db28) at ../../gcc/tree-ssa-pre.c:4972 > #16 0x0000000000bb6c8f in execute_one_pass (pass=0x1f621b0) at ../../gcc/passes.c:2317 > #17 0x0000000000bb6ede in execute_pass_list_1 (pass=0x1f621b0) at ../../gcc/passes.c:2370 > #18 0x0000000000bb6f0f in execute_pass_list_1 (pass=0x1f61d90) at ../../gcc/passes.c:2371 > #19 0x0000000000bb6f51 in execute_pass_list (fn=0x7ffff698db28, pass=0x1f61cd0) at ../../gcc/passes.c:2381 > #20 0x00000000007bb3f6 in cgraph_node::expand (this=0x7ffff695b000) at ../../gcc/cgraphunit.c:1895 > #21 0x00000000007bba15 in expand_all_functions () at ../../gcc/cgraphunit.c:2031 > #22 0x00000000007bc4e9 in symbol_table::compile (this=0x7ffff6adb000) at ../../gcc/cgraphunit.c:2384 > #23 0x00000000006f846c in lto_main () at ../../gcc/lto/lto.c:3315 > #24 0x0000000000cb465f in compile_file () at ../../gcc/toplev.c:594 > #25 0x0000000000cb6bb8 in do_compile () at ../../gcc/toplev.c:2081 > #26 0x0000000000cb6e04 in toplev::main (this=0x7fffffffe860, argc=33, argv=0x7fffffffe968) at ../../gcc/toplev.c:2182 > #27 0x00000000015c9739 in main (argc=33, argv=0x7fffffffe968) at ../../gcc/main.c:39 > > Though few lines down alias.c globs to element type: > if (TREE_CODE (t) == ARRAY_TYPE && !TYPE_NONALIASED_COMPONENT (t)) > set = get_alias_set (TREE_TYPE (t)); > > I supose we can move it up and then skip calculation of those, but sollution > bellow makes sense to me. Type has TYPE_CANONICAL if it (or its parts) can be > accessed. Incomplete array fields can be accessed and thus need > TYPE_CANONICAL. Yes, that notion makes sense. > LTO bootstrapped on x86_64-linux, regtested with other patches, I am > re-testing in isolation OK? > > Honza > > * lto/lto.c (hash_canonical_type): Check that we hash only > complete types or incomplete arrays. > (gimple_register_canonical_type): Do not compute canonical type > of types where it does not matter. > * tree.c (gimple_canonical_types_compatible_p): Sanity check that > we do not compute canonical type based on incomplete type. > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 223490) > +++ lto/lto.c (working copy) > @@ -309,6 +309,13 @@ hash_canonical_type (tree type) > { > inchash::hash hstate; > > + /* We can hash only types that can be accessed in memory. > + Those are complete types and arrays of incomplete type but complete > + TREE_TYPE. Verify we do not recurse to something else. */ > + gcc_checking_assert (COMPLETE_TYPE_P (type) > + || (TREE_CODE (type) == ARRAY_TYPE > + && COMPLETE_TYPE_P (TREE_TYPE (type)))); > + > /* Combine a few common features of types so that types are grouped into > smaller sets; when searching for existing matching types to merge, > only existing types having the same features as the new type will be > @@ -496,6 +499,22 @@ gimple_register_canonical_type (tree t) > if (TYPE_CANONICAL (t)) > return; > > + /* No need for canonical types of functions and methods; those are never > + accessed as memory locations. */ > + if (TREE_CODE (t) == FUNCTION_TYPE || TREE_CODE (t) == METHOD_TYPE) > + return; Just occured to me that it might make sense to remove the FUNCTION/METHOD_TYPE case in useless_type_conversion_p (I wonder in which cases we enter up in that path...). > + /* Incomplete structures/unions does not have good definition of > + canonical type at inter-module level because they are compatible with > + every complete type (as oposed to every complete type of same name > + with non-LTO build). Do not compute canonical types for those, because > + they can not be accessed anyway. > + > + However canonical types of arrays matters, because even if outer dimension > + is unknown, its elements still can be accessed. */ > + if (!COMPLETE_TYPE_P (t) > + && (TREE_CODE (t) != ARRAY_TYPE || !COMPLETE_TYPE_P (TREE_TYPE (t)))) > + return; > + > gimple_register_canonical_type_1 (t, hash_canonical_type (t)); > } > > Index: tree.c > =================================================================== > --- tree.c (revision 223490) > +++ tree.c (working copy) > @@ -12720,6 +12720,28 @@ gimple_canonical_types_compatible_p (con > if (t1 == NULL_TREE || t2 == NULL_TREE) > return false; > > + /* We consider complete types always compatible with incomplete type. > + This does not make sense for canonical type calculation and thus we > + need to ensure that we are never called on it. > + > + FIXME: For more correctness the function probably should have three modes > + 1) mode assuming that types are complete mathcing their structure matching > + 2) mode allowing incomplete types but producing equivalence classes > + and thus ignoring all info from complete types > + 3) mode allowing incomplete types to match complete but checking > + compatibility between complete types. > + > + 1 and 2 can be used for canonical type calculation. 3 is the real > + definition of type compatibility that can be used i.e. for warnings during > + declaration merging. */ > + > + gcc_assert (!trust_type_canonical gcc_checking_assert, like the one in the hashing path? > + || ((COMPLETE_TYPE_P (t1) > + || (TREE_CODE (t1) == ARRAY_TYPE > + && COMPLETE_TYPE_P (TREE_TYPE (t1)))) > + && (COMPLETE_TYPE_P (t2) > + || (TREE_CODE (t2) == ARRAY_TYPE > + && COMPLETE_TYPE_P (TREE_TYPE (t2)))))); Now we have it spelled out 4 times ... makes sense to create a new macro for it? (though I cannot think of a good name... UNACCESSIBLE_TYPE_P ()?) Otherwise looks ok to me. Thanks, Richard.