This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


> 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)
> 
> gcc/
> 
> 	* cgraph.c (gt_ggc_mx): New.
> 	(gt_pch_nx (symtab_node_base *)): New.
> 	(gt_pch_nx (symtab_node_base *, gt_pointer_operator, void *)): New.
> 	(dummy_call_site_hash): New.
> 	(dummy_ipa_ref_list): New.
> 	(dummy_cgraph_clone_info): New.
> 	(dummy_ipa_replace_map_pointer): New.
> 	(dummy_varpool_node_ptr): New.
> 
> 	* cgraph.h (symtab_node_base): Convert to a class;
> 	add GTY((user)).
> 	(gt_ggc_mx): New.
> 	(gt_pch_nx (symtab_node_base *p)): New.
> 	(gt_pch_nx (symtab_node_base *p, gt_pointer_operator op,
> 	void *cookie)): New.
> 	(cgraph_node): Inherit from symtab_node; convert to GTY((user)).
> 	(varpool_node): Likewise.
> 	(symtab_node_def): Remove.
> 	(is_a_helper <cgraph_node>::test (symtab_node_def *)): Convert to...
> 	(is_a_helper <cgraph_node>::test (symtab_node_base *)): ...this.
> 	(is_a_helper <varpool_node>::test (symtab_node_def *)): Convert to...
> 	(is_a_helper <varpool_node>::test (symtab_node_base *)): ...this.
> 
> 	* ipa-ref.h (symtab_node_def): Drop.
> 	(symtab_node): Change underlying type from symtab_node_def to
> 	symtab_node_base.
> 	(const_symtab_node): Likwise.
> 
> 	* is-a.h: Update examples in comment.
> 
> 	* symtab.c (symtab_hash): Change symtab_node_def to symtab_node_base.
> 	(assembler_name_hash): Likewise.
> ---
>  gcc/cgraph.c  | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gcc/cgraph.h  |  48 ++++++-------
>  gcc/ipa-ref.h |   6 +-
>  gcc/is-a.h    |   6 +-
>  gcc/symtab.c  |   4 +-
>  5 files changed, 247 insertions(+), 35 deletions(-)
> 
> 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?

> +      gt_ggc_mx_symtab_node_base (x->previous);
> +      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.

> +      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?
> +        {
> +        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.

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

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

> +            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.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]