Patchwork Use types_compatible_p in get_binfo_at_offset

login
register
mail settings
Submitter Jan Hubicka
Date May 11, 2013, 12:16 a.m.
Message ID <20130511001601.GB29273@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/243101/
State New
Headers show

Comments

Jan Hubicka - May 11, 2013, 12:16 a.m.
> 
> The existing check should work ok with lto. If not then we should figure out why we do not merge the main variants properly.
Hmm, adding:

seems to bring a lot of positives and by quick inspection most of them seem
like same classes.  I will try to find some time to debug this more. But
glancing over the the dumps, I see many of them just have different name
spaces.  Do we even attempt to merge namespace_decl? How types from same
namespaces in different units are supposed to match?

Honza
> 
> Richard.
> 
> >Honza
>
Jan Hubicka - May 11, 2013, 12:19 a.m.
> seems to bring a lot of positives and by quick inspection most of them seem
> like same classes.  I will try to find some time to debug this more. But
> glancing over the the dumps, I see many of them just have different name
> spaces.  Do we even attempt to merge namespace_decl? How types from same
> namespaces in different units are supposed to match?
Just as example
 <record_type 0x7ff129ff6bd0 X86Assembler addressable needs-constructing BLK
    size <integer_cst 0x7ff136ee2760 type <integer_type 0x7ff1370af0a8 bitsizetype> constant 2368>
    unit size <integer_cst 0x7ff136ee2780 type <integer_type 0x7ff1370af000 sizetype> constant 296>
    align 64 symtab 0 alias set -1 canonical type 0x7ff136efc498
    fields <field_decl 0x7ff129fe2850 D.752229
        type <record_type 0x7ff129ff6150 GenericAssembler needs-constructing TI
            size <integer_cst 0x7ff13709eda0 constant 128>
            unit size <integer_cst 0x7ff13709edc0 constant 16>
            align 64 symtab 0 alias set -1 canonical type 0x7ff136ec6b28 fields <field_decl 0x7ff129fe2098 printer> context <namespace_decl 0x7ff129e45e60 JSC>
            pointer_to_this <pointer_type 0x7ff129c7f888> chain <type_decl 0x7ff129fd7e60 GenericAssembler>>
        ignored TI file /abuild/jh/mozilla-central2/mozilla-central/js/src/assembler/assembler/X86Assembler.h line 140 col 7
        size <integer_cst 0x7ff137227400 constant 72>
        unit size <integer_cst 0x7ff136eb50a0 constant 9>
        align 64 offset_align 128
        offset <integer_cst 0x7ff13709ed60 constant 0>
        bit offset <integer_cst 0x7ff13709ede0 constant 0> context <record_type 0x7ff129ff6bd0 X86Assembler>
        chain <field_decl 0x7ff129fe28e8 m_formatter type <record_type 0x7ff129ff6b28 X86InstructionFormatter>
            used private nonlocal BLK file /abuild/jh/mozilla-central2/mozilla-central/js/src/assembler/assembler/X86Assembler.h line 3207 col 7
            size <integer_cst 0x7ff136e64d00 constant 2240>
            unit size <integer_cst 0x7ff136e64d20 constant 280>
            align 64 offset_align 128 offset <integer_cst 0x7ff13709edc0 16> bit offset <integer_cst 0x7ff13709ede0 0> context <record_type 0x7ff129ff6bd0 X86Assembler> chain <type_decl 0x7ff129ff75c0 X86Assembler>>> context <namespace_decl 0x7ff129e45e60 JSC>
    pointer_to_this <pointer_type 0x7ff129c7fa80> chain <type_decl 0x7ff129ff7508 X86Assembler>>
 <record_type 0x7ff12c9e7690 X86Assembler addressable needs-constructing BLK
    size <integer_cst 0x7ff136ee2760 type <integer_type 0x7ff1370af0a8 bitsizetype> constant 2368>
    unit size <integer_cst 0x7ff136ee2780 type <integer_type 0x7ff1370af000 sizetype> constant 296>
    align 64 symtab 0 alias set -1 canonical type 0x7ff136efc498
    fields <field_decl 0x7ff12db56000 D.789661
        type <record_type 0x7ff12e6b4000 GenericAssembler needs-constructing TI
            size <integer_cst 0x7ff13709eda0 constant 128>
            unit size <integer_cst 0x7ff13709edc0 constant 16>
            align 64 symtab 0 alias set -1 canonical type 0x7ff136ec6b28 fields <field_decl 0x7ff12c72e8e8 printer> context <namespace_decl 0x7ff1315587e8 JSC>
            pointer_to_this <pointer_type 0x7ff12c6cb738> chain <type_decl 0x7ff13155f228 GenericAssembler>>
        ignored TI file /abuild/jh/mozilla-central2/mozilla-central/js/src/assembler/assembler/X86Assembler.h line 140 col 0
        size <integer_cst 0x7ff137227400 constant 72>
        unit size <integer_cst 0x7ff136eb50a0 constant 9>
        align 64 offset_align 128
        offset <integer_cst 0x7ff13709ed60 constant 0>
        bit offset <integer_cst 0x7ff13709ede0 constant 0> context <record_type 0x7ff12c9e7690 X86Assembler>
        chain <field_decl 0x7ff12db56098 m_formatter type <record_type 0x7ff12c9e75e8 X86InstructionFormatter>
            used private nonlocal BLK file /abuild/jh/mozilla-central2/mozilla-central/js/src/assembler/assembler/X86Assembler.h line 3207 col 0
            size <integer_cst 0x7ff136e64d00 constant 2240>
            unit size <integer_cst 0x7ff136e64d20 constant 280>
            align 64 offset_align 128 offset <integer_cst 0x7ff13709edc0 16> bit offset <integer_cst 0x7ff13709ede0 0> context <record_type 0x7ff12c9e7690 X86Assembler> chain <type_decl 0x7ff12c673678 X86Assembler>>> context <namespace_decl 0x7ff1315587e8 JSC>
    pointer_to_this <pointer_type 0x7ff12c6cbf18> chain <type_decl 0x7ff12c6732e0 X86Assembler>>

