diff mbox

[06/12] Consolidate string hashers

Message ID 87k2uua3a9.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford June 23, 2015, 2:49 p.m. UTC
This patch replaces various string hashers with a single copy
in hash-traits.h.


gcc/
	* hash-traits.h (string_hash, nofree_string_hash): New classes.
	* genmatch.c (capture_id_map_hasher): Use nofree_string_hash.
	* passes.c (pass_registry_hasher): Likewise.
	* config/alpha/alpha.c (string_traits): Likewise.
	* config/i386/winnt.c (i386_find_on_wrapper_list): Likewise.
	* config/m32c/m32c.c (pragma_traits): Likewise.
	* config/mep/mep.c (pragma_traits): Likewise.

gcc/java/
	* jcf-io.c (memoized_class_lookups): Use nofree_string_hash.
	(find_class): Likewise.

Comments

Mikhail Maltsev June 24, 2015, 10:13 a.m. UTC | #1
On 23.06.2015 17:49, Richard Sandiford wrote:
> This patch replaces various string hashers with a single copy
> in hash-traits.h.

(snip)

> Index: gcc/config/alpha/alpha.c
> ===================================================================
> --- gcc/config/alpha/alpha.c	2015-06-23 15:48:30.751788389 +0100
> +++ gcc/config/alpha/alpha.c	2015-06-23 15:48:30.747788453 +0100
> @@ -4808,13 +4808,7 @@ alpha_multipass_dfa_lookahead (void)
>  
>  struct GTY(()) alpha_links;
>  
> -struct string_traits : default_hashmap_traits
> -{
> -  static bool equal_keys (const char *const &a, const char *const &b)
> -  {
> -    return strcmp (a, b) == 0;
> -  }
> -};
> +typedef simple_hashmap_traits <nofree_string_hash> string_traits;
>  

I remember that when we briefly discussed unification of string traits,
a looked through GCC code and this one seemed weird to me: it does not
reimplement the hash function. I.e. the pointer value is used as hash. I
wonder, is it intentional or not? This could actually work if strings
are interned (but in that case there is no need to compare them, because
comparing pointers would be enough).
Richard Sandiford June 24, 2015, 11:29 a.m. UTC | #2
Mikhail Maltsev <maltsevm@gmail.com> writes:
> On 23.06.2015 17:49, Richard Sandiford wrote:
>> Index: gcc/config/alpha/alpha.c
>> ===================================================================
>> --- gcc/config/alpha/alpha.c	2015-06-23 15:48:30.751788389 +0100
>> +++ gcc/config/alpha/alpha.c	2015-06-23 15:48:30.747788453 +0100
>> @@ -4808,13 +4808,7 @@ alpha_multipass_dfa_lookahead (void)
>>  
>>  struct GTY(()) alpha_links;
>>  
>> -struct string_traits : default_hashmap_traits
>> -{
>> -  static bool equal_keys (const char *const &a, const char *const &b)
>> -  {
>> -    return strcmp (a, b) == 0;
>> -  }
>> -};
>> +typedef simple_hashmap_traits <nofree_string_hash> string_traits;
>>  
>
> I remember that when we briefly discussed unification of string traits,
> a looked through GCC code and this one seemed weird to me: it does not
> reimplement the hash function. I.e. the pointer value is used as hash. I
> wonder, is it intentional or not? This could actually work if strings
> are interned (but in that case there is no need to compare them, because
> comparing pointers would be enough).

I think it was accidental.  The code originally used splay trees and
so didn't need to provide a hash.

SYMBOL_REF names are unique, like you say, so pointer equality should be
enough.  Even then though, htab_hash_string ought to give a better hash
than the pointer value (as well as giving a stable order, although that
isn't important here).  So IMO the patch as it stands is still an
improvement: we're keeping the existing comparison function but adding
a better hasher.

If the series goes anywhere I might look at adding a dedicated "interned
string hasher" that sits inbetween pointer_hash and string_hash.

Thanks,
Richard
Jeff Law June 25, 2015, 4:36 p.m. UTC | #3
On 06/23/2015 08:49 AM, Richard Sandiford wrote:
> This patch replaces various string hashers with a single copy
> in hash-traits.h.
>
>
> gcc/
> 	* hash-traits.h (string_hash, nofree_string_hash): New classes.
> 	* genmatch.c (capture_id_map_hasher): Use nofree_string_hash.
> 	* passes.c (pass_registry_hasher): Likewise.
> 	* config/alpha/alpha.c (string_traits): Likewise.
> 	* config/i386/winnt.c (i386_find_on_wrapper_list): Likewise.
> 	* config/m32c/m32c.c (pragma_traits): Likewise.
> 	* config/mep/mep.c (pragma_traits): Likewise.
>
> gcc/java/
> 	* jcf-io.c (memoized_class_lookups): Use nofree_string_hash.
> 	(find_class): Likewise.
OK.
jeff
diff mbox

Patch

Index: gcc/hash-traits.h
===================================================================
--- gcc/hash-traits.h	2015-06-23 15:48:30.751788389 +0100
+++ gcc/hash-traits.h	2015-06-23 15:48:30.743788520 +0100
@@ -121,6 +121,27 @@  pointer_hash <Type>::is_empty (Type *e)
   return e == NULL;
 }
 
