Re: [PATCH v2 1/6] Convert symtab, cgraph and varpool nodes into a real class hierarchy

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2013-09-10 at 15:34 +0200, Jan Hubicka wrote:

Thanks for reviewing this, and sorry for the late response (I lost most
of last week to illness).  Some questions inline below...

> > This patch is the handwritten part of the conversion of these types
> > to C++; it requires the followup patch, which is autogenerated.
> > 
> > It converts:
> >   struct GTY(()) symtab_node_base
> > to:
> >   class GTY((user)) symtab_node_base
> > 
> > and converts:
> >   struct GTY(()) cgraph_node
> > to:
> >   struct GTY((user)) cgraph_node : public symtab_node_base
> > 
> > and:
> >   struct GTY(()) varpool_node
> > to:
> >   class GTY((user)) varpool_node : public symtab_node_base
> > 
> > dropping the symtab_node_def union.
> > 
> > Since gengtype is unable to cope with inheritance, we have to mark the
> > types with GTY((user)), and handcode the gty field-visiting functions.
> > Given the simple hierarchy, we don't need virtual functions for this.
> > 
> > Unfortunately doing so runs into various bugs in gengtype's handling
> > of GTY((user)), so the patch also includes workarounds for these bugs.
> > 
> > gengtype walks the graph of the *types* in the code, and produces
> > functions in gtype-desc.[ch] for all types that are reachable from a
> > GTY root.
> > 
> > However, it ignores the contents of GTY((user)) types when walking
> > this graph.
> > 
> > Hence if you have a subgraph of types that are only reachable
> > via fields in GTY((user)) types, gengtype won't generate helper code
> > for those types.
> > 
> > Ideally there would be some way to mark a GTY((user)) type to say
> > which types it references, to avoid having to mark these types as
> > GTY((user)).
> > 
> > For now, work around this issue by providing explicit roots of the
> > missing types, of dummy variables (see the bottom of cgraph.c)
> > 
[..]

> > diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> > index f12bf1b..4b12163 100644
> > --- a/gcc/cgraph.c
> > +++ b/gcc/cgraph.c
> > @@ -2994,4 +2994,222 @@ cgraph_get_body (struct cgraph_node *node)
> >    return true;
> >  }
> >  
> > +/* GTY((user)) hooks for symtab_node_base (and its subclasses).
> > +   We could use virtual functions for this, but given the presence of the
> > +   "type" field and the trivial size of the class hierarchy, switches are
> > +   perhaps simpler and faster.  */
> > +
> > +void gt_ggc_mx (symtab_node_base *x)
> > +{
> > +  /* Hand-written equivalent of the chain_next/chain_prev machinery, to
> > +     avoid deep call-stacks.
> > +
> > +     Locate the neighbors of x (within the linked-list) that haven't been
> > +     marked yet, so that x through xlimit give a range suitable for marking.
> > +     Note that x (on entry) itself has already been marked by the
> > +     gtype-desc.c code, so we first try its successor.
> > +  */
> > +  symtab_node_base * xlimit = x ? x->next : NULL;
> > +  while (ggc_test_and_set_mark (xlimit))
> > +   xlimit = xlimit->next;
> > +  if (x != xlimit)
> > +    for (;;)
> > +      {
> > +        symtab_node_base * const xprev = x->previous;
> > +        if (xprev == NULL) break;
> > +        x = xprev;
> > +        (void) ggc_test_and_set_mark (xprev);
> > +      }
> > +  while (x != xlimit)
> > +    {
> > +      /* Code common to all symtab nodes. */
> > +      gt_ggc_m_9tree_node (x->decl);
> > +      gt_ggc_mx_symtab_node_base (x->next);
> Aren't you marking next twice? Once by xlimit walk and one by recursion?
Good catch; removed.

> > +      gt_ggc_mx_symtab_node_base (x->previous);
The comment above also applies to "previous", so I've removed this also.

> > +      gt_ggc_mx_symtab_node_base (x->next_sharing_asm_name);
> > +      gt_ggc_mx_symtab_node_base (x->previous_sharing_asm_name);
> > +      gt_ggc_mx_symtab_node_base (x->same_comdat_group);
> 
> You can skip marking these.  They only point within symbol table and
> not externally.
OK; removed.

> > +      gt_ggc_m_20vec_ipa_ref_t_va_gc_ (x->ref_list.references);
> > +      gt_ggc_m_9tree_node (x->alias_target);
> > +      gt_ggc_m_18lto_file_decl_data (x->lto_file_data);
> > +
> > +      /* Extra code, per subclass. */
> > +      switch (x->type)
> Didn't we agreed on the is_a API?

There's just one interesting subclass here, so I've converted this to:

          if (cgraph_node *cgn = dyn_cast <cgraph_node *> (x))
            {

eliminating the switch and static_cast.

> > +        {
> > +        case SYMTAB_FUNCTION:
> > +          {
> 
> > +            cgraph_node *cgn = static_cast <cgraph_node *> (x);
> > +            gt_ggc_m_11cgraph_edge (cgn->callees);
> > +            gt_ggc_m_11cgraph_edge (cgn->callers);
> > +            gt_ggc_m_11cgraph_edge (cgn->indirect_calls);
> > +            gt_ggc_m_11cgraph_node (cgn->origin);
> > +            gt_ggc_m_11cgraph_node (cgn->nested);
> > +            gt_ggc_m_11cgraph_node (cgn->next_nested);
> > +            gt_ggc_m_11cgraph_node (cgn->next_sibling_clone);
> > +            gt_ggc_m_11cgraph_node (cgn->prev_sibling_clone);
> > +            gt_ggc_m_11cgraph_node (cgn->clones);
> > +            gt_ggc_m_11cgraph_node (cgn->clone_of);
> Same as here.

Sorry, it's not clear to me what you meant by "Same as here." here.  Do
you mean that I can skip marking them because they're within the symbol
table?   If so, are you referring to all 10 fields above ("callees"
through "clone_of")?


> 
> > +            gt_ggc_m_P11cgraph_edge4htab (cgn->call_site_hash);
> Move call site hash out of GGC rather than introducing hack.

I see that this is allocated in cgraph_edge (), and it appears to be an
index.  I can create it with htab_create rather than htab_create_ggc,
but if so, there doesn't seem a natural place to call htab_delete.  Is
that OK?


> > +            gt_ggc_m_9tree_node (cgn->former_clone_of);
> > +            gt_ggc_m_11cgraph_node (cgn->global.inlined_to);
> And here

Again, do you mean that "inlined_to" can be skipped since it's in the
symbol table?


> > +            gt_ggc_m_28vec_ipa_replace_map_p_va_gc_ (cgn->clone.tree_map);
> > +            gt_ggc_m_15bitmap_head_def (cgn->clone.args_to_skip);
> > +            gt_ggc_m_15bitmap_head_def (cgn->clone.combined_args_to_skip);
> > +            gt_ggc_m_9tree_node (cgn->thunk.alias);
> > +          }
> > +          break;
> > +        default:
> > +          break;
> > +        }
> > +      x = x->next;
> > +    }
> > +}
> > +
> > +void gt_pch_nx (symtab_node_base *x)
> > +{
> > +  symtab_node_base * xlimit = x ? x->next : NULL;
> > +  while (gt_pch_note_object (xlimit, xlimit, gt_pch_p_16symtab_node_base))
> > +   xlimit = xlimit->next;
> > +  if (x != xlimit)
> > +    for (;;)
> > +      {
> > +        symtab_node_base * const xprev = x->previous;
> > +        if (xprev == NULL) break;
> > +        x = xprev;
> > +        (void) gt_pch_note_object (xprev, xprev,
> > +                                   gt_pch_p_16symtab_node_base);
> > +      }
> > +  while (x != xlimit)
> > +    {
> > +      /* Code common to all symtab nodes. */
> > +      gt_pch_n_9tree_node (x->decl);
> > +      gt_pch_nx_symtab_node_base (x->next);
> > +      gt_pch_nx_symtab_node_base (x->previous);
> > +      gt_pch_nx_symtab_node_base (x->next_sharing_asm_name);
> > +      gt_pch_nx_symtab_node_base (x->previous_sharing_asm_name);
> > +      gt_pch_nx_symtab_node_base (x->same_comdat_group);
> > +      gt_pch_n_20vec_ipa_ref_t_va_gc_ (x->ref_list.references);
> > +      gt_pch_n_9tree_node (x->alias_target);
> > +      gt_pch_n_18lto_file_decl_data (x->lto_file_data);
> > +
> > +      /* Extra code, per subclass. */
> > +      switch (x->type)
> > +        {
> > +        case SYMTAB_FUNCTION:
> > +          {
> > +            cgraph_node *cgn = static_cast <cgraph_node *> (x);
> > +            gt_pch_n_11cgraph_edge (cgn->callees);
> > +            gt_pch_n_11cgraph_edge (cgn->callers);
> > +            gt_pch_n_11cgraph_edge (cgn->indirect_calls);
> > +            gt_pch_n_11cgraph_node (cgn->origin);
> > +            gt_pch_n_11cgraph_node (cgn->nested);
> > +            gt_pch_n_11cgraph_node (cgn->next_nested);
> > +            gt_pch_n_11cgraph_node (cgn->next_sibling_clone);
> > +            gt_pch_n_11cgraph_node (cgn->prev_sibling_clone);
> > +            gt_pch_n_11cgraph_node (cgn->clones);
> > +            gt_pch_n_11cgraph_node (cgn->clone_of);
> > +            gt_pch_n_P11cgraph_edge4htab (cgn->call_site_hash);
> > +            gt_pch_n_9tree_node (cgn->former_clone_of);
> > +            gt_pch_n_11cgraph_node (cgn->global.inlined_to);
> > +            gt_pch_n_28vec_ipa_replace_map_p_va_gc_ (cgn->clone.tree_map);
> > +            gt_pch_n_15bitmap_head_def (cgn->clone.args_to_skip);
> > +            gt_pch_n_15bitmap_head_def (cgn->clone.combined_args_to_skip);
> > +            gt_pch_n_9tree_node (cgn->thunk.alias);
> 
> We can skip good part of those. Just small of those is build at PCH time. But lets do that incrementally, PCH is touchy business.

OK.  I'll just make analogous changes here to those made to the
gt_ggc_mx function.

> 
> The patch is OK with those changes.  The dummy workarounds are ugly :(
> Honza

Thanks.