Fix SPEC gcc micompile with LTO
Hi, this patch fixes the miscompare I introduced to spec2006 GCC benchmark when build with LTO. The problem is that fld_incomplete_type_of builds new pointer type to incomplete type rather than complete but it ends up giving wrong type canonical. This patch also improves TBAA with early opts because we do no lose info by producing incomplete variants. Note that build_pointer_type may return existing type and in that case I overwrite TYPE_CANONICAL of it, but I believe it should be harmless because all pointers to a given type should have canonicals constructed same way. lto-bootstrapped/regtested x86_64-linux. Honza * gcc.dg/lto/tbaa-1.c: New testcase. * tree.c (fld_incomplete_type_of): Copy TYPE_CANONICAL while creating pointer type. Index: testsuite/gcc.dg/lto/tbaa-1.c === --- testsuite/gcc.dg/lto/tbaa-1.c (nonexistent) +++ testsuite/gcc.dg/lto/tbaa-1.c (working copy) @@ -0,0 +1,41 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -flto -fdump-tree-evrp" } */ +typedef struct rtx_def *rtx; +typedef struct cselib_val_struct +{ + union + { + } u; + struct elt_loc_list *locs; +} +cselib_val; +struct elt_loc_list +{ + struct elt_loc_list *next; + rtx loc; +}; +static int n_useless_values; +unchain_one_elt_loc_list (pl) + struct elt_loc_list **pl; +{ + struct elt_loc_list *l = *pl; + *pl = l->next; +} + +discard_useless_locs (x, info) + void **x; +{ + cselib_val *v = (cselib_val *) * x; + struct elt_loc_list **p = &v->locs; + int had_locs = v->locs != 0; + while (*p) +{ + unchain_one_elt_loc_list (p); + p = &(*p)->next; +} + if (had_locs && v->locs == 0) +{ + n_useless_values++; +} +} +/* { dg-final { scan-tree-dump-times "n_useless_values" 2 "evrp" } } */ Index: tree.c === --- tree.c (revision 265766) +++ tree.c (working copy) @@ -5146,6 +5146,7 @@ fld_incomplete_type_of (tree t, struct f else first = build_reference_type_for_mode (t2, TYPE_MODE (t), TYPE_REF_CAN_ALIAS_ALL (t)); + TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); }
Re: Fix SPEC gcc micompile with LTO
On Mon, 5 Nov 2018, Jan Hubicka wrote: > Hi, > this patch fixes the miscompare I introduced to spec2006 GCC benchmark > when build with LTO. > The problem is that fld_incomplete_type_of builds new pointer type to > incomplete type rather than complete but it ends up giving wrong type > canonical. > > This patch also improves TBAA with early opts because we do no lose info > by producing incomplete variants. > Note that build_pointer_type may return existing type and in that case I > overwrite TYPE_CANONICAL of it, but I believe it should be harmless > because all pointers to a given type should have canonicals constructed > same way. > > lto-bootstrapped/regtested x86_64-linux. > > Honza > * gcc.dg/lto/tbaa-1.c: New testcase. > * tree.c (fld_incomplete_type_of): Copy TYPE_CANONICAL while creating > pointer type. > Index: testsuite/gcc.dg/lto/tbaa-1.c > === > --- testsuite/gcc.dg/lto/tbaa-1.c (nonexistent) > +++ testsuite/gcc.dg/lto/tbaa-1.c (working copy) > @@ -0,0 +1,41 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -flto -fdump-tree-evrp" } */ > +typedef struct rtx_def *rtx; > +typedef struct cselib_val_struct > +{ > + union > + { > + } u; > + struct elt_loc_list *locs; > +} > +cselib_val; > +struct elt_loc_list > +{ > + struct elt_loc_list *next; > + rtx loc; > +}; > +static int n_useless_values; > +unchain_one_elt_loc_list (pl) > + struct elt_loc_list **pl; > +{ > + struct elt_loc_list *l = *pl; > + *pl = l->next; > +} > + > +discard_useless_locs (x, info) > + void **x; > +{ > + cselib_val *v = (cselib_val *) * x; > + struct elt_loc_list **p = &v->locs; > + int had_locs = v->locs != 0; > + while (*p) > +{ > + unchain_one_elt_loc_list (p); > + p = &(*p)->next; > +} > + if (had_locs && v->locs == 0) > +{ > + n_useless_values++; > +} > +} > +/* { dg-final { scan-tree-dump-times "n_useless_values" 2 "evrp" } } */ > > Index: tree.c > === > --- tree.c(revision 265766) > +++ tree.c(working copy) > @@ -5146,6 +5146,7 @@ fld_incomplete_type_of (tree t, struct f > else > first = build_reference_type_for_mode (t2, TYPE_MODE (t), > TYPE_REF_CAN_ALIAS_ALL (t)); > + TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); Hmm, this _should_ be a no-op. Can you, before that line, add gcc_assert (TYPE_CANONICAL (t2) != t2 && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); ? That is, the incomplete variant should share TYPE_CANONICAL with the pointed-to type and be _not_ the canonical leader (otherwise all other pointer types are bogus). > add_tree_to_fld_list (first, fld); > return fld_type_variant (first, t, fld); > } > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Re: Fix SPEC gcc micompile with LTO
> Hmm, this _should_ be a no-op. Can you, before that line, add > > gcc_assert (TYPE_CANONICAL (t2) != t2 > && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > > ? That is, the incomplete variant should share TYPE_CANONICAL with > the pointed-to type and be _not_ the canonical leader (otherwise > all other pointer types are bogus). It looks like good idea. I am re-checking with that change that already found a bug - build_distinct_type_variant actually resets TYPE_CANONICAL which I have missed earlier. So I am testing Index: tree.c === --- tree.c (revision 265807) +++ tree.c (working copy) @@ -5146,6 +5146,9 @@ fld_incomplete_type_of (tree t, struct f else first = build_reference_type_for_mode (t2, TYPE_MODE (t), TYPE_REF_CAN_ALIAS_ALL (t)); + gcc_assert (TYPE_CANONICAL (t2) != t2 + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); + TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); } @@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f SET_TYPE_MODE (copy, VOIDmode); SET_TYPE_ALIGN (copy, BITS_PER_UNIT); TYPE_SIZE_UNIT (copy) = NULL; + TYPE_CANONICAL (copy) = t; if (AGGREGATE_TYPE_P (t)) { TYPE_FIELDS (copy) = NULL; Honza
Re: Fix SPEC gcc micompile with LTO
On Mon, 5 Nov 2018, Jan Hubicka wrote: > > Hmm, this _should_ be a no-op. Can you, before that line, add > > > > gcc_assert (TYPE_CANONICAL (t2) != t2 > > && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > > > > ? That is, the incomplete variant should share TYPE_CANONICAL with > > the pointed-to type and be _not_ the canonical leader (otherwise > > all other pointer types are bogus). > > It looks like good idea. I am re-checking with that change that already > found a bug - build_distinct_type_variant actually resets TYPE_CANONICAL > which I have missed earlier. So I am testing > > Index: tree.c > === > --- tree.c(revision 265807) > +++ tree.c(working copy) > @@ -5146,6 +5146,9 @@ fld_incomplete_type_of (tree t, struct f > else > first = build_reference_type_for_mode (t2, TYPE_MODE (t), > TYPE_REF_CAN_ALIAS_ALL (t)); > + gcc_assert (TYPE_CANONICAL (t2) != t2 > + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > + TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); as said the TYPE_CANONICAL assign should be already done exactly this way in build_{poitner,reference}_for_mode. So you should be able to drop this from the patch. > add_tree_to_fld_list (first, fld); > return fld_type_variant (first, t, fld); > } > @@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f > SET_TYPE_MODE (copy, VOIDmode); > SET_TYPE_ALIGN (copy, BITS_PER_UNIT); > TYPE_SIZE_UNIT (copy) = NULL; > + TYPE_CANONICAL (copy) = t; Or use build_variant_type_copy in the first place? But you do not seme to queue the new types in the variant list? > if (AGGREGATE_TYPE_P (t)) > { > TYPE_FIELDS (copy) = NULL; > > Honza > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Re: Fix SPEC gcc micompile with LTO
> > + gcc_assert (TYPE_CANONICAL (t2) != t2 > > + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > > + TYPE_CANONICAL (first) = TYPE_CANONICAL (TYPE_MAIN_VARIANT (t)); > > as said the TYPE_CANONICAL assign should be already done exactly this > way in build_{poitner,reference}_for_mode. So you should be able to > drop this from the patch. OK, I have turned this into a sanity check and re-testing. > > > add_tree_to_fld_list (first, fld); > > return fld_type_variant (first, t, fld); > > } > > @@ -5169,6 +5172,7 @@ fld_incomplete_type_of (tree t, struct f > > SET_TYPE_MODE (copy, VOIDmode); > > SET_TYPE_ALIGN (copy, BITS_PER_UNIT); > > TYPE_SIZE_UNIT (copy) = NULL; > > + TYPE_CANONICAL (copy) = t; > > Or use build_variant_type_copy in the first place? But you do not > seme to queue the new types in the variant list? build_variant_type_copy would set TYPE_MAIN_VARAINT (copy) to be T. I do not want this to happen since streaming would then pick complete type as well. Honza
Re: Fix SPEC gcc micompile with LTO
Hi, this is patch I ended up testing. It ensures that canonical types of copies I create are same as of originals C++ FE has its own refernece type construction (cp_build_reference_type) and it creates additional pointer types with TYPE_REF_IS_RVALUE set and it has different TYPE_CANONICAL. Obviously we do not see this in middle-end and we end up merging the types despite fact they have different TYPE_CANONICAL. I guess I can immitate the behaviour in fld_incomplete_type_of by implementing my own variant of build_pointer_type that also matches TYPE_CANONICAL of the pointer it creates. I wonder if there are better solutions? Honza Index: tree.c === --- tree.c (revision 265807) +++ tree.c (working copy) @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t); TYPE_NAME (v) = TYPE_NAME (t); TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t); + TYPE_CANONICAL (v) = TYPE_CANONICAL (t); add_tree_to_fld_list (v, fld); return v; } @@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f else first = build_reference_type_for_mode (t2, TYPE_MODE (t), TYPE_REF_CAN_ALIAS_ALL (t)); + gcc_assert (TYPE_CANONICAL (t2) != t2 + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)) + && TYPE_CANONICAL (first) +== TYPE_CANONICAL (TYPE_MAIN_VARIANT (t))); add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); } @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f SET_TYPE_MODE (copy, VOIDmode); SET_TYPE_ALIGN (copy, BITS_PER_UNIT); TYPE_SIZE_UNIT (copy) = NULL; + TYPE_CANONICAL (copy) = TYPE_CANONICAL (t); if (AGGREGATE_TYPE_P (t)) { TYPE_FIELDS (copy) = NULL;
Re: Fix SPEC gcc micompile with LTO
> Hi, > this is patch I ended up testing. It ensures that canonical types of > copies I create are same as of originals C++ FE has its own refernece piece of mail got lost rendering the paragraph unreadable. I wanted to say: This is patch I ended up testing. It ensures that canonical types of copies I create are same as of originals. It however ICEs building auto-profile.c because C++ FE has its own reference type construction (cp_build_reference_type) and it creates additional pointer types with TYPE_REF_IS_RVALUE set and it has different TYPE_CANONICAL. > > Obviously we do not see this in middle-end and we end up merging the > types despite fact they have different TYPE_CANONICAL. > I guess I can immitate the behaviour in fld_incomplete_type_of by > implementing my own variant of build_pointer_type that also matches > TYPE_CANONICAL of the pointer it creates. I wonder if there are better > solutions? > > Honza > > Index: tree.c > === > --- tree.c(revision 265807) > +++ tree.c(working copy) > @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st >TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t); >TYPE_NAME (v) = TYPE_NAME (t); >TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t); > + TYPE_CANONICAL (v) = TYPE_CANONICAL (t); >add_tree_to_fld_list (v, fld); >return v; > } > @@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f > else > first = build_reference_type_for_mode (t2, TYPE_MODE (t), > TYPE_REF_CAN_ALIAS_ALL (t)); > + gcc_assert (TYPE_CANONICAL (t2) != t2 > + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)) > + && TYPE_CANONICAL (first) > + == TYPE_CANONICAL (TYPE_MAIN_VARIANT (t))); > add_tree_to_fld_list (first, fld); > return fld_type_variant (first, t, fld); > } > @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f > SET_TYPE_MODE (copy, VOIDmode); > SET_TYPE_ALIGN (copy, BITS_PER_UNIT); > TYPE_SIZE_UNIT (copy) = NULL; > + TYPE_CANONICAL (copy) = TYPE_CANONICAL (t); > if (AGGREGATE_TYPE_P (t)) > { > TYPE_FIELDS (copy) = NULL;
Re: Fix SPEC gcc micompile with LTO
On November 5, 2018 5:11:09 PM GMT+01:00, Jan Hubicka wrote: >> Hi, >> this is patch I ended up testing. It ensures that canonical types of >> copies I create are same as of originals C++ FE has its own refernece >piece of mail got lost rendering the paragraph unreadable. I wanted to >say: > >This is patch I ended up testing. It ensures that canonical types of >copies I create are same as of originals. It however ICEs building >auto-profile.c because C++ FE has its own reference type construction >(cp_build_reference_type) and it creates additional pointer types with >TYPE_REF_IS_RVALUE set and it has different TYPE_CANONICAL. Hmm. I guess we need to fix that, otherwise alias will be broken (or you need to resort to a FE specific routine for pointer building). Richard. >> Obviously we do not see this in middle-end and we end up merging the >> types despite fact they have different TYPE_CANONICAL. >> I guess I can immitate the behaviour in fld_incomplete_type_of by >> implementing my own variant of build_pointer_type that also matches >> TYPE_CANONICAL of the pointer it creates. I wonder if there are >better >> solutions? >> >> Honza >> >> Index: tree.c >> === >> --- tree.c (revision 265807) >> +++ tree.c (working copy) >> @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st >>TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t); >>TYPE_NAME (v) = TYPE_NAME (t); >>TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t); >> + TYPE_CANONICAL (v) = TYPE_CANONICAL (t); >>add_tree_to_fld_list (v, fld); >>return v; >> } >> @@ -5146,6 +5147,10 @@ fld_incomplete_type_of (tree t, struct f >>else >> first = build_reference_type_for_mode (t2, TYPE_MODE (t), >> TYPE_REF_CAN_ALIAS_ALL (t)); >> + gcc_assert (TYPE_CANONICAL (t2) != t2 >> + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)) >> + && TYPE_CANONICAL (first) >> + == TYPE_CANONICAL (TYPE_MAIN_VARIANT (t))); >>add_tree_to_fld_list (first, fld); >>return fld_type_variant (first, t, fld); >> } >> @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f >>SET_TYPE_MODE (copy, VOIDmode); >>SET_TYPE_ALIGN (copy, BITS_PER_UNIT); >>TYPE_SIZE_UNIT (copy) = NULL; >> + TYPE_CANONICAL (copy) = TYPE_CANONICAL (t); >>if (AGGREGATE_TYPE_P (t)) >> { >>TYPE_FIELDS (copy) = NULL;
Re: Fix SPEC gcc micompile with LTO
Hi, this is updated version of the patch. As discussed on IRC, C++ has two different types of references (rvalue and normal). They have different canonical type, but this does not affect outcome of get_alias_set because it rebuilds the pointer/reference types from scratch. The effect of this patch is to turn both into one type when pointer is reuilt that should be safe. So I simply relaxed the sanity check that type canonicals needs to be same for pointers. lto-bootstrapped/regtested x86_64-linux, plan to commit it shortly. Honza * gcc.dg/lto/tbaa-1.c: New testcase. * tree.c (fld_type_variant): Copy canonical type. (fld_incomplete_type_of): Check that canonical types looks sane; copy canonical type. (verify_type): Accept when incomplete type has complete canonical type. Index: testsuite/gcc.dg/lto/tbaa-1.c === --- testsuite/gcc.dg/lto/tbaa-1.c (nonexistent) +++ testsuite/gcc.dg/lto/tbaa-1.c (working copy) @@ -0,0 +1,41 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -flto -fdump-tree-evrp" } */ +typedef struct rtx_def *rtx; +typedef struct cselib_val_struct +{ + union + { + } u; + struct elt_loc_list *locs; +} +cselib_val; +struct elt_loc_list +{ + struct elt_loc_list *next; + rtx loc; +}; +static int n_useless_values; +unchain_one_elt_loc_list (pl) + struct elt_loc_list **pl; +{ + struct elt_loc_list *l = *pl; + *pl = l->next; +} + +discard_useless_locs (x, info) + void **x; +{ + cselib_val *v = (cselib_val *) * x; + struct elt_loc_list **p = &v->locs; + int had_locs = v->locs != 0; + while (*p) +{ + unchain_one_elt_loc_list (p); + p = &(*p)->next; +} + if (had_locs && v->locs == 0) +{ + n_useless_values++; +} +} +/* { dg-final { scan-tree-dump-times "n_useless_values" 2 "evrp" } } */ Index: tree.c === --- tree.c (revision 265807) +++ tree.c (working copy) @@ -5118,6 +5118,7 @@ fld_type_variant (tree first, tree t, st TYPE_ADDR_SPACE (v) = TYPE_ADDR_SPACE (t); TYPE_NAME (v) = TYPE_NAME (t); TYPE_ATTRIBUTES (v) = TYPE_ATTRIBUTES (t); + TYPE_CANONICAL (v) = TYPE_CANONICAL (t); add_tree_to_fld_list (v, fld); return v; } @@ -5146,6 +5147,8 @@ fld_incomplete_type_of (tree t, struct f else first = build_reference_type_for_mode (t2, TYPE_MODE (t), TYPE_REF_CAN_ALIAS_ALL (t)); + gcc_assert (TYPE_CANONICAL (t2) != t2 + && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); } @@ -5169,6 +5174,7 @@ fld_incomplete_type_of (tree t, struct f SET_TYPE_MODE (copy, VOIDmode); SET_TYPE_ALIGN (copy, BITS_PER_UNIT); TYPE_SIZE_UNIT (copy) = NULL; + TYPE_CANONICAL (copy) = TYPE_CANONICAL (t); if (AGGREGATE_TYPE_P (t)) { TYPE_FIELDS (copy) = NULL; @@ -13880,7 +13893,8 @@ verify_type (const_tree t) with variably sized arrays because their sizes possibly gimplified to different variables. */ && !variably_modified_type_p (ct, NULL) - && !gimple_canonical_types_compatible_p (t, ct, false)) + && !gimple_canonical_types_compatible_p (t, ct, false) + && COMPLETE_TYPE_P (t)) { error ("TYPE_CANONICAL is not compatible"); debug_tree (ct);