NFC: Clean up the implementation of StringPool a bit, and remove dependence on some "implicitly MallocAllocator" based methods on StringMapEntry. This allows reducing the #includes in StringMapEntry.h.

Summary:
StringPool has many caveats and isn't used in the monorepo.  I will
propose removing it as a patch separate from this refactoring patch.

Reviewers: rriddle

Subscribers: hiraditya, dexonsmith, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D77976

llvm/include/llvm/ADT/StringMap.h

@@ -14,6 +14,7 @@
 #define LLVM_ADT_STRINGMAP_H
 
 #include "llvm/ADT/StringMapEntry.h"
+#include "llvm/Support/AllocatorBase.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include <initializer_list>
 #include <iterator>

llvm/include/llvm/ADT/StringMapEntry.h

@@ -16,7 +16,6 @@
 #define LLVM_ADT_STRINGMAPENTRY_H
 
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/AllocatorBase.h"
 
 namespace llvm {
 
@@ -113,17 +112,6 @@ public:
     return newItem;
   }
 
-  /// Create - Create a StringMapEntry with normal malloc/free.
-  template <typename... InitType>
-  static StringMapEntry *Create(StringRef key, InitType &&... initVal) {
-    MallocAllocator allocator;
-    return Create(key, allocator, std::forward<InitType>(initVal)...);
-  }
-
-  static StringMapEntry *Create(StringRef key) {
-    return Create(key, ValueTy());
-  }
-
   /// GetStringMapEntryFromKeyData - Given key data that is known to be embedded
   /// into a StringMapEntry, return the StringMapEntry itself.
   static StringMapEntry &GetStringMapEntryFromKeyData(const char *keyData) {
@@ -139,12 +127,6 @@ public:
     this->~StringMapEntry();
     allocator.Deallocate(static_cast<void *>(this), AllocSize);
   }
-
-  /// Destroy this object, releasing memory back to the malloc allocator.
-  void Destroy() {
-    MallocAllocator allocator;
-    Destroy(allocator);
-  }
 };
 
 } // end namespace llvm

llvm/include/llvm/Support/StringPool.h

@@ -1,4 +1,4 @@
-//===- StringPool.h - Interned string pool ----------------------*- C++ -*-===//
+//===- StringPool.h - Intern'd string pool ----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,8 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file declares an interned string pool, which helps reduce the cost of
-// strings by using the same storage for identical strings.
+// This file declares an interned string pool with separately malloc and
+// reference counted entries.  This can reduce the cost of strings by using the
+// same storage for identical strings.
 //
 // To intern a string:
 //
@@ -29,110 +30,112 @@
 #define LLVM_SUPPORT_STRINGPOOL_H
 
 #include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringRef.h"
