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:
> <array_ref 0x7ffff69c2968
> type <pointer_type 0x7ffff6942150
> type <integer_type 0x7ffff6cd50a8 char readonly string-flag QI
> size <integer_cst 0x7ffff6ad7ca8 constant 8>
> unit size <integer_cst 0x7ffff6ad7cc0 constant 1>
> align 8 symtab -158253232 alias set 0 canonical type
> 0x7ffff6adb498 precision 8 min <integer_cst 0x7ffff6cd2090 -128> max
> <integer_cst 0x7ffff6cd2078 127>
> pointer_to_this <pointer_type 0x7ffff6cd5150>>
> readonly unsigned DI
> size <integer_cst 0x7ffff6ad7bb8 constant 64>
> unit size <integer_cst 0x7ffff6ad7bd0 constant 8>
> align 64 symtab 0 alias set -1 canonical type 0x7ffff6af37e0
> pointer_to_this <pointer_type 0x7ffff69479d8>>
> readonly
> arg 0 <mem_ref 0x7ffff69d1028
> type <array_type 0x7ffff694b348 type <pointer_type 0x7ffff6942150>
> BLK
> align 64 symtab 0 alias set -1 structural equality
> pointer_to_this <pointer_type 0x7ffff694b3f0>>
>
> arg 0 <addr_expr 0x7ffff69ce380 type <pointer_type 0x7ffff694b3f0>
> constant arg 0 <var_decl 0x7ffff6941480 reg_note_name>>
> arg 1 <integer_cst 0x7ffff69b6678 constant 0>>
> arg 1 <ssa_name 0x7ffff69c5630
> type <integer_type 0x7ffff6adb690 int asm_written public SI
> size <integer_cst 0x7ffff6ad7df8 constant 32>
> unit size <integer_cst 0x7ffff6ad7e10 constant 4>
> align 32 symtab -158421968 alias set 3 canonical type
> 0x7ffff6adb690 precision 32 min <integer_cst 0x7ffff6ad7db0 -2147483648> max
> <integer_cst 0x7ffff6ad7dc8 2147483647>
> pointer_to_this <pointer_type 0x7ffff6af37e0> reference_to_this
> <reference_type 0x7ffff6942b28>>
> 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(tree_node*)::__FUNCTION__>
> "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.