Honza
> 
> Honza
> > 
> > Richard.
> > 
> > >Honza
> >
Richard Guenther - May 11, 2013, 8:40 a.m.
Jan Hubicka <hubicka@ucw.cz> wrote:

>> 
>> The existing check should work ok with lto. If not then we should
>figure out why we do not merge the main variants properly.
>Hmm, adding:
>Index: tree.c
>===================================================================
>--- tree.c      (revision 198796)
>+++ tree.c      (working copy)
>@@ -11572,6 +11572,12 @@ get_binfo_at_offset (tree binfo, HOST_WI
> 
>     if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (expected_type))
>          return binfo;
>+      else
>+       if (types_compatible_p (type, expected_type))
>+         {
>+           debug_tree (TYPE_MAIN_VARIANT (type));
>+           debug_tree (TYPE_MAIN_VARIANT (expected_type));
>+         }
>       if (offset < 0)
>        return NULL_TREE;
> 
>@@ -11605,6 +11611,12 @@ get_binfo_at_offset (tree binfo, HOST_WI
>                found_binfo = base_binfo;
>                break;
>              }
>+             else
>+               if (types_compatible_p (TREE_TYPE (base_binfo),
>TREE_TYPE (fld)))
>+                 {
>+                   debug_tree (TREE_TYPE (base_binfo));
>+                   debug_tree (TREE_TYPE (fld));
>+                 }
>          if (!found_binfo)
>            return NULL_TREE;
>          binfo = found_binfo;
>
>seems to bring a lot of positives and by quick inspection most of them
>seem
>like same classes.  I will try to find some time to debug this more.
>But
>glancing over the the dumps, I see many of them just have different
>name
>spaces.  Do we even attempt to merge namespace_decl? How types from
>same
>namespaces in different units are supposed to match?
>
We do not merge namespace decls, which is likely the issue here. My in-progress tree merging should eventually fix this...

Richard.

