[PR/82546] tree node size

[Although I filed this as a middle-end bug, it's really a core infra 
bug, not sure who the best reviewer is]

In working on tree streaming in the modules branch, I discovered poor 
tree node size and hierarchy bits.

Here's a fix for the first part of that.  tree.c (tree_code_size) 
returns sizeof (tree_type_non_common) for any tcc_type node. That's 
wasteful, given we have tree_type_common-> 
tree_type_with_lang_specific-> tree_type_non_common available as 
choices.  It's also obscuring defects in (at least) the c++ FE where we 
use tree_type_non_common fields, but claim the node doesn't contain that 
structure.   Ew.

This patch makes tree_code_size ask the lang hook for the size of 
non-core type nodes.  It also fixes the c++ and objc FEs to return a 
size for the nodes it cares about.

I don't (yet) know whether all the core types are tree_type_non_common, 
nor whether the FE's can return smaller sizes than the do with this 
patch.  But at least the control flow is now correct.  during developing 
this patch I added an assert that the lang hook was returning a size at 
least as big as tree_type_non_common, so they couldn't be more broken 
than before the patch.

I intend to continue cleaning this up of course.  It's not clear to me 
whether we should cache these node sizes in an array, and the way it 
goes about checking nodes with nested switches is understandable, but 
possible not the fastest solution. However let's at least get the sizing 
right first.

ok?

nathan

On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote:
>[Although I filed this as a middle-end bug, it's really a core infra 
>bug, not sure who the best reviewer is]
>
>In working on tree streaming in the modules branch, I discovered poor 
>tree node size and hierarchy bits.
>
>Here's a fix for the first part of that.  tree.c (tree_code_size) 
>returns sizeof (tree_type_non_common) for any tcc_type node. That's 
>wasteful, given we have tree_type_common-> 
>tree_type_with_lang_specific-> tree_type_non_common available as 
>choices.  It's also obscuring defects in (at least) the c++ FE where we
>
>use tree_type_non_common fields, but claim the node doesn't contain
>that 
>structure.   Ew.
>
>This patch makes tree_code_size ask the lang hook for the size of 
>non-core type nodes.  It also fixes the c++ and objc FEs to return a 
>size for the nodes it cares about.
>
>I don't (yet) know whether all the core types are tree_type_non_common,
>
>nor whether the FE's can return smaller sizes than the do with this 
>patch.  But at least the control flow is now correct.  during
>developing 
>this patch I added an assert that the lang hook was returning a size at
>
>least as big as tree_type_non_common, so they couldn't be more broken 
>than before the patch.
>
>I intend to continue cleaning this up of course.  It's not clear to me 
>whether we should cache these node sizes in an array, and the way it 
>goes about checking nodes with nested switches is understandable, but 
>possible not the fastest solution. However let's at least get the
>sizing 
>right first.

We were conservative exactly to avoid the langhook here. I think there's similar 'bug' on the decl side. 

Richard. 

>ok?
>
>nathan

On 10/16/2017 02:49 AM, Richard Biener wrote:
> On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org> wrote:

>> I intend to continue cleaning this up of course.  It's not clear to me
>> whether we should cache these node sizes in an array, and the way it
>> goes about checking nodes with nested switches is understandable, but
>> possible not the fastest solution. However let's at least get the
>> sizing
>> right first.
> 
> We were conservative exactly to avoid the langhook here. I think there's similar 'bug' on the decl side.

The other code types (decls, exprs, etc) call the langhook.  tcc_type 
seems the exception (now?).

nathan

On Mon, 16 Oct 2017, Nathan Sidwell wrote:

> On 10/16/2017 02:49 AM, Richard Biener wrote:
> > On October 13, 2017 8:29:40 PM GMT+02:00, Nathan Sidwell <nathan@acm.org>
> > wrote:
> 
> > > I intend to continue cleaning this up of course.  It's not clear to me
> > > whether we should cache these node sizes in an array, and the way it
> > > goes about checking nodes with nested switches is understandable, but
> > > possible not the fastest solution. However let's at least get the
> > > sizing
> > > right first.
> > 
> > We were conservative exactly to avoid the langhook here. I think there's
> > similar 'bug' on the decl side.
> 
> The other code types (decls, exprs, etc) call the langhook.  tcc_type seems
> the exception (now?).

Sorry for not looking at the patch before replying.  The patch looks ok
but shouldn't LANG_TYPE be also handled by the FE?  LANG_TYPE itself
is an odd beast if I may say that - it's only used by the C++ and Ada FEs
and the Ada FE does only

/* Make a dummy type corresponding to GNAT_TYPE.  */

