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.
