On Mon, 23 Nov 2015, Martin Jambor wrote:
> 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 think it also causes the following and one related ICE
FAIL: gcc.dg/vect/pr62021.c -flto -ffat-lto-objects (internal compiler
error)
/space/rguenther/src/svn/trunk3/gcc/testsuite/gcc.dg/vect/pr62021.c:7:1:
internal compiler error: in get_alias_set, at alias.c:880^M
0x7528a7 get_alias_set(tree_node*)^M
/space/rguenther/src/svn/trunk3/gcc/alias.c:880^M
0x751ce5 component_uses_parent_alias_set_from(tree_node const*)^M
/space/rguenther/src/svn/trunk3/gcc/alias.c:635^M
0x7522ad reference_alias_ptr_type_1^M
/space/rguenther/src/svn/trunk3/gcc/alias.c:747^M
0x752683 get_alias_set(tree_node*)^M
...
Richard.