tree
make_dummy_type (Entity_Id gnat_type)
{
...
  /* Create a debug type so that debug info consumers only see an 
unspecified
     type.  */
  if (Needs_Debug_Info (gnat_type))
    {
      debug_type = make_node (LANG_TYPE);
      SET_TYPE_DEBUG_TYPE (gnu_type, debug_type);

      TYPE_NAME (debug_type) = TYPE_NAME (gnu_type);
      TYPE_ARTIFICIAL (debug_type) = TYPE_ARTIFICIAL (gnu_type);
    }


Thus the patch is ok.

Thanks,
Richard.

On 10/17/2017 05:26 AM, Richard Biener wrote:

> Sorry for not looking at the patch before replying.  The patch looks ok
> but shouldn't LANG_TYPE be also handled by the FE?  LANG_TYPE itself
> is an odd beast if I may say that - it's only used by the C++ and Ada FEs
> and the Ada FE does only

I agree.  I think LANG_TYPE may be from when there were no FE-specific 
nodes.  It should probably be killed and resurrected as appropriate FE 
nodes.

Olivier, as an ADA person, is that something that could be done?

> Thus the patch is ok.

Thanks,

As a heads up, we currently have:

struct type_common;
struct type_with_lang_specific : type_common;
struct type_non_common : type_with_lang_specific;

And many (most?, all?) FE type nodes derive from type_non_common (even 
if, as I discovered, they don't know it).  It seems the hierarchy would 
be better as:

struct type_common;
struct type_non_common : type_common; // FE type derive here
struct type_with_lang_specific : type_non_common;

After all, why would a FE-specific type need a lang-specific pointer?  I 
don't think there are types that just have the land-pointer and don't 
have non_common.

That's the direction I'm heading in with this clean up.

nathan

On Tue, 17 Oct 2017, Nathan Sidwell wrote:

> On 10/17/2017 05:26 AM, Richard Biener wrote:
> 
> > Sorry for not looking at the patch before replying.  The patch looks ok
> > but shouldn't LANG_TYPE be also handled by the FE?  LANG_TYPE itself
> > is an odd beast if I may say that - it's only used by the C++ and Ada FEs
> > and the Ada FE does only
> 
> I agree.  I think LANG_TYPE may be from when there were no FE-specific nodes.
> It should probably be killed and resurrected as appropriate FE nodes.
> 
> Olivier, as an ADA person, is that something that could be done?
> 
> > Thus the patch is ok.
> 
> Thanks,
> 
> As a heads up, we currently have:
> 
> struct type_common;
> struct type_with_lang_specific : type_common;
> struct type_non_common : type_with_lang_specific;
> 
> And many (most?, all?) FE type nodes derive from type_non_common (even if, as
> I discovered, they don't know it).  It seems the hierarchy would be better as:
> 
> struct type_common;
> struct type_non_common : type_common; // FE type derive here
> struct type_with_lang_specific : type_non_common;
> 
> After all, why would a FE-specific type need a lang-specific pointer?  I don't
> think there are types that just have the land-pointer and don't have
> non_common.
> 
> That's the direction I'm heading in with this clean up.

Not sure.  For decls the lang_specific pointer is even in decl_common!

It's probably that there are basically no types w/o a FE using the
lang-specific pointer but there are some types using
type_common fields only (but need lang-specifics).

So maybe do like with decls and remove type_with_lang_specific, instead
folding it into type_common?

Richard.

> On Oct 17, 2017, at 16:10 , Nathan Sidwell <nathan@acm.org> wrote:
> 
> On 10/17/2017 05:26 AM, Richard Biener wrote:
> 
>> Sorry for not looking at the patch before replying.  The patch looks ok
>> but shouldn't LANG_TYPE be also handled by the FE?  LANG_TYPE itself
>> is an odd beast if I may say that - it's only used by the C++ and Ada FEs
>> and the Ada FE does only
> 
> I agree.  I think LANG_TYPE may be from when there were no FE-specific nodes.  It should probably be killed and resurrected as appropriate FE nodes.
> 
> Olivier, as an ADA person, is that something that could be done?

I'd think so. LANG_TYPE is treated specially in several
places and Ada debug types are pretty sensitive so this would
require caution but I don't see/know-of obvious reasons why this
couldn't be done.

Eric and Pierre-Marie (cc'ed) might have more precise insights.

Olivier

> I'd think so. LANG_TYPE is treated specially in several
> places and Ada debug types are pretty sensitive so this would
> require caution but I don't see/know-of obvious reasons why this
> couldn't be done.

LANG_TYPE is only used in Ada to trigger the specific treatment in 
gen_type_die_with_usage for DW_TAG_unspecified_type, so very minor.

> On 18 Oct 2017, at 15:59, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> I'd think so. LANG_TYPE is treated specially in several
>> places and Ada debug types are pretty sensitive so this would
>> require caution but I don't see/know-of obvious reasons why this
>> couldn't be done.
> 
> LANG_TYPE is only used in Ada to trigger the specific treatment in 
> gen_type_die_with_usage for DW_TAG_unspecified_type, so very minor.

OK, thanks for confirming Eric!