Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

This patch simplifies DenseMap::grow by reimplementing it in terms of
DenseMapBase::moveFrom.

Since moveFrom iterates over the bucket range, we don't need:

if (!OldBuckets)

The old bucket array is released by the destructor on Tmp.

This patch removes moveFromOldBuckets as it's no longer used with this
patch. moveFromImpl is "inlined" into moveFrom.

This patch simplifies DenseMap::grow by reimplementing it in terms of
DenseMapBase::moveFrom.

Since moveFrom iterates over the bucket range, we don't need:

  if (!OldBuckets)

The old bucket array is released by the destructor on Tmp.

This patch removes moveFromOldBuckets as it's no longer used with this
patch.  moveFromImpl is "inlined" into moveFrom.
@llvmbot
Copy link
Member

llvmbot commented Nov 15, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

This patch simplifies DenseMap::grow by reimplementing it in terms of
DenseMapBase::moveFrom.

Since moveFrom iterates over the bucket range, we don't need:

if (!OldBuckets)

The old bucket array is released by the destructor on Tmp.

This patch removes moveFromOldBuckets as it's no longer used with this
patch. moveFromImpl is "inlined" into moveFrom.


Full diff: https://github.com/llvm/llvm-project/pull/168238.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMap.h (+20-33)
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index db59bd595bca7..f559f77cca1fd 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -413,11 +413,13 @@ class DenseMapBase : public DebugEpochBase {
     return NextPowerOf2(NumEntries * 4 / 3 + 1);
   }
 
-  void moveFromImpl(iterator_range<BucketT *> OldBuckets) {
+  // Move key/value from Other to *this.
+  // Other is left in a valid but empty state.
+  void moveFrom(DerivedT &Other) {
     // Insert all the old elements.
     const KeyT EmptyKey = KeyInfoT::getEmptyKey();
     const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
-    for (BucketT &B : OldBuckets) {
+    for (BucketT &B : Other.buckets()) {
       if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey) &&
           !KeyInfoT::isEqual(B.getFirst(), TombstoneKey)) {
         // Insert the key/value into the new table.
@@ -434,17 +436,6 @@ class DenseMapBase : public DebugEpochBase {
       }
       B.getFirst().~KeyT();
     }
-  }
-
-  void moveFromOldBuckets(iterator_range<BucketT *> OldBuckets) {
-    initEmpty();
-    moveFromImpl(OldBuckets);
-  }
-
-  // Move key/value from Other to *this.
-  // Other is left in a valid but empty state.
-  void moveFrom(DerivedT &Other) {
-    moveFromImpl(Other.buckets());
     Other.derived().kill();
   }
 
@@ -738,6 +729,11 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
   unsigned NumTombstones;
   unsigned NumBuckets;
 
+  struct ExactBucketCount {};
+  explicit DenseMap(unsigned NumBuckets, ExactBucketCount) {
+    initWithExactBucketCount(NumBuckets);
+  }
+
 public:
   /// Create a DenseMap with an optional \p NumElementsToReserve to guarantee
   /// that this number of elements can be inserted in the map without grow().
@@ -824,9 +820,8 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
     return true;
   }
 
-  void init(unsigned InitNumEntries) {
-    auto InitBuckets = BaseT::getMinBucketToReserveForEntries(InitNumEntries);
-    if (allocateBuckets(InitBuckets)) {
+  void initWithExactBucketCount(unsigned NewNumBuckets) {
+    if (allocateBuckets(NewNumBuckets)) {
       this->BaseT::initEmpty();
     } else {
       NumEntries = 0;
@@ -834,6 +829,11 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
     }
   }
 
+  void init(unsigned InitNumEntries) {
+    auto InitBuckets = BaseT::getMinBucketToReserveForEntries(InitNumEntries);
+    initWithExactBucketCount(InitBuckets);
+  }
+
   // Put the zombie instance in a known good state after a move.
   void kill() {
     deallocateBuckets();
@@ -842,23 +842,10 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
   }
 
   void grow(unsigned AtLeast) {
-    unsigned OldNumBuckets = NumBuckets;
-    BucketT *OldBuckets = Buckets;
-
-    allocateBuckets(std::max<unsigned>(
-        64, static_cast<unsigned>(NextPowerOf2(AtLeast - 1))));
-    assert(Buckets);
-    if (!OldBuckets) {
-      this->BaseT::initEmpty();
-      return;
-    }
-
-    this->moveFromOldBuckets(
-        llvm::make_range(OldBuckets, OldBuckets + OldNumBuckets));
-
-    // Free the old table.
-    deallocate_buffer(OldBuckets, sizeof(BucketT) * OldNumBuckets,
-                      alignof(BucketT));
+    AtLeast = std::max<unsigned>(64, NextPowerOf2(AtLeast - 1));
+    DenseMap Tmp(AtLeast, ExactBucketCount{});
+    Tmp.moveFrom(*this);
+    swapImpl(Tmp);
   }
 
   // Plan how to shrink the bucket table.  Return:

@kazutakahirata kazutakahirata merged commit 688b190 into llvm:main Nov 16, 2025
12 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20251115a_ADT_DenseMap_DenseMap_grow branch November 16, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants