Revert "[ADT][StringMap] Add ability to precompute and reuse the string hash"

Crash identified internally in lld's use of StringMap in
`compareSections`. Will investigate offline before recommitting.

This reverts commit 67c631d.

Author: dwblaikie

llvm/include/llvm/ADT/StringMap.h

@@ -17,7 +17,6 @@
 #include "llvm/ADT/StringMapEntry.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Support/AllocatorBase.h"
-#include "llvm/Support/DJB.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include <initializer_list>
 #include <iterator>
@@ -61,20 +60,12 @@ class StringMapImpl {
   /// specified bucket will be non-null.  Otherwise, it will be null.  In either
   /// case, the FullHashValue field of the bucket will be set to the hash value
   /// of the string.
-  unsigned LookupBucketFor(StringRef Key) {
-    return LookupBucketFor(Key, hash(Key));
-  }
-
-  /// Overload that explicitly takes precomputed hash(Key).
-  unsigned LookupBucketFor(StringRef Key, uint32_t FullHashValue);
+  unsigned LookupBucketFor(StringRef Key);
 
   /// FindKey - Look up the bucket that contains the specified key. If it exists
   /// in the map, return the bucket number of the key.  Otherwise return -1.
   /// This does not modify the map.
-  int FindKey(StringRef Key) const { return FindKey(Key, hash(Key)); }
-
-  /// Overload that explicitly takes precomputed hash(Key).
-  int FindKey(StringRef Key, uint32_t FullHashValue) const;
+  int FindKey(StringRef Key) const;
 
   /// RemoveKey - Remove the specified StringMapEntry from the table, but do not
   /// delete it.  This aborts if the value isn't in the table.
@@ -103,13 +94,6 @@ class StringMapImpl {
   bool empty() const { return NumItems == 0; }
   unsigned size() const { return NumItems; }
 
-  /// Returns the hash value that will be used for the given string.
-  /// This allows precomputing the value and passing it explicitly
-  /// to some of the functions.
-  /// The implementation of this function is not guaranteed to be stable
-  /// and may change.
-  static uint32_t hash(StringRef Key);
-
   void swap(StringMapImpl &Other) {
     std::swap(TheTable, Other.TheTable);
     std::swap(NumBuckets, Other.NumBuckets);
@@ -231,19 +215,15 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
                       StringMapKeyIterator<ValueTy>(end()));
   }
 
-  iterator find(StringRef Key) { return find(Key, hash(Key)); }
-
-  iterator find(StringRef Key, uint32_t FullHashValue) {
-    int Bucket = FindKey(Key, FullHashValue);
+  iterator find(StringRef Key) {
+    int Bucket = FindKey(Key);
     if (Bucket == -1)
       return end();
     return iterator(TheTable + Bucket, true);
   }
 
-  const_iterator find(StringRef Key) const { return find(Key, hash(Key)); }
-
-  const_iterator find(StringRef Key, uint32_t FullHashValue) const {
-    int Bucket = FindKey(Key, FullHashValue);
+  const_iterator find(StringRef Key) const {
+    int Bucket = FindKey(Key);
     if (Bucket == -1)
       return end();
     return const_iterator(TheTable + Bucket, true);
@@ -325,13 +305,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
   /// if and only if the insertion takes place, and the iterator component of
   /// the pair points to the element with key equivalent to the key of the pair.
   std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> KV) {
-    return try_emplace_with_hash(KV.first, hash(KV.first),
-                                 std::move(KV.second));
-  }
-
-  std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> KV,
-                                   uint32_t FullHashValue) {
-    return try_emplace_with_hash(KV.first, FullHashValue, std::move(KV.second));
+    return try_emplace(KV.first, std::move(KV.second));
   }
 
   /// Inserts elements from range [first, last). If multiple elements in the
@@ -365,14 +339,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
   /// the pair points to the element with key equivalent to the key of the pair.
   template <typename... ArgsTy>
   std::pair<iterator, bool> try_emplace(StringRef Key, ArgsTy &&...Args) {
-    return try_emplace_with_hash(Key, hash(Key), std::forward<ArgsTy>(Args)...);
-  }
-
-  template <typename... ArgsTy>
-  std::pair<iterator, bool> try_emplace_with_hash(StringRef Key,
-                                                  uint32_t FullHashValue,
-                                                  ArgsTy &&...Args) {
-    unsigned BucketNo = LookupBucketFor(Key, FullHashValue);
+    unsigned BucketNo = LookupBucketFor(Key);
     StringMapEntryBase *&Bucket = TheTable[BucketNo];
     if (Bucket && Bucket != getTombstoneVal())
       return std::make_pair(iterator(TheTable + BucketNo, false),

llvm/lib/Support/StringMap.cpp

@@ -43,8 +43,6 @@ static inline unsigned *getHashTable(StringMapEntryBase **TheTable,
   return reinterpret_cast<unsigned *>(TheTable + NumBuckets + 1);
 }
 
-uint32_t StringMapImpl::hash(StringRef Key) { return xxh3_64bits(Key); }
-
 StringMapImpl::StringMapImpl(unsigned InitSize, unsigned itemSize) {
   ItemSize = itemSize;
 
@@ -83,14 +81,11 @@ void StringMapImpl::init(unsigned InitSize) {
 /// specified bucket will be non-null.  Otherwise, it will be null.  In either
 /// case, the FullHashValue field of the bucket will be set to the hash value
 /// of the string.
-unsigned StringMapImpl::LookupBucketFor(StringRef Name,
-                                        uint32_t FullHashValue) {
-#ifdef EXPENSIVE_CHECKS
-  assert(FullHashValue == hash(Name));
-#endif
+unsigned StringMapImpl::LookupBucketFor(StringRef Name) {
   // Hash table unallocated so far?
   if (NumBuckets == 0)
     init(16);
+  unsigned FullHashValue = xxh3_64bits(Name);
   if (shouldReverseIterate())
     FullHashValue = ~FullHashValue;
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);
@@ -144,12 +139,10 @@ unsigned StringMapImpl::LookupBucketFor(StringRef Name,
 /// FindKey - Look up the bucket that contains the specified key. If it exists
 /// in the map, return the bucket number of the key.  Otherwise return -1.
 /// This does not modify the map.
-int StringMapImpl::FindKey(StringRef Key, uint32_t FullHashValue) const {
+int StringMapImpl::FindKey(StringRef Key) const {
   if (NumBuckets == 0)
     return -1; // Really empty table?
-#ifdef EXPENSIVE_CHECKS
-  assert(FullHashValue == hash(Key);
-#endif
+  unsigned FullHashValue = xxh3_64bits(Key);
   if (shouldReverseIterate())
     FullHashValue = ~FullHashValue;
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);

llvm/unittests/ADT/StringMapTest.cpp

@@ -487,17 +487,6 @@ TEST_F(StringMapTest, NotEqualWithDifferentValues) {
   ASSERT_TRUE(B != A);
 }
 
-TEST_F(StringMapTest, PrecomputedHash) {
-  StringMap<int> A;
-  StringRef Key = "foo";
-  int Value = 42;
-  uint64_t Hash = StringMap<int>::hash(Key);
-  A.insert({"foo", Value}, Hash);
-  auto I = A.find(Key, Hash);
-  ASSERT_NE(I, A.end());
-  ASSERT_EQ(I->second, Value);
-}
-
 struct Countable {
   int &InstanceCount;
   int Number;