Symbol table 6/many: Symbol table hashes

[Martin Jambor]

Hi,

a bit late, but I do have two tiny comments nevertheless...

On Mon, Apr 16, 2012 at 06:09:40PM +0200, Jan Hubicka wrote:
> Hi,
> this patch moves cgraph/varpool hashes into symbol table hashes, so the
> symbol table is actually almost a symbol table ;)
> Work done.
> 
> Bootstrapped/regtested x86_64-linux. Will commit it after bit of more
> testing - the assembler name handling is slipperly wrt aliases.
> 
> Honza
> 
> 	* cgraph.c (cgraph_hash, assembler_name_hash): Remove.
> 	(hash_node, eq_node): Remove.
> 	(cgraph_create_node): Do not handle hashtable.
> 	(cgraph_get_node): Remove.
> 	(cgraph_insert_node_to_hashtable): Remove.
> 	(hash_node_by_assembler_name): Remove.
> 	(eq_assembler_name): Remove.
> 	(cgraph_node_for_asm): Rewrite.
> 	(cgraph_find_replacement_node): Break out from ...
> 	(cgraph_remove_node): ... here; do not maintain hashtables.
> 	(change_decl_assembler_name): Remove.
> 	(cgraph_clone_node): Do not maintain hashtables.
> 	* cgraph.h (const_symtab_node): New typedef.
> 	(cgraph_insert_node_to_hashtable): Remove.
> 	(symtab_get_node, symtab_node_for_asm,
> 	symtab_insert_node_to_hashtable): Declare.
> 	(cgraph_find_replacement_node): Declare.
> 	(cgraph_get_node, varpool_get_node): Turn into inlines.
> 	(cgraph, varpool): Work sanely on NULL pointers.
> 	(FOR_EACH_SYMBOL): New walker.
> 	* ipa-inline-transform.c (save_inline_function_body): Use
> 	symtab_insert_node_to_hashtable.
> 	* symtab.c: Include ggc.h and diagnostics.h
> 	(symtab_hash, assembler_name_hash): New static vars;
> 	(hash_node, eq_node, hash_node_by_assembler_name,
> 	eq_assembler_name): New.
> 	(symtab_register_node): Update hashtables.
> 	(symtab_insert_node_to_hashtable): New.
> 	(symtab_unregister_node): Update hashtables.
> 	(symtab_get_node): New.
> 	(symtab_node_for_asm): New.
> 	(change_decl_assembler_name): New.
> 	* Makefile.in (symtab.o): Needs GTY.
> 	* varpool.c (varpool_hash): Remove.
> 	(hash_varpool_node, eq_varpool_node, varpool_get_node): Remove.
> 	(varpool_node): Rewrite using varpool_get_node.
> 	(varpool_remove_node): DO not maintain hashtables.
> 	(varpool_node_for_asm); Rewrite.
> Index: cgraph.c
> ===================================================================
> *** cgraph.c	(revision 186496)
> --- cgraph.c	(working copy)

...

> *************** cgraph_create_node_1 (void)
> *** 479,520 ****
>   struct cgraph_node *
>   cgraph_create_node (tree decl)
>   {
> !   struct cgraph_node key, *node, **slot;
> ! 
>     gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
>   
> -   if (!cgraph_hash)
> -     cgraph_hash = htab_create_ggc (10, hash_node, eq_node, NULL);
> - 
> -   key.symbol.decl = decl;
> -   slot = (struct cgraph_node **) htab_find_slot (cgraph_hash, &key, INSERT);
> -   gcc_assert (!*slot);

Here we used to assert that we do not already have that declaration in
the hash...

> Index: symtab.c
> ===================================================================
> *** symtab.c	(revision 186496)
> --- symtab.c	(working copy)
> *************** symtab_node symtab_nodes;
> *** 35,65 ****
> --- 42,166 ----
>      them, to support -fno-toplevel-reorder.  */
>   int symtab_order;
>   
> + /* Returns a hash code for P.  */
> + 
> + static hashval_t
> + hash_node (const void *p)
> + {
> +   const_symtab_node n = (const_symtab_node ) p;
> +   return (hashval_t) DECL_UID (n->symbol.decl);
> + }
> + 
> + 
> + /* Returns nonzero if P1 and P2 are equal.  */
> + 
> + static int
> + eq_node (const void *p1, const void *p2)
> + {
> +   const_symtab_node n1 = (const_symtab_node) p1;
> +   const_symtab_node n2 = (const_symtab_node) p2;
> +   return DECL_UID (n1->symbol.decl) == DECL_UID (n2->symbol.decl);
> + }
> + 
> + /* Returns a hash code for P.  */
> + 
> + static hashval_t
> + hash_node_by_assembler_name (const void *p)
> + {
> +   const_symtab_node n = (const_symtab_node) p;
> +   return (hashval_t) decl_assembler_name_hash (DECL_ASSEMBLER_NAME (n->symbol.decl));
> + }
> + 
> + /* Returns nonzero if P1 and P2 are equal.  */
> + 
> + static int
> + eq_assembler_name (const void *p1, const void *p2)
> + {
> +   const_symtab_node n1 = (const_symtab_node) p1;
> +   const_tree name = (const_tree)p2;
> +   return (decl_assembler_name_equal (n1->symbol.decl, name));
> + }
> + 
> + 
>   /* Add node into symbol table.  This function is not used directly, but via
>      cgraph/varpool node creation routines.  */
>   
>   void
>   symtab_register_node (symtab_node node)
>   {
> +   struct symtab_node_base key;
> +   symtab_node *slot;
> + 
>     node->symbol.next = symtab_nodes;
>     node->symbol.previous = NULL;
>     if (symtab_nodes)
>       symtab_nodes->symbol.previous = node;
>     symtab_nodes = node;
>   
> +   if (!symtab_hash)
> +     symtab_hash = htab_create_ggc (10, hash_node, eq_node, NULL);
> +   key.decl = node->symbol.decl;
> +   slot = (symtab_node *) htab_find_slot (symtab_hash, &key, INSERT);
> +   if (*slot == NULL)
> +     *slot = node;
> + 

...yet here we don't.  Shouldn't we?

...

> *************** symtab_remove_node (symtab_node node)
> *** 95,97 ****
> --- 250,341 ----
>     else if (symtab_variable_p (node))
>       varpool_remove_node (varpool (node));
>   }
> + 
> + /* Return the cgraph node that has ASMNAME for its DECL_ASSEMBLER_NAME.
> +    Return NULL if there's no such node.  */

symtab node :-)


Otherwise, these are all really very nice simplifications.

Martin