-#include <cassert>
 
 namespace llvm {
 
-  class PooledStringPtr;
+class PooledStringPtr;
 
-  /// StringPool - An interned string pool. Use the intern method to add a
-  /// string. Strings are removed automatically as PooledStringPtrs are
-  /// destroyed.
-  class StringPool {
-    /// PooledString - This is the value of an entry in the pool's interning
-    /// table.
-    struct PooledString {
-      StringPool *Pool = nullptr;  ///< So the string can remove itself.
-      unsigned Refcount = 0;       ///< Number of referencing PooledStringPtrs.
-
-    public:
-      PooledString() = default;
-    };
-
-    friend class PooledStringPtr;
-
-    using table_t = StringMap<PooledString>;
-    using entry_t = StringMapEntry<PooledString>;
-    table_t InternTable;
+/// StringPool - An interned string pool. Use the intern method to add a
+/// string. Strings are removed automatically as PooledStringPtrs are
+/// destroyed.
+class StringPool {
+  /// PooledString - This is the value of an entry in the pool's interning
+  /// table.
+  struct PooledString {
+    StringPool *pool = nullptr; ///< So the string can remove itself.
+    unsigned refcount = 0;      ///< Number of referencing PooledStringPtrs.
 
   public:
-    StringPool();
-    ~StringPool();
-
-    /// intern - Adds a string to the pool and returns a reference-counted
-    /// pointer to it. No additional memory is allocated if the string already
-    /// exists in the pool.
-    PooledStringPtr intern(StringRef Str);
-
-    /// empty - Checks whether the pool is empty. Returns true if so.
-    ///
-    inline bool empty() const { return InternTable.empty(); }
+    PooledString() = default;
   };
 
-  /// PooledStringPtr - A pointer to an interned string. Use operator bool to
-  /// test whether the pointer is valid, and operator * to get the string if so.
-  /// This is a lightweight value class with storage requirements equivalent to
-  /// a single pointer, but it does have reference-counting overhead when
-  /// copied.
-  class PooledStringPtr {
-    using entry_t = StringPool::entry_t;
+  friend class PooledStringPtr;
+  using Entry = StringMapEntry<PooledString>;
+  StringMap<PooledString> internTable;
 
-    entry_t *S = nullptr;
+public:
+  StringPool();
+  ~StringPool();
 
-  public:
-    PooledStringPtr() = default;
+  /// intern - Adds a string to the pool and returns a reference-counted
+  /// pointer to it. No additional memory is allocated if the string already
+  /// exists in the pool.
+  PooledStringPtr intern(StringRef string);
 
-    explicit PooledStringPtr(entry_t *E) : S(E) {
-      if (S) ++S->getValue().Refcount;
+  /// empty - Checks whether the pool is empty. Returns true if so.
+  bool empty() const { return internTable.empty(); }
+};
+
+/// PooledStringPtr - A pointer to an interned string. Use operator bool to
+/// test whether the pointer is valid, and operator * to get the string if so.
+/// This is a lightweight value class with storage requirements equivalent to
+/// a single pointer, but it does have reference-counting overhead when
+/// copied.
+class PooledStringPtr {
+  using Entry = StringPool::Entry;
+  Entry *entry = nullptr;
+
+public:
+  PooledStringPtr() = default;
+
+  explicit PooledStringPtr(Entry *e) : entry(e) {
+    if (entry)
+      ++entry->getValue().refcount;
+  }
+
+  PooledStringPtr(const PooledStringPtr &that) : entry(that.entry) {
+    if (entry)
+      ++entry->getValue().refcount;
+  }
+
+  PooledStringPtr &operator=(const PooledStringPtr &that) {
+    if (entry != that.entry) {
+      clear();
+      entry = that.entry;
+      if (entry)
+        ++entry->getValue().refcount;
     }
+    return *this;
+  }
 
-    PooledStringPtr(const PooledStringPtr &That) : S(That.S) {
-      if (S) ++S->getValue().Refcount;
+  void clear() {
+    if (!entry)
+      return;
+    if (--entry->getValue().refcount == 0) {
+      entry->getValue().pool->internTable.remove(entry);
+      MallocAllocator allocator;
+      entry->Destroy(allocator);
     }
+    entry = nullptr;
+  }
 
-    PooledStringPtr &operator=(const PooledStringPtr &That) {
-      if (S != That.S) {
-        clear();
-        S = That.S;
-        if (S) ++S->getValue().Refcount;
-      }
-      return *this;
-    }
+  ~PooledStringPtr() { clear(); }
 
-    void clear() {
-      if (!S)
-        return;
-      if (--S->getValue().Refcount == 0) {
-        S->getValue().Pool->InternTable.remove(S);
-        S->Destroy();
-      }
-      S = nullptr;
-    }
+  const char *begin() const {
+    assert(*this && "Attempt to dereference empty PooledStringPtr!");
+    return entry->getKeyData();
+  }
 
-    ~PooledStringPtr() { clear(); }
+  const char *end() const {
+    assert(*this && "Attempt to dereference empty PooledStringPtr!");
+    return entry->getKeyData() + entry->getKeyLength();
+  }
 
-    inline const char *begin() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyData();
-    }
+  unsigned size() const {
+    assert(*this && "Attempt to dereference empty PooledStringPtr!");
+    return entry->getKeyLength();
+  }
 
-    inline const char *end() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyData() + S->getKeyLength();
-    }
+  const char *operator*() const { return begin(); }
+  explicit operator bool() const { return entry != nullptr; }
 
-    inline unsigned size() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyLength();
-    }
-
-    inline const char *operator*() const { return begin(); }
-    inline explicit operator bool() const { return S != nullptr; }
-
-    inline bool operator==(const PooledStringPtr &That) const { return S == That.S; }
-    inline bool operator!=(const PooledStringPtr &That) const { return S != That.S; }
-  };
+  bool operator==(const PooledStringPtr &that) const {
+    return entry == that.entry;
+  }
+  bool operator!=(const PooledStringPtr &that) const {
+    return entry != that.entry;
+  }
+};
 
 } // end namespace llvm
 

