fixup! [ADT] Add TrieRawHashMap

Author: cachemeifyoucan

llvm/include/llvm/ADT/TrieRawHashMap.h

@@ -105,10 +105,6 @@ class ThreadSafeTrieRawHashMapBase {
 
   public:
     PointerBase() noexcept = default;
-    PointerBase(PointerBase &&) = default;
-    PointerBase(const PointerBase &) = default;
-    PointerBase &operator=(PointerBase &&) = default;
-    PointerBase &operator=(const PointerBase &) = default;
 
   private:
     friend class ThreadSafeTrieRawHashMapBase;
@@ -228,9 +224,7 @@ class ThreadSafeTrieRawHashMap : public ThreadSafeTrieRawHashMapBase {
     friend class ThreadSafeTrieRawHashMap;
 
     ValueT *get() const {
-      if (void *B = PointerBase::get())
-        return reinterpret_cast<ValueT *>(B);
-      return nullptr;
+      return reinterpret_cast<ValueT *>(PointerBase::get());
     }
 
   public:
@@ -245,10 +239,6 @@ class ThreadSafeTrieRawHashMap : public ThreadSafeTrieRawHashMapBase {
     explicit operator bool() const { return get(); }
 
     PointerImpl() = default;
-    PointerImpl(PointerImpl &&) = default;
-    PointerImpl(const PointerImpl &) = default;
-    PointerImpl &operator=(PointerImpl &&) = default;
-    PointerImpl &operator=(const PointerImpl &) = default;
 
   protected:
     PointerImpl(PointerBase Result) : PointerBase(Result) {}
@@ -263,10 +253,6 @@ class ThreadSafeTrieRawHashMap : public ThreadSafeTrieRawHashMapBase {
 
   public:
     pointer() = default;
-    pointer(pointer &&) = default;
-    pointer(const pointer &) = default;
-    pointer &operator=(pointer &&) = default;
-    pointer &operator=(const pointer &) = default;
 
   private:
     pointer(PointerBase Result) : pointer::PointerImpl(Result) {}
@@ -277,11 +263,6 @@ class ThreadSafeTrieRawHashMap : public ThreadSafeTrieRawHashMapBase {
 
   public:
     const_pointer() = default;
-    const_pointer(const_pointer &&) = default;
-    const_pointer(const const_pointer &) = default;
-    const_pointer &operator=(const_pointer &&) = default;
-    const_pointer &operator=(const const_pointer &) = default;
-
     const_pointer(const pointer &P) : const_pointer::PointerImpl(P) {}
 
   private:

llvm/lib/Support/TrieHashIndexGenerator.h

@@ -41,7 +41,6 @@ struct IndexGenerator {
   }
 
   size_t hint(unsigned Index, unsigned Bit) {
-    assert(Index >= 0);
     assert(Bit < Bytes.size() * 8);
     assert(Bit == 0 || (Bit - NumRootBits) % NumSubtrieBits == 0);
     StartBit = Bit;

llvm/lib/Support/TrieRawHashMap.cpp

@@ -49,6 +49,7 @@ struct TrieContent final : public TrieNode {
 
   static bool classof(const TrieNode *TN) { return !TN->IsSubtrie; }
 };
+
 static_assert(sizeof(TrieContent) ==
                   ThreadSafeTrieRawHashMapBase::TrieContentBaseSize,
               "Check header assumption!");
@@ -165,7 +166,7 @@ struct ThreadSafeTrieRawHashMapBase::ImplType {
     while (!Root.Next.compare_exchange_weak(CurrentHead, S.get()))
       S->Next.exchange(CurrentHead);
 
-    // Ownership transferred to subtrie.
+    // Ownership transferred to subtrie successfully. Release the unique_ptr.
     return S.release();
   }
 
@@ -191,9 +192,13 @@ ThreadSafeTrieRawHashMapBase::getOrCreateImpl() {
   // If another thread wins this one is destroyed locally.
   std::unique_ptr<ImplType> Impl = ImplType::create(0, NumRootBits);
   ImplType *ExistingImpl = nullptr;
+
+  // If the ownership transferred succesfully, release unique_ptr and return
+  // the pointer to the new ImplType.
   if (ImplPtr.compare_exchange_strong(ExistingImpl, Impl.get()))
     return *Impl.release();
 
+  // Already created, return the existing ImplType.
   return *ExistingImpl;
 }
 

llvm/unittests/ADT/TrieRawHashMapTest.cpp

@@ -49,7 +49,7 @@ class TrieRawHashMapTestHelper {
 } // namespace llvm
 
 namespace {
-template <typename DataType, size_t HashSize>
+template <typename DataType, size_t HashSize = sizeof(uint64_t)>
 class SimpleTrieHashMapTest : public TrieRawHashMapTestHelper,
                               public ::testing::Test {
 public:
@@ -64,11 +64,7 @@ class SimpleTrieHashMapTest : public TrieRawHashMapTestHelper,
   }
 
   void destroyTrie() { Trie.reset(); }
-
-  ~SimpleTrieHashMapTest() {
-    if (Trie)
-      Trie.reset();
-  }
+  ~SimpleTrieHashMapTest() { destroyTrie(); }
 
   // Use the number itself as hash to test the pathological case.
   static HashType hash(uint64_t Num) {
@@ -83,7 +79,7 @@ class SimpleTrieHashMapTest : public TrieRawHashMapTestHelper,
   std::optional<TrieType> Trie;
 };
 
-using SmallNodeTrieTest = SimpleTrieHashMapTest<uint64_t, sizeof(uint64_t)>;
+using SmallNodeTrieTest = SimpleTrieHashMapTest<uint64_t>;
 
 TEST_F(SmallNodeTrieTest, TrieAllocation) {
   NumType Numbers[] = {
@@ -209,9 +205,7 @@ TEST_F(SmallNodeTrieTest, TrieStructureSmallFinalSubtrie) {
   }
   for (NumType N : Numbers) {
     TrieType::pointer Lookup = Trie.find(hash(N));
-    EXPECT_TRUE(Lookup);
-    if (!Lookup)
-      continue;
+    ASSERT_TRUE(Lookup);
     EXPECT_EQ(hash(N), Lookup->Hash);
     EXPECT_EQ(N, Lookup->Data);
 
@@ -273,11 +267,11 @@ TEST_F(SmallNodeTrieTest, TrieDestructionLoop) {
 
 struct NumWithDestructorT {
   uint64_t Num;
-  ~NumWithDestructorT() {}
+  llvm::function_ref<void()> DestructorCallback;
+  ~NumWithDestructorT() { DestructorCallback(); }
 };
 
-using NodeWithDestructorTrieTest =
-    SimpleTrieHashMapTest<NumWithDestructorT, sizeof(uint64_t)>;
+using NodeWithDestructorTrieTest = SimpleTrieHashMapTest<NumWithDestructorT>;
 
 TEST_F(NodeWithDestructorTrieTest, TrieDestructionLoop) {
   // Test destroying large Trie. Make sure there is no recursion that can
@@ -289,17 +283,26 @@ TEST_F(NodeWithDestructorTrieTest, TrieDestructionLoop) {
   // Fill them up. Pick a MaxN high enough to cause a stack overflow in debug
   // builds.
   static constexpr uint64_t MaxN = 100000;
+
+  uint64_t DestructorCalled = 0;
+  auto DtorCallback = [&DestructorCalled]() { ++DestructorCalled; };
   for (uint64_t N = 0; N != MaxN; ++N) {
     HashType Hash = hash(N);
-    Trie.insert(TrieType::pointer(), TrieType::value_type(Hash, NumType{N}));
+    Trie.insert(TrieType::pointer(),
+                TrieType::value_type(Hash, NumType{N, DtorCallback}));
   }
+  // Reset the count after all the temporaries get destroyed.
+  DestructorCalled = 0;
 
   // Destroy tries. If destruction is recursive and MaxN is high enough, these
   // will both fail.
   destroyTrie();
+
+  // Count the number of destructor calls during `destroyTrie()`.
+  ASSERT_EQ(DestructorCalled, MaxN);
 }
 
-using NumStrNodeTrieTest = SimpleTrieHashMapTest<std::string, sizeof(uint64_t)>;
+using NumStrNodeTrieTest = SimpleTrieHashMapTest<std::string>;
 
 TEST_F(NumStrNodeTrieTest, TrieInsertLazy) {
   for (unsigned RootBits : {2, 3, 6, 10}) {