>Honza
>> 
>> Richard.
>> 
>> >Honza
>>
Jan Hubicka - May 11, 2013, 11:39 a.m.
> >But
> >glancing over the the dumps, I see many of them just have different
> >name
> >spaces.  Do we even attempt to merge namespace_decl? How types from
> >same
> >namespaces in different units are supposed to match?
> >
> We do not merge namespace decls, which is likely the issue here. My in-progress tree merging should eventually fix this...

Yep, I think namespaces should to be merged. They are however (always?)
refereed by TYPE_CONTEXT and I do not see that to be part of type merging
rules. So I do not think it prevents us from merging types from different
namespace decls.

On the other hand, I alwas saw a lot of duplicates of the same structure/class
getting unmerged even without namespaces.  I am not sure how much of those are
just bugs and how much we keep for valid reasons (such as slightly different
debug info attached to them because of different #include order or so)

I uploaded the list of false negatives when compiling Mozilla's JS interpretter
to http://atrey.karlin.mff.cuni.cz/~hubicka/binfo.txt
I suppose any bigger C++ project will give such a list and it looks like good
list of cases to analyze.

Can we resonably expect those to be all merged?  Perhaps C++ one decl rule
allows us to simply match names+namespaces (after merging) here?

Honza

> 
> Richard.
> 
> >Honza
> >> 
> >> Richard.
> >> 
> >> >Honza
> >> 
>
Richard Guenther - May 13, 2013, 9:01 a.m.
On Sat, May 11, 2013 at 1:39 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >But
>> >glancing over the the dumps, I see many of them just have different
>> >name
>> >spaces.  Do we even attempt to merge namespace_decl? How types from
>> >same
>> >namespaces in different units are supposed to match?
>> >
>> We do not merge namespace decls, which is likely the issue here. My in-progress tree merging should eventually fix this...
>
> Yep, I think namespaces should to be merged. They are however (always?)
> refereed by TYPE_CONTEXT and I do not see that to be part of type merging
> rules. So I do not think it prevents us from merging types from different
> namespace decls.

Note that only TYPE_DECLs are siblings of NAMESPACE_DECLs and we
do stream DECL_CONTEXT (and also factor in that at least for types).  So
in the end we should only merge same types from the same namespace
which means we _should_ factor in the types overall context, even if we
fail to do that now.

> On the other hand, I alwas saw a lot of duplicates of the same structure/class
> getting unmerged even without namespaces.  I am not sure how much of those are
> just bugs and how much we keep for valid reasons (such as slightly different
> debug info attached to them because of different #include order or so)

Bugs are certainly possible ;)

> I uploaded the list of false negatives when compiling Mozilla's JS interpretter
> to http://atrey.karlin.mff.cuni.cz/~hubicka/binfo.txt
> I suppose any bigger C++ project will give such a list and it looks like good
> list of cases to analyze.
>
> Can we resonably expect those to be all merged?  Perhaps C++ one decl rule
> allows us to simply match names+namespaces (after merging) here?

But we cannot assume C++ rules for types.

Richard.

> Honza
>
>>
>> Richard.
>>
>> >Honza
>> >>
>> >> Richard.
>> >>
>> >> >Honza
>> >>
>>

Patch

Index: tree.c
===================================================================
--- tree.c      (revision 198796)
+++ tree.c      (working copy)
@@ -11572,6 +11572,12 @@  get_binfo_at_offset (tree binfo, HOST_WI
 
       if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (expected_type))
          return binfo;
+      else
+       if (types_compatible_p (type, expected_type))
+         {
+           debug_tree (TYPE_MAIN_VARIANT (type));
+           debug_tree (TYPE_MAIN_VARIANT (expected_type));
+         }
       if (offset < 0)
        return NULL_TREE;
 
@@ -11605,6 +11611,12 @@  get_binfo_at_offset (tree binfo, HOST_WI
                found_binfo = base_binfo;
                break;
              }
+             else
+               if (types_compatible_p (TREE_TYPE (base_binfo), TREE_TYPE (fld)))
+                 {
+                   debug_tree (TREE_TYPE (base_binfo));
+                   debug_tree (TREE_TYPE (fld));
+                 }
          if (!found_binfo)
            return NULL_TREE;
          binfo = found_binfo;