llvm/lib/IR/Value.cpp

@@ -128,8 +128,10 @@ void Value::deleteValue() {
 
 void Value::destroyValueName() {
   ValueName *Name = getValueName();
-  if (Name)
-    Name->Destroy();
+  if (Name) {
+    MallocAllocator Allocator;
+    Name->Destroy(Allocator);
+  }
   setValueName(nullptr);
 }
 
@@ -312,7 +314,8 @@ void Value::setNameImpl(const Twine &NewName) {
     destroyValueName();
 
     // Create the new name.
-    setValueName(ValueName::Create(NameRef));
+    MallocAllocator Allocator;
+    setValueName(ValueName::Create(NameRef, Allocator));
     getValueName()->setValue(this);
     return;
   }

llvm/lib/IR/ValueSymbolTable.cpp

@@ -31,7 +31,7 @@ using namespace llvm;
 
 // Class destructor
 ValueSymbolTable::~ValueSymbolTable() {
-#ifndef NDEBUG   // Only do this in -g mode...
+#ifndef NDEBUG // Only do this in -g mode...
   for (const auto &VI : vmap)
     dbgs() << "Value still in symbol table! Type = '"
            << *VI.getValue()->getType() << "' Name = '" << VI.getKeyData()
@@ -69,7 +69,7 @@ ValueName *ValueSymbolTable::makeUniqueName(Value *V,
 
 // Insert a value into the symbol table with the specified name...
 //
-void ValueSymbolTable::reinsertValue(Value* V) {
+void ValueSymbolTable::reinsertValue(Value *V) {
   assert(V->hasName() && "Can't insert nameless Value into symbol table");
 
   // Try inserting the name, assuming it won't conflict.
@@ -83,7 +83,8 @@ void ValueSymbolTable::reinsertValue(Value* V) {
   SmallString<256> UniqueName(V->getName().begin(), V->getName().end());
 
   // The name is too already used, just free it so we can allocate a new name.
-  V->getValueName()->Destroy();
+  MallocAllocator Allocator;
+  V->getValueName()->Destroy(Allocator);
 
   ValueName *VN = makeUniqueName(V, UniqueName);
   V->setValueName(VN);
@@ -116,11 +117,11 @@ ValueName *ValueSymbolTable::createValueName(StringRef Name, Value *V) {
 // dump - print out the symbol table
 //
 LLVM_DUMP_METHOD void ValueSymbolTable::dump() const {
-  //dbgs() << "ValueSymbolTable:\n";
+  // dbgs() << "ValueSymbolTable:\n";
   for (const auto &I : *this) {
-    //dbgs() << "  '" << I->getKeyData() << "' = ";
+    // dbgs() << "  '" << I->getKeyData() << "' = ";
     I.getValue()->dump();
-    //dbgs() << "\n";
+    // dbgs() << "\n";
   }
 }
 #endif

llvm/lib/Support/StringPool.cpp

@@ -1,4 +1,4 @@
-//===-- StringPool.cpp - Interned string pool -----------------------------===//
+//===-- StringPool.cpp - Intern'd string pool -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -11,25 +11,23 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/StringPool.h"
-#include "llvm/ADT/StringRef.h"
-#include <cassert>
-
 using namespace llvm;
 
 StringPool::StringPool() {}
 
 StringPool::~StringPool() {
-  assert(InternTable.empty() && "PooledStringPtr leaked!");
+  assert(internTable.empty() && "PooledStringPtr leaked!");
 }
 
-PooledStringPtr StringPool::intern(StringRef Key) {
-  table_t::iterator I = InternTable.find(Key);
-  if (I != InternTable.end())
-    return PooledStringPtr(&*I);
+PooledStringPtr StringPool::intern(StringRef key) {
+  auto it = internTable.find(key);
+  if (it != internTable.end())
+    return PooledStringPtr(&*it);
 
-  entry_t *S = entry_t::Create(Key);
-  S->getValue().Pool = this;
-  InternTable.insert(S);
+  MallocAllocator allocator;
+  auto *entry = Entry::Create(key, allocator);
+  entry->getValue().pool = this;
+  internTable.insert(entry);
 
-  return PooledStringPtr(S);
+  return PooledStringPtr(entry);
 }

llvm/unittests/ADT/StringMapTest.cpp

@@ -224,9 +224,10 @@ TEST_F(StringMapTest, IterationTest) {
 
 // Test StringMapEntry::Create() method.
 TEST_F(StringMapTest, StringMapEntryTest) {
+  MallocAllocator Allocator;
   StringMap<uint32_t>::value_type* entry =
       StringMap<uint32_t>::value_type::Create(
-          StringRef(testKeyFirst, testKeyLength), 1u);
+          StringRef(testKeyFirst, testKeyLength), Allocator, 1u);
   EXPECT_STREQ(testKey, entry->first().data());
   EXPECT_EQ(1u, entry->second);
   free(entry);
@@ -353,14 +354,15 @@ TEST_F(StringMapTest, MoveOnly) {
   StringMap<MoveOnly> t;
   t.insert(std::make_pair("Test", MoveOnly(42)));
   StringRef Key = "Test";
-  StringMapEntry<MoveOnly>::Create(Key, MoveOnly(42))
-      ->Destroy();
+  StringMapEntry<MoveOnly>::Create(Key, t.getAllocator(), MoveOnly(42))
+      ->Destroy(t.getAllocator());
 }
 
 TEST_F(StringMapTest, CtorArg) {
   StringRef Key = "Test";
-  StringMapEntry<MoveOnly>::Create(Key, Immovable())
-      ->Destroy();
+  MallocAllocator Allocator;
+  StringMapEntry<MoveOnly>::Create(Key, Allocator, Immovable())
+      ->Destroy(Allocator);
 }
 
 TEST_F(StringMapTest, MoveConstruct) {

llvm/unittests/ADT/StringSetTest.cpp

@@ -1,4 +1,4 @@
-//===- llvm/unittest/ADT/StringSetTest.cpp - StringSet unit tests ----------===//
+//===- llvm/unittest/ADT/StringSetTest.cpp - StringSet unit tests ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -33,12 +33,13 @@ TEST_F(StringSetTest, InsertAndCountStringMapEntry) {
   // Test insert(StringMapEntry) and count(StringMapEntry)
   // which are required for set_difference(StringSet, StringSet).
   StringSet<> Set;
-  StringMapEntry<StringRef> *Element = StringMapEntry<StringRef>::Create("A");
+  StringMapEntry<StringRef> *Element =
+      StringMapEntry<StringRef>::Create("A", Set.getAllocator());
   Set.insert(*Element);
   size_t Count = Set.count(*Element);
   size_t Expected = 1;
   EXPECT_EQ(Expected, Count);
-  Element->Destroy();
+  Element->Destroy(Set.getAllocator());
 }
 
 TEST_F(StringSetTest, EmptyString) {