Hi,
On Mon, Nov 23, 2015 at 12:00:25AM +0100, Jan Hubicka wrote:
> Hi,
> here is updated patch which I finally comitted today. It addresses all the
> comments
> and also fixes one nasty bug that really cost me a lot of time to understand.
>
> + /* LTO type merging does not make any difference between
> + component pointer types. We may have
> +
> + struct foo {int *a;};
> +
> + as TYPE_CANONICAL of
> +
> + struct bar {float *a;};
> +
> + Because accesses to int * and float * do not alias, we would get
> + false negative when accessing the same memory location by
> + float ** and bar *. We thus record the canonical type as:
> +
> + struct {void *a;};
> +
> + void * is special cased and works as a universal pointer type.
> + Accesses to it conflicts with accesses to any other pointer
> + type. */
>
> This problem manifested itself only as a lto-bootstrap miscompare on 32bit
> build and I spent a lot of time localizing the wrong code since it reproduces
> only in quite large programs where we get conficts in canonical type merging
> like this.
>
> The patch thus updates record_component_aliases to substitute void_ptr_type
> for
> all pointer types. I re-did the stats. Now the improvement on dealII is 14%
> that is quite a bit lower than earlier, but still substantial. Since we have
> voidptr globing counter, I know that the number of disambiguations would go at
> least 19% up if we did not do it.
>
> THere is a lot of low hanging fruit in that area now, but the real solution
> is to
> track types that needs to be merge by fortran rules and don't do all this
> fancy
> globing for C/C++ types. I will open branch for IPA work and try to prepare
> this
> for next stage 1.
>
> bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested
> on i386-linux
> and also on some bigger apps, committed
>
> Note that we still have bootstrap miscompare with LTO build and
> --disable-checking,
> I am looking for that now. Additoinally after fixing the ICEs preventing us
> to build
> the gnat1 binary, gnat1 aborts. Both these are independent of the patch.
>
> Honza
> * lto.c (iterative_hash_canonical_type): Always recurse for pointers.
> (gimple_register_canonical_type_1): Check that pointers do not get
> canonical types.
> (gimple_register_canonical_type): Do not register pointers.
>
> * tree.c (build_pointer_type_for_mode,build_reference_type_for_mode):
> In LTO we do not compute TYPE_CANONICAL of pointers.
> (gimple_canonical_types_compatible_p): Improve coments; sanity check
> that pointers do not have canonical type that would make us believe
> they are different.
> * alias.c (get_alias_set): Do structural type equality on pointers;
> enable pointer path for LTO; also glob pointer to vector with pointer
> to vector element; glob pointers and references for LTO; do more strict
> sanity checking about build_pointer_type returning the canonical type
> which is also the main variant.
> (record_component_aliases): When component type is pointer and we
> do LTO; record void_type_node alias set.
...
> Index: alias.c
> ===================================================================
> --- alias.c (revision 230714)
> +++ alias.c (working copy)
> @@ -869,13 +869,23 @@ get_alias_set (tree t)
> set = lang_hooks.get_alias_set (t);
> if (set != -1)
> return set;
> - return 0;
> + /* Handle structure type equality for pointer types. This is easy
> + to do, because the code bellow ignore canonical types on these anyway.
> + This is important for LTO, where TYPE_CANONICAL for pointers can not
> + be meaningfuly computed by the frotnend. */
> + if (!POINTER_TYPE_P (t))
> + {
> + /* In LTO we set canonical types for all types where it makes
> + sense to do so. Double check we did not miss some type. */
> + gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t));
> + return 0;
I have hit this assert on our LTO tests when doing a merge from trunk
to the HSA branch. On the branch, we generate very simple static
constructors/destructors which just call libgomp (un)registration
routines to which we pass data in static variables of artificial
types. The assert happens inside varpool_node::finalize_decl calls on
those variables, e.g.:
lto1: internal compiler error: in get_alias_set, at alias.c:880
0x613650 get_alias_set(tree_node*)
/home/mjambor/gcc/branch/src/gcc/alias.c:880
0x71d2c7 set_mem_attributes_minus_bitpos(rtx_def*, tree_node*, int, long)
/home/mjambor/gcc/branch/src/gcc/emit-rtl.c:1772
0xd2d2f0 make_decl_rtl(tree_node*)
/home/mjambor/gcc/branch/src/gcc/varasm.c:1473
0xd310c7 assemble_variable(tree_node*, int, int, int)
/home/mjambor/gcc/branch/src/gcc/varasm.c:2144
0xd37b32 varpool_node::assemble_decl()
/home/mjambor/gcc/branch/src/gcc/varpool.c:580
0x6896aa varpool_node::finalize_decl(tree_node*)
/home/mjambor/gcc/branch/src/gcc/cgraphunit.c:820
0x844da1 hsa_output_kernels
/home/mjambor/gcc/branch/src/gcc/hsa-brig.c:1954
0x849f04 hsa_output_libgomp_mapping
/home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2116
0x849f04 hsa_output_brig()
/home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2356
(hsa_output_brig is called from compile_file in toplev.c)
I am going to commit the following hunk to the branch so that I can
get on with the merge. If you think it is wrong in any way, please
let me know so that we can find a proper solution.
Thanks,
Martin
--- /home/mjambor/gcc/trunk/src/gcc/alias.c 2015-11-23 11:45:27.850846512
+0100
+++ gcc/alias.c 2015-11-23 14:37:32.098133073 +0100
@@ -877,7 +877,9 @@ get_alias_set (tree t)
{
/* In LTO we set canonical types for all types where it makes
sense to do so. Double check we did not miss some type. */
- gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t));
+ gcc_checking_assert (!in_lto_p
+ || !type_with_alias_set_p (t)
+ || TYPE_ARTIFICIAL (t));
return 0;
}
}
> + }
> + }
> + else
> + {
> + t = TYPE_CANONICAL (t);
> + gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t));
> }
> -
> - t = TYPE_CANONICAL (t);
> -
> - /* The canonical type should not require structural equality checks. */
> - gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t));
>
> /* If this is a type with a known alias set, return it. */
> if (TYPE_ALIAS_SET_KNOWN_P (t))
...
