Re: IMA repairs: Don't use DECL_ASSEMBLER_NAME for a key in cgraph

[Jan Hubicka <jh at suse dot cz>]

> 
> As previously discussed, using DECL_ASSEMBLER_NAME for a key in cgraph
> interferes with the C front end's desire not to set DECL_CONTEXT on
> file-scope decls until the end of the translation unit.  It is also an
> efficiency issue -- if this is not done, D_A_N need not be computed
> for decls which are never output, which can be a lot of them in a C++
> program -- so I have chosen to tackle this by making cgraph.c use
> DECL_UID for a key instead of DECL_ASSEMBLER_NAME.  This relies on the
> assumption that all front ends do indeed create exactly one decl per
> object-file symbol.  That assumption is made in several other places
> (notably dwarf2out.c and dbxout.c) so I do not mind extending it.
> 
> Of some concern, though, is that we cannot provide
> cgraph_(varpool_)node_for_identifier anymore.  The only user of this
> is varasm.c::mark_referenced.  Most of the calls to that have a DECL
> in hand; for them I have introduced a new function
> mark_decl_referenced (which, you will note, does NOT set
> TREE_SYMBOL_REFERENCED on D_A_N, to avoid instantiating D_A_N when we
> don't have to).  For those that don't (notably assemble_name), I have

This sounds like very plausible sollution.  Do you have any numbers how
much it helps to C++ copmile time perfomrance by any chance?
> applied band-aids elsewhere.  (We need to get away from this mess
> where we don't know what is going to be emitted until we start writing
> out the assembly file.  cgraph should have all the information and
> make all the decisions there.)

We should know everything in advance.  mark_reference callback simply
stops marking nodes once analysis is done.
It is mostly used to mark "used" attribute, some of C++ ABI issues where
some symbols must be output for not very obvious reasons and to deal
with data structures.  I have half done patch that fix the last, the
nasty part of it is that datastructures are still frontend specific so I
need to figure out whether I want the hooks or some kind of
gimplification on them.

Thanks for working on this!
Honza
> 
> Bootstrapped i686-linux, all languages, with one regression:
> gcc.dg/debug/dwarf2/dwarf-die5.c.  I need help fixing this; I got lost
> in dwarf2out.c.  (Patch will of course not be applied until the
> regression is fixed.  Obviously the #if 0 block in mark_referenced
> will be properly removed at that time.)
> 
> zw
> 
>         * cgraph.c (hash_node, eq_node, cgraph_node, cgraph_remove_node)
>         (cgraph_varpool_hash_node, eq_cgraph_varpool_node)
>         (cgraph_varpool_node, cgraph_function_possibly_inlined_p):
>         Use DECL_UID for the key, not DECL_ASSEMBLER_NAME.
>         (change_decl_assembler_name): No need to muck with the hash tables.
>         (cgraph_node_for_identifier, cgraph_varpool_node_for_identifier):
>         Delete.
>         * cgraph.h: Remove prototypes of deleted functions.
>         * varasm.c (mark_referenced): Abort if not handed an IDENTIFIER_NODE;
>         just set TREE_SYMBOL_REFERENCED and return.
>         (mark_decl_referenced): New function.
>         * tree.h: Prototype mark_decl_referenced.
>         * final.c (output_addr_const) <case SYMBOL_REF>: Call
>         mark_decl_referenced before assemble_name.
>         * c-decl.c (finish_decl): Use mark_decl_referenced.
> cp:
>         * decl.c (cp_finish_decl): Use mark_decl_referenced.
>         * decl2.c (maybe_make_one_only): Likewise.
>         * method.c (use_thunk): Likewise.
> 
> ===================================================================
> Index: c-decl.c
> --- c-decl.c	14 May 2004 02:29:19 -0000	1.500
> +++ c-decl.c	18 May 2004 08:05:36 -0000
> @@ -2981,7 +2981,7 @@ finish_decl (tree decl, tree init, tree 
>  
>    /* If this was marked 'used', be sure it will be output.  */
>    if (lookup_attribute ("used", DECL_ATTRIBUTES (decl)))
> -    mark_referenced (DECL_ASSEMBLER_NAME (decl));
> +    mark_decl_referenced (decl);
>  
>    if (TREE_CODE (decl) == TYPE_DECL)
>      {
> ===================================================================
> Index: cgraph.c
> --- cgraph.c	13 May 2004 06:39:32 -0000	1.49
> +++ cgraph.c	18 May 2004 08:05:36 -0000
> @@ -138,9 +138,7 @@ static int eq_node (const void *, const 
>  static hashval_t
>  hash_node (const void *p)
>  {
> -  return ((hashval_t)
> -	  IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME
> -				 (((struct cgraph_node *) p)->decl)));
> +  return ((hashval_t) DECL_UID (((struct cgraph_node *) p)->decl));
>  }
>  
>  /* Returns nonzero if P1 and P2 are equal.  */
> @@ -148,8 +146,7 @@ hash_node (const void *p)
>  static int
>  eq_node (const void *p1, const void *p2)
>  {
> -  return ((DECL_ASSEMBLER_NAME (((struct cgraph_node *) p1)->decl)) ==
> -	  (tree) p2);
> +  return (DECL_UID (((struct cgraph_node *) p1)->decl) == (unsigned int)p2);
>  }
>  
>  /* Allocate new callgraph node and insert it into basic data structures.  */
> @@ -183,9 +180,10 @@ cgraph_node (tree decl)
>      cgraph_hash = htab_create_ggc (10, hash_node, eq_node, NULL);
>  
>    slot = (struct cgraph_node **)
> -    htab_find_slot_with_hash (cgraph_hash, DECL_ASSEMBLER_NAME (decl),
> -			      IDENTIFIER_HASH_VALUE
> -			        (DECL_ASSEMBLER_NAME (decl)), INSERT);
> +    htab_find_slot_with_hash (cgraph_hash,
> +			      (void *)DECL_UID (decl),
> +			      (hashval_t)DECL_UID (decl),
> +			      INSERT);
>    if (*slot)
>      return *slot;
>  
> @@ -218,26 +216,6 @@ cgraph_edge (struct cgraph_node *node, t
>    return e;
>  }
>  
> -/* Try to find existing function for identifier ID.  */
> -struct cgraph_node *
> -cgraph_node_for_identifier (tree id)
> -{
> -  struct cgraph_node **slot;
> -
> -  if (TREE_CODE (id) != IDENTIFIER_NODE)
> -    abort ();
> -
> -  if (!cgraph_hash)
> -    return NULL;
> -
> -  slot = (struct cgraph_node **)
> -    htab_find_slot_with_hash (cgraph_hash, id,
> -			      IDENTIFIER_HASH_VALUE (id), NO_INSERT);
> -  if (!slot)
> -    return NULL;
> -  return *slot;
> -}
> -
>  /* Create edge from CALLER to CALLEE in the cgraph.  */
>  
>  struct cgraph_edge *
> @@ -347,9 +325,10 @@ cgraph_remove_node (struct cgraph_node *
>    if (node->next)
>      node->next->previous = node->previous;
>    slot = 
> -    htab_find_slot_with_hash (cgraph_hash, DECL_ASSEMBLER_NAME (node->decl),
> -			      IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME
> -						     (node->decl)), NO_INSERT);
> +    htab_find_slot_with_hash (cgraph_hash,
> +			      (void *)DECL_UID (node->decl),
> +			      (hashval_t)DECL_UID (node->decl),
> +			      NO_INSERT);
>    if (*slot == node)
>      {
>        if (node->next_clone)
> @@ -552,9 +531,7 @@ dump_cgraph (FILE *f)
>  static hashval_t
>  cgraph_varpool_hash_node (const void *p)
>  {
> -  return ((hashval_t)
> -	  IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME
> -				 (((struct cgraph_varpool_node *) p)->decl)));
> +  return ((hashval_t) DECL_UID (((struct cgraph_varpool_node *) p)->decl));
>  }
>  
>  /* Returns nonzero if P1 and P2 are equal.  */
> @@ -562,8 +539,9 @@ cgraph_varpool_hash_node (const void *p)
>  static int
>  eq_cgraph_varpool_node (const void *p1, const void *p2)
>  {
> -  return ((DECL_ASSEMBLER_NAME (((struct cgraph_varpool_node *) p1)->decl)) ==
> -	  (tree) p2);
> +  return (DECL_UID (((struct cgraph_varpool_node *) p1)->decl)
> +	  == (unsigned int) p2);
> +
>  }
>  
>  /* Return cgraph_varpool node assigned to DECL.  Create new one when needed.  */
> @@ -580,8 +558,9 @@ cgraph_varpool_node (tree decl)
>      cgraph_varpool_hash = htab_create_ggc (10, cgraph_varpool_hash_node,
>  				           eq_cgraph_varpool_node, NULL);
>    slot = (struct cgraph_varpool_node **)
> -    htab_find_slot_with_hash (cgraph_varpool_hash, DECL_ASSEMBLER_NAME (decl),
> -			      IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME (decl)),
> +    htab_find_slot_with_hash (cgraph_varpool_hash,
> +			      (void *)DECL_UID (decl),
> +			      (hashval_t)DECL_UID (decl),
>  			      INSERT);
>    if (*slot)
>      return *slot;
> @@ -597,10 +576,6 @@ cgraph_varpool_node (tree decl)
>  void
>  change_decl_assembler_name (tree decl, tree name)
>  {
> -  struct cgraph_node *node = NULL;
> -  struct cgraph_varpool_node *vnode = NULL;
> -  void **slot;
> -
>    if (!DECL_ASSEMBLER_NAME_SET_P (decl))
>      {
>        SET_DECL_ASSEMBLER_NAME (decl, name);
> @@ -613,83 +588,7 @@ change_decl_assembler_name (tree decl, t
>        && DECL_RTL_SET_P (decl))
>      warning ("%D renamed after being referenced in assembly", decl);
>  
> -  if (TREE_CODE (decl) == FUNCTION_DECL && cgraph_hash)
> -    {
> -      /* Take a look whether declaration is in the cgraph structure.  */
> -      slot = 
> -	htab_find_slot_with_hash (cgraph_hash, DECL_ASSEMBLER_NAME (decl),
> -				   IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME
> -							  (decl)), NO_INSERT);
> -      if (slot)
> -	node = *slot;
> -
> -      /* It is, verify that we are the canonical node for this decl.  */
> -      if (node && node->decl == decl)
> -	{
> -	  node = *slot;
> -	  htab_clear_slot (cgraph_hash, slot);
> -      	 }
> -       else
> -	 node = NULL;
> -    }
> -  if (TREE_CODE (decl) == VAR_DECL && TREE_STATIC (decl) && cgraph_varpool_hash)
> -    {
> -      /* Take a look whether declaration is in the cgraph structure.  */
> -      slot = 
> -	htab_find_slot_with_hash (cgraph_varpool_hash, DECL_ASSEMBLER_NAME (decl),
> -				   IDENTIFIER_HASH_VALUE (DECL_ASSEMBLER_NAME
> -							  (decl)), NO_INSERT);
> -      if (slot)
> -	vnode = *slot;
> -
> -      /* It is, verify that we are the canonical vnode for this decl.  */
> -      if (vnode && vnode->decl == decl)
> -	{
> -	  vnode = *slot;
> -	  htab_clear_slot (cgraph_varpool_hash, slot);
> -      	 }
> -       else
> -	 vnode = NULL;
> -    }
>    SET_DECL_ASSEMBLER_NAME (decl, name);
> -  if (node)
> -    {
> -      slot = 
> -	htab_find_slot_with_hash (cgraph_hash, name,
> -				  IDENTIFIER_HASH_VALUE (name), INSERT);
> -      if (*slot)
> -	abort ();
> -      *slot = node;
> -    }
> -  if (vnode)
> -    {
> -      slot = 
> -	htab_find_slot_with_hash (cgraph_varpool_hash, name,
> -				  IDENTIFIER_HASH_VALUE (name), INSERT);
> -      if (*slot)
> -	abort ();
> -      *slot = vnode;
> -    }
> -}
> -
> -/* Try to find existing function for identifier ID.  */
> -struct cgraph_varpool_node *
> -cgraph_varpool_node_for_identifier (tree id)
> -{
> -  struct cgraph_varpool_node **slot;
> -
> -  if (TREE_CODE (id) != IDENTIFIER_NODE)
> -    abort ();
> -
> -  if (!cgraph_varpool_hash)
> -    return NULL;
> -
> -  slot = (struct cgraph_varpool_node **)
> -    htab_find_slot_with_hash (cgraph_varpool_hash, id,
> -			      IDENTIFIER_HASH_VALUE (id), NO_INSERT);
> -  if (!slot)
> -    return NULL;
> -  return *slot;
>  }
>  
>  /* Notify finalize_compilation_unit that given node is reachable
> @@ -767,7 +666,7 @@ cgraph_function_possibly_inlined_p (tree
>      return (DECL_INLINE (decl) && !flag_really_no_inline);
>    if (!cgraph_inline_hash)
>      return false;
> -  return (htab_find_slot (cgraph_inline_hash, DECL_ASSEMBLER_NAME (decl),
> +  return (htab_find_slot (cgraph_inline_hash, (void *)DECL_UID (decl),
>  			  NO_INSERT) != NULL);
>  }
>  
> ===================================================================
> Index: cgraph.h
> --- cgraph.h	13 May 2004 06:39:33 -0000	1.30
> +++ cgraph.h	18 May 2004 08:05:36 -0000
> @@ -164,7 +164,6 @@ struct cgraph_edge *cgraph_create_edge (
>  				        tree);
>  struct cgraph_node *cgraph_node (tree decl);
>  struct cgraph_edge *cgraph_edge (struct cgraph_node *, tree call_expr);
> -struct cgraph_node *cgraph_node_for_identifier (tree id);
>  bool cgraph_calls_p (tree, tree);
>  struct cgraph_local_info *cgraph_local_info (tree);
>  struct cgraph_global_info *cgraph_global_info (tree);
> @@ -174,7 +173,6 @@ struct cgraph_edge * cgraph_clone_edge (
>  struct cgraph_node * cgraph_clone_node (struct cgraph_node *);
>  
>  struct cgraph_varpool_node *cgraph_varpool_node (tree decl);
> -struct cgraph_varpool_node *cgraph_varpool_node_for_identifier (tree id);
>  void cgraph_varpool_mark_needed_node (struct cgraph_varpool_node *);
>  void cgraph_varpool_finalize_decl (tree);
>  bool cgraph_varpool_assemble_pending_decls (void);
> ===================================================================
> Index: final.c
> --- final.c	13 May 2004 21:52:34 -0000	1.313
> +++ final.c	18 May 2004 08:05:36 -0000
> @@ -3252,6 +3252,8 @@ output_addr_const (FILE *file, rtx x)
>        break;
>  
>      case SYMBOL_REF:
> +      if (SYMBOL_REF_DECL (x))
> +	mark_decl_referenced (SYMBOL_REF_DECL (x));
>  #ifdef ASM_OUTPUT_SYMBOL_REF
>        ASM_OUTPUT_SYMBOL_REF (file, x);
>  #else
> ===================================================================
> Index: tree.h
> --- tree.h	18 May 2004 02:53:55 -0000	1.493
> +++ tree.h	18 May 2004 08:05:36 -0000
> @@ -3654,6 +3654,7 @@ extern void variable_section (tree, int)
>  enum tls_model decl_tls_model (tree);
>  extern void resolve_unique_section (tree, int, int);
>  extern void mark_referenced (tree);
> +extern void mark_decl_referenced (tree);
>  extern void notice_global_symbol (tree);
>  
>  /* In stmt.c */
> ===================================================================
> Index: varasm.c
> --- varasm.c	13 May 2004 06:39:52 -0000	1.424
> +++ varasm.c	18 May 2004 08:05:37 -0000
> @@ -1689,6 +1689,9 @@ assemble_label (const char *name)
>  void
>  mark_referenced (tree id)
>  {
> +  if (TREE_CODE (id) != IDENTIFIER_NODE)
> +    abort ();
> +#if 0 /* Let's see if we can get away without this.  */
>    if (!TREE_SYMBOL_REFERENCED (id))
>      {
>        struct cgraph_node *node;
> @@ -1705,7 +1708,20 @@ mark_referenced (tree id)
>        if (vnode)
>  	cgraph_varpool_mark_needed_node (vnode);
>      }
> +#endif
>    TREE_SYMBOL_REFERENCED (id) = 1;
> +}
> +
> +/* Set the symbol_referenced flag for DECL and notify callgraph.  */
> +void
> +mark_decl_referenced (tree decl)
> +{
> +  if (TREE_CODE (decl) == FUNCTION_DECL)
> +    cgraph_mark_needed_node (cgraph_node (decl));
> +  else if (TREE_CODE (decl) == VAR_DECL)
> +    cgraph_varpool_mark_needed_node (cgraph_varpool_node (decl));
> +  /* else do nothing - we can get various sorts of CST nodes here,
> +     which do not need to be marked.  */
>  }
>  
>  /* Output to FILE a reference to the assembler name of a C-level name NAME.
> ===================================================================
> Index: cp/decl.c
> --- cp/decl.c	13 May 2004 06:40:16 -0000	1.1206
> +++ cp/decl.c	18 May 2004 08:05:38 -0000
> @@ -4912,7 +4912,7 @@ cp_finish_decl (tree decl, tree init, tr
>  
>    /* If this was marked 'used', be sure it will be output.  */
>    if (lookup_attribute ("used", DECL_ATTRIBUTES (decl)))
> -    mark_referenced (DECL_ASSEMBLER_NAME (decl));
> +    mark_decl_referenced (decl);
>  }
>  
>  /* This is here for a midend callback from c-common.c.  */
> ===================================================================
> Index: cp/decl2.c
> --- cp/decl2.c	13 May 2004 06:40:17 -0000	1.707
> +++ cp/decl2.c	18 May 2004 08:05:38 -0000
> @@ -1450,7 +1450,7 @@ maybe_make_one_only (tree decl)
>  	{
>  	  DECL_COMDAT (decl) = 1;
>  	  /* Mark it needed so we don't forget to emit it.  */
> -	  mark_referenced (DECL_ASSEMBLER_NAME (decl));
> +	  mark_decl_referenced (decl);
>  	}
>      }
>  }
> ===================================================================
> Index: cp/method.c
> --- cp/method.c	10 Apr 2004 14:44:14 -0000	1.281
> +++ cp/method.c	18 May 2004 08:05:39 -0000
> @@ -353,7 +353,7 @@ use_thunk (tree thunk_fndecl, bool emit_
>       this translation unit.  */
>    TREE_ADDRESSABLE (function) = 1;
>    mark_used (function);
> -  mark_referenced (DECL_ASSEMBLER_NAME (function));
> +  mark_decl_referenced (function);
>    if (!emit_p)
>      return;
>  
> @@ -495,7 +495,7 @@ use_thunk (tree thunk_fndecl, bool emit_
>  
>        /* Since we want to emit the thunk, we explicitly mark its name as
>  	 referenced.  */
> -      mark_referenced (DECL_ASSEMBLER_NAME (thunk_fndecl));
> +      mark_decl_referenced (thunk_fndecl);
>  
>        /* But we don't want debugging information about it.  */
>        DECL_IGNORED_P (thunk_fndecl) = 1;