| Message ID | 20130510171251.GG3568@virgil.suse |
|---|---|
| State | New |
| Headers | show |
> 2013-05-10 Martin Jambor <mjambor@suse.cz> > > * tree.c (get_binfo_at_offset): Use types_compatible_p to compare > types. > > Index: src/gcc/tree.c > =================================================================== > --- src.orig/gcc/tree.c > +++ src/gcc/tree.c > @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI > tree fld; > int i; > > - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (expected_type)) > + if (types_compatible_p (type, expected_type)) As discussed on lunch, it seems fine to me, but I am not an expert ;) I wonder about the following test few lines bellow: /* Offset 0 indicates the primary base, whose vtable contents are represented in the binfo for the derived class. */ else if (offset != 0) { tree base_binfo, found_binfo = NULL_TREE; for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) if (TREE_TYPE (base_binfo) == TREE_TYPE (fld)) don't we want also more relaxed testing? "grep "TYPE.* ==.*TYPE " *.c" seems to show some extra dubious cases i.e. in fold-const, tree-ssa-coaelesce and other places where we most probably care only about semantical equivalence of the type... Thanks for looking into this! Honza
Hi, On Fri, May 10, 2013 at 07:24:06PM +0200, Jan Hubicka wrote: > > 2013-05-10 Martin Jambor <mjambor@suse.cz> > > > > * tree.c (get_binfo_at_offset): Use types_compatible_p to compare > > types. > > > > Index: src/gcc/tree.c > > =================================================================== > > --- src.orig/gcc/tree.c > > +++ src/gcc/tree.c > > @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI > > tree fld; > > int i; > > > > - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (expected_type)) > > + if (types_compatible_p (type, expected_type)) > As discussed on lunch, it seems fine to me, but I am not an expert ;) I wonder about > the following test few lines bellow: > /* Offset 0 indicates the primary base, whose vtable contents are > represented in the binfo for the derived class. */ > else if (offset != 0) > { > tree base_binfo, found_binfo = NULL_TREE; > for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) > if (TREE_TYPE (base_binfo) == TREE_TYPE (fld)) > > don't we want also more relaxed testing? we most probably do, good catch. I'll update the patch, re-test and commit it early next week unless someone objects. > > "grep "TYPE.* ==.*TYPE " *.c" seems to show some extra dubious cases i.e. > in fold-const, tree-ssa-coaelesce and other places where we most probably care > only about semantical equivalence of the type... > Yeah, examinig those is also probably a good idea. Thanks, Martin
Martin Jambor <mjambor@suse.cz> wrote: >Hi, > >On Fri, May 10, 2013 at 07:24:06PM +0200, Jan Hubicka wrote: >> > 2013-05-10 Martin Jambor <mjambor@suse.cz> >> > >> > * tree.c (get_binfo_at_offset): Use types_compatible_p to compare >> > types. >> > >> > Index: src/gcc/tree.c >> > =================================================================== >> > --- src.orig/gcc/tree.c >> > +++ src/gcc/tree.c >> > @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI >> > tree fld; >> > int i; >> > >> > - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT >(expected_type)) >> > + if (types_compatible_p (type, expected_type)) >> As discussed on lunch, it seems fine to me, but I am not an expert ;) This doesn't look good to me. With lto all types that are structurally equivalent will compare compatible this way. Probably not what you want. Richard. > I wonder about >> the following test few lines bellow: >> /* Offset 0 indicates the primary base, whose vtable contents >are >> represented in the binfo for the derived class. */ >> else if (offset != 0) >> { >> tree base_binfo, found_binfo = NULL_TREE; >> for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) >> if (TREE_TYPE (base_binfo) == TREE_TYPE (fld)) >> >> don't we want also more relaxed testing? > >we most probably do, good catch. I'll update the patch, re-test and >commit it early next week unless someone objects. > >> >> "grep "TYPE.* ==.*TYPE " *.c" seems to show some extra dubious cases >i.e. >> in fold-const, tree-ssa-coaelesce and other places where we most >probably care >> only about semantical equivalence of the type... >> > >Yeah, examinig those is also probably a good idea. > >Thanks, > >Martin
> Martin Jambor <mjambor@suse.cz> wrote: > > >Hi, > > > >On Fri, May 10, 2013 at 07:24:06PM +0200, Jan Hubicka wrote: > >> > 2013-05-10 Martin Jambor <mjambor@suse.cz> > >> > > >> > * tree.c (get_binfo_at_offset): Use types_compatible_p to compare > >> > types. > >> > > >> > Index: src/gcc/tree.c > >> > =================================================================== > >> > --- src.orig/gcc/tree.c > >> > +++ src/gcc/tree.c > >> > @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI > >> > tree fld; > >> > int i; > >> > > >> > - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT > >(expected_type)) > >> > + if (types_compatible_p (type, expected_type)) > >> As discussed on lunch, it seems fine to me, but I am not an expert ;) > > This doesn't look good to me. With lto all types that are structurally equivalent will compare compatible this way. Probably not what you want. Yep, that was my concern, too. I wonder how to recognize the two types represent the same object. Perhaps by adding an assiciated vtable (via binfos) into the structural match while merging? Honza
Jan Hubicka <hubicka@ucw.cz> wrote: >> Martin Jambor <mjambor@suse.cz> wrote: >> >> >Hi, >> > >> >On Fri, May 10, 2013 at 07:24:06PM +0200, Jan Hubicka wrote: >> >> > 2013-05-10 Martin Jambor <mjambor@suse.cz> >> >> > >> >> > * tree.c (get_binfo_at_offset): Use types_compatible_p to >compare >> >> > types. >> >> > >> >> > Index: src/gcc/tree.c >> >> > >=================================================================== >> >> > --- src.orig/gcc/tree.c >> >> > +++ src/gcc/tree.c >> >> > @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI >> >> > tree fld; >> >> > int i; >> >> > >> >> > - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT >> >(expected_type)) >> >> > + if (types_compatible_p (type, expected_type)) >> >> As discussed on lunch, it seems fine to me, but I am not an expert >;) >> >> This doesn't look good to me. With lto all types that are >structurally equivalent will compare compatible this way. Probably not >what you want. > >Yep, that was my concern, too. I wonder how to recognize the two types >represent the same object. Perhaps by adding an assiciated vtable (via >binfos) >into the structural match while merging? The existing check should work ok with lto. If not then we should figure out why we do not merge the main variants properly. Richard. >Honza
Index: src/gcc/tree.c =================================================================== --- src.orig/gcc/tree.c +++ src/gcc/tree.c @@ -11483,7 +11483,7 @@ get_binfo_at_offset (tree binfo, HOST_WI tree fld; int i; - if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (expected_type)) + if (types_compatible_p (type, expected_type)) return binfo; if (offset < 0) return NULL_TREE;