Patchwork Use types_compatible_p in get_binfo_at_offset

login
register
mail settings
Submitter Martin Jambor
Date May 10, 2013, 5:12 p.m.
Message ID <20130510171251.GG3568@virgil.suse>
Download mbox | patch
Permalink /patch/243041/
State New
Headers show

Comments

Martin Jambor - May 10, 2013, 5:12 p.m.
Hi,

PR 57084 has alerted me that IN LTO we do not devirtualize calls
during ipa-cp and ipa-inline that we do in non-LTO compilations.  The
reason is that the type comparison in get_binfo_at_offset is failing,
because the two types do not have the same TYPE_MAIN_VARIANT but
TYPE_CANONICAL should be checked instead.  Or better yet, call
types_compatible_p which hopefully will also know what to do in these
cases, which is what the patch below does.

The change increases the number of type-based devirtualizations in
ipa-cp and ipa-inline from 137 to 179 in 483.xalancbmk when compiles
with -O2 -flto -m32 (-m32 is not significant here, it was only
required to trigger the bug and I made the measurements in the same
environment).

Bootstrapped and tested on x86_64-linux without any issues.  OK for
trunk?

Thanks,

Martin


2013-05-10  Martin Jambor  <mjambor@suse.cz>

	* tree.c (get_binfo_at_offset): Use types_compatible_p to compare
	types.
Jan Hubicka - May 10, 2013, 5:24 p.m.
> 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
Martin Jambor - May 10, 2013, 5:40 p.m.
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
Richard Guenther - May 10, 2013, 6:34 p.m.
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
Jan Hubicka - May 10, 2013, 7:01 p.m.
> 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
Richard Guenther - May 10, 2013, 7:54 p.m.
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

Patch

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;