On Mon, 17 Feb 2014, Jan Hubicka wrote:
> > On Fri, 14 Feb 2014, Jan Hubicka wrote:
> >
> > > > > This smells bad, since it is given a canonical type that is after the
> > > > > structural equivalency merging that ignores BINFOs, so it may be
> > > > > completely
> > > > > different class with completely different bases than the original.
> > > > > Bases are
> > > > > structuraly merged, too and may be exchanged for normal fields because
> > > > > DECL_ARTIFICIAL (that separate bases and fields) does not seem to be
> > > > > part of
> > > > > the canonical type definition in LTO.
> > > >
> > > > Can you elaborate on that DECL_ARTIFICIAL thing? That is, what is
> > > > broken
> > > > by considering all fields during that merging?
> > >
> > > To make the code work with LTO, one can not merge
> > > struct B {struct A a}
> > > struct B: A {}
> > >
> > > these IMO differ only by DECL_ARTIFICIAL flag on the fields.
> >
> > "The code" == that BINFO walk? Is that because we walk a completely
>
> Yes.
>
> > unrelated BINFO chain? I'd say we should have merged its types
> > so that difference shouldn't matter.
> >
> > Hopefully ;)
>
> I am trying to make point that will matter. Here is completed testcase above:
>
> struct A {int a;};
> struct C:A {};
> struct B {struct A a;};
> struct C *p2;
> struct B *p1;
> int
> t()
> {
> p1->a.a = 2;
> return p2->a;
> }
>
> With patch I get:
>
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c (revision 207777)
> +++ lto/lto.c (working copy)
> @@ -49,6 +49,8 @@ along with GCC; see the file COPYING3.
> #include "data-streamer.h"
> #include "context.h"
> #include "pass_manager.h"
> +#include "print-tree.h"
>
>
> /* Number of parallel tasks to run, -1 if we want to use GNU Make jobserver.
> */
> @@ -619,6 +621,15 @@ gimple_canonical_type_eq (const void *p1
> {
> const_tree t1 = (const_tree) p1;
> const_tree t2 = (const_tree) p2;
> + if (gimple_canonical_types_compatible_p (CONST_CAST_TREE (t1),
> + CONST_CAST_TREE (t2))
> + && TREE_CODE (CONST_CAST_TREE (t1)) == RECORD_TYPE)
> + {
> + debug_tree (CONST_CAST_TREE (t1));
> + fprintf (stderr, "bases:%i\n", BINFO_BASE_BINFOS (TYPE_BINFO
> (t1))->length());
> + debug_tree (CONST_CAST_TREE (t2));
> + fprintf (stderr, "bases:%i\n", BINFO_BASE_BINFOS (TYPE_BINFO
> (t2))->length());
> + }
> return gimple_canonical_types_compatible_p (CONST_CAST_TREE (t1),
> CONST_CAST_TREE (t2));
> }
>
> <record_type 0x7ffff6c52888 B SI
> size <integer_cst 0x7ffff6ae83a0 type <integer_type 0x7ffff6ae5150
> bitsizetype> constant 32>
> unit size <integer_cst 0x7ffff6ae83c0 type <integer_type 0x7ffff6ae50a8
> sizetype> constant 4>
> align 32 symtab 0 alias set -1 canonical type 0x7ffff6c52888
> fields <field_decl 0x7ffff6adec78 a
> type <record_type 0x7ffff6c52738 A SI size <integer_cst
> 0x7ffff6ae83a0 32> unit size <integer_cst 0x7ffff6ae83c0 4>
> align 32 symtab 0 alias set -1 canonical type 0x7ffff6c52738
> fields <field_decl 0x7ffff6adebe0 a> context <translation_unit_decl
> 0x7ffff6af2e60 D.2821>
> chain <type_decl 0x7ffff6af2f18 A>>
> nonlocal SI file t.C line 3 col 20 size <integer_cst 0x7ffff6ae83a0
> 32> unit size <integer_cst 0x7ffff6ae83c0 4>
> align 32 offset_align 128
> offset <integer_cst 0x7ffff6ae8060 constant 0>
> bit offset <integer_cst 0x7ffff6ae80e0 constant 0> context
> <record_type 0x7ffff6c52888 B>
> chain <type_decl 0x7ffff6c55170 B type <record_type 0x7ffff6c52930 B>
> nonlocal VOID file t.C line 3 col 10
> align 1 context <record_type 0x7ffff6c52888 B> result
> <record_type 0x7ffff6c52888 B>>> context <translation_unit_decl
> 0x7ffff6af2e60 D.2821>
> pointer_to_this <pointer_type 0x7ffff6c529d8> chain <type_decl
> 0x7ffff6c550b8 B>>
> bases:0
> <record_type 0x7ffff6c52b28 C SI
> size <integer_cst 0x7ffff6ae83a0 type <integer_type 0x7ffff6ae5150
> bitsizetype> constant 32>
> unit size <integer_cst 0x7ffff6ae83c0 type <integer_type 0x7ffff6ae50a8
> sizetype> constant 4>
> align 32 symtab 0 alias set -1 structural equality
> fields <field_decl 0x7ffff6adeda8 D.2831
> type <record_type 0x7ffff6c52738 A SI size <integer_cst
> 0x7ffff6ae83a0 32> unit size <integer_cst 0x7ffff6ae83c0 4>
> align 32 symtab 0 alias set -1 canonical type 0x7ffff6c52738
> fields <field_decl 0x7ffff6adebe0 a> context <translation_unit_decl
> 0x7ffff6af2e60 D.2821>
> chain <type_decl 0x7ffff6af2f18 A>>
> ignored SI file t.C line 2 col 8 size <integer_cst 0x7ffff6ae83a0 32>
> unit size <integer_cst 0x7ffff6ae83c0 4>
> align 32 offset_align 128
> offset <integer_cst 0x7ffff6ae8060 constant 0>
> bit offset <integer_cst 0x7ffff6ae80e0 constant 0> context
> <record_type 0x7ffff6c52a80 C>
> chain <type_decl 0x7ffff6c552e0 C type <record_type 0x7ffff6c52b28 C>
> nonlocal VOID file t.C line 2 col 12
> align 1 context <record_type 0x7ffff6c52a80 C> result
> <record_type 0x7ffff6c52a80 C>>> context <translation_unit_decl
> 0x7ffff6af2e60 D.2821>
> chain <type_decl 0x7ffff6c55228 C>>
> bases:1
>
> So we prevail structure B with structure C. One has bases to walk other
> doesn't. If that BINFO walk in alias.c (on canonical types) did
> something useful, we have a wrong code bug.
Yeah, ok. But we treat those types (B and C) TBAA equivalent because
structurally they are the same ;) Luckily C has a "proper" field
for its base (proper means that offset and size are correct as well
as the type). It indeed has DECL_ARTIFICIAL set and yes, we treat
those as "real" fields when doing the structural comparison.
More interesting is of course when we can re-use tail-padding in
one but not the other (works as expected - not merged).
struct A { A (); short x; bool a;};
struct C:A { bool b; };
struct B {struct A a; bool b;};
struct C *p2;
struct B *p1;
int
t()
{
p1->a.a = 2;
return p2->a;
}
> Yes, zero sized classes are those having no fields (but other stuff,
> type decls, bases etc.)
Yeah, but TBAA obviously doesn't care about type decls and bases.
Richard.