+/* Hasher for "const char *" strings, using string rather than pointer
+   equality.  */
+
+struct string_hash : pointer_hash <const char>
+{
+  static inline hashval_t hash (const char *);
+  static inline bool equal (const char *, const char *);
+};
+
+inline hashval_t
+string_hash::hash (const char *id)
+{
+  return htab_hash_string (id);
+}
+
+inline bool
+string_hash::equal (const char *id1, const char *id2)
+{
+  return strcmp (id1, id2) == 0;
+}
+
 /* Remover and marker for entries in gc memory.  */
 
 template<typename T>
@@ -190,6 +211,11 @@  struct ggc_ptr_hash : pointer_hash <T>,
 template <typename T>
 struct ggc_cache_ptr_hash : pointer_hash <T>, ggc_cache_remove <T *> {};
 
+/* Traits for string elements that should not be freed when an element
+   is deleted.  */
+
+struct nofree_string_hash : string_hash, typed_noop_remove <const char *> {};
+
 template <typename T> struct default_hash_traits;
 
 template <typename T>
Index: gcc/genmatch.c
===================================================================
--- gcc/genmatch.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/genmatch.c	2015-06-23 15:48:30.743788520 +0100
@@ -392,26 +392,7 @@  get_operator (const char *id)
   return 0;
 }
 
-
-/* Helper for the capture-id map.  */
-
-struct capture_id_map_hasher : default_hashmap_traits
-{
-  static inline hashval_t hash (const char *);
-  static inline bool equal_keys (const char *, const char *);
-};
-
-inline hashval_t
-capture_id_map_hasher::hash (const char *id)
-{
-  return htab_hash_string (id);
-}
-
-inline bool
-capture_id_map_hasher::equal_keys (const char *id1, const char *id2)
-{
-  return strcmp (id1, id2) == 0;
-}
+typedef simple_hashmap_traits<nofree_string_hash> capture_id_map_hasher;
 
 typedef hash_map<const char *, unsigned, capture_id_map_hasher> cid_map_t;
 
Index: gcc/passes.c
===================================================================
--- gcc/passes.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/passes.c	2015-06-23 15:48:30.747788453 +0100
@@ -861,29 +861,7 @@  pass_manager::register_dump_files (opt_p
   while (pass);
 }
 
-/* Helper for pass_registry hash table.  */
-
-struct pass_registry_hasher : default_hashmap_traits
-{
-  static inline hashval_t hash (const char *);
-  static inline bool equal_keys (const char *, const char *);
-};
-
-/* Pass registry hash function.  */
-
-inline hashval_t
-pass_registry_hasher::hash (const char *name)
-{
-  return htab_hash_string (name);
-}
-
-/* Hash equal function  */
-
-inline bool
-pass_registry_hasher::equal_keys (const char *s1, const char *s2)
-{
-  return !strcmp (s1, s2);
-}
+typedef simple_hashmap_traits<nofree_string_hash> pass_registry_hasher;
 
 static hash_map<const char *, opt_pass *, pass_registry_hasher>
   *name_to_pass_map;
Index: gcc/config/alpha/alpha.c
===================================================================
--- gcc/config/alpha/alpha.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/config/alpha/alpha.c	2015-06-23 15:48:30.747788453 +0100
@@ -4808,13 +4808,7 @@  alpha_multipass_dfa_lookahead (void)
 
 struct GTY(()) alpha_links;
 
-struct string_traits : default_hashmap_traits
-{
-  static bool equal_keys (const char *const &a, const char *const &b)
-  {
-    return strcmp (a, b) == 0;
-  }
-};
+typedef simple_hashmap_traits <nofree_string_hash> string_traits;
 
 struct GTY(()) machine_function
 {
Index: gcc/config/i386/winnt.c
===================================================================
--- gcc/config/i386/winnt.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/config/i386/winnt.c	2015-06-23 15:48:30.739788568 +0100
@@ -709,29 +709,6 @@  i386_pe_record_stub (const char *name)
 
 #ifdef CXX_WRAP_SPEC_LIST
 
-/* Hashtable helpers.  */
-
-struct wrapped_symbol_hasher : nofree_ptr_hash <const char>
-{
-  static inline hashval_t hash (const char *);
-  static inline bool equal (const char *, const char *);
-  static inline void remove (const char *);
-};
-
-inline hashval_t
-wrapped_symbol_hasher::hash (const char *v)
-{
-  return htab_hash_string (v);
-}
-
-/*  Hash table equality helper function.  */
-
-inline bool
-wrapped_symbol_hasher::equal (const char *x, const char *y)
-{
-  return !strcmp (x, y);
-}
-
 /* Search for a function named TARGET in the list of library wrappers
    we are using, returning a pointer to it if found or NULL if not.
    This function might be called on quite a few symbols, and we only
@@ -743,7 +720,7 @@  wrapped_symbol_hasher::equal (const char
 i386_find_on_wrapper_list (const char *target)
 {
   static char first_time = 1;
-  static hash_table<wrapped_symbol_hasher> *wrappers;
+  static hash_table<nofree_string_hash> *wrappers;
 
   if (first_time)
     {
@@ -756,7 +733,7 @@  i386_find_on_wrapper_list (const char *t
       char *bufptr;
       /* Breaks up the char array into separated strings
          strings and enter them into the hash table.  */
-      wrappers = new hash_table<wrapped_symbol_hasher> (8);
+      wrappers = new hash_table<nofree_string_hash> (8);
       for (bufptr = wrapper_list_buffer; *bufptr; ++bufptr)
 	{
 	  char *found = NULL;
Index: gcc/config/m32c/m32c.c
===================================================================
--- gcc/config/m32c/m32c.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/config/m32c/m32c.c	2015-06-23 15:48:30.747788453 +0100
@@ -3055,16 +3055,7 @@  m32c_insert_attributes (tree node ATTRIB
     }	
 }
 
-
-struct pragma_traits : default_hashmap_traits
-{
-  static hashval_t hash (const char *str) { return htab_hash_string (str); }
-  static bool
-  equal_keys (const char *a, const char *b)
-  {
-    return !strcmp (a, b);
-  }
-};
+typedef simple_hashmap_traits<nofree_string_hash> pragma_traits;
 
 /* Hash table of pragma info.  */
 static GTY(()) hash_map<const char *, unsigned, pragma_traits> *pragma_htab;
Index: gcc/config/mep/mep.c
===================================================================
--- gcc/config/mep/mep.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/config/mep/mep.c	2015-06-23 15:48:30.743788520 +0100
@@ -4073,15 +4073,7 @@  struct GTY(()) pragma_entry {
   int flag;
 };
 
-struct pragma_traits : default_hashmap_traits
-{
-  static hashval_t hash (const char *s) { return htab_hash_string (s); }
-  static bool
-  equal_keys (const char *a, const char *b)
-  {
-    return strcmp (a, b) == 0;
-  }
-};
+typedef simple_hashmap_traits<nofree_string_hash> pragma_traits;
 
 /* Hash table of farcall-tagged sections.  */
 static GTY(()) hash_map<const char *, pragma_entry, pragma_traits> *
Index: gcc/java/jcf-io.c
===================================================================
--- gcc/java/jcf-io.c	2015-06-23 15:48:30.751788389 +0100
+++ gcc/java/jcf-io.c	2015-06-23 15:48:30.743788520 +0100
@@ -273,33 +273,11 @@  find_classfile (char *filename, JCF *jcf
   return open_class (filename, jcf, fd, dep_name);
 }
 
-
-/* Hash table helper.  */
-
-struct charstar_hash : nofree_ptr_hash <const char>
-{
-  static inline hashval_t hash (const char *candidate);
-  static inline bool equal (const char *existing, const char *candidate);
-};
-
-inline hashval_t
-charstar_hash::hash (const char *candidate)
-{
-  return htab_hash_string (candidate);
-}
-
-inline bool
-charstar_hash::equal (const char *existing, const char *candidate)
-{
-  return strcmp (existing, candidate) == 0;
-}
-
-
 /* A hash table keeping track of class names that were not found
    during class lookup.  (There is no need to cache the values
    associated with names that were found; they are saved in
    IDENTIFIER_CLASS_VALUE.)  */
-static hash_table<charstar_hash> *memoized_class_lookups;
+static hash_table<nofree_string_hash> *memoized_class_lookups;
 
 /* Returns a freshly malloc'd string with the fully qualified pathname
    of the .class file for the class CLASSNAME.  CLASSNAME must be
@@ -322,11 +300,11 @@  find_class (const char *classname, int c
 
   /* Create the hash table, if it does not already exist.  */
   if (!memoized_class_lookups)
-    memoized_class_lookups = new hash_table<charstar_hash> (37);
+    memoized_class_lookups = new hash_table<nofree_string_hash> (37);
 
   /* Loop for this class in the hashtable.  If it is present, we've
      already looked for this class and failed to find it.  */
-  hash = charstar_hash::hash (classname);
+  hash = nofree_string_hash::hash (classname);
   if (memoized_class_lookups->find_with_hash (classname, hash))
     return NULL;