Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

DenseMap.h has:

  • DenseMapBase::copyFrom
  • DenseMap::copyFrom
  • SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns. Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

  • Set NumEntries and NumTombstones to 0 unconditionally.
  • Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.

DenseMap.h has:

- DenseMapBase::copyFrom
- DenseMap::copyFrom
- SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns.  Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

- Set NumEntries and NumTombstones to 0 unconditionally.
- Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.
@llvmbot
Copy link
Member

llvmbot commented Oct 25, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

DenseMap.h has:

  • DenseMapBase::copyFrom
  • DenseMap::copyFrom
  • SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns. Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

  • Set NumEntries and NumTombstones to 0 unconditionally.
  • Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.


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

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMap.h (+16-26)
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index baa91f3a5f533..c784859081221 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -418,9 +418,16 @@ class DenseMapBase : public DebugEpochBase {
     }
   }
 
-  template <typename OtherBaseT>
-  void copyFrom(
-      const DenseMapBase<OtherBaseT, KeyT, ValueT, KeyInfoT, BucketT> &other) {
+  void copyFrom(const DerivedT &other) {
+    this->destroyAll();
+    derived().deallocateBuckets();
+    setNumEntries(0);
+    setNumTombstones(0);
+    if (!derived().allocateBuckets(other.getNumBuckets())) {
+      // The bucket list is empty.  No work to do.
+      return;
+    }
+
     assert(&other != this);
     assert(getNumBuckets() == other.getNumBuckets());
 
@@ -725,7 +732,7 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
 
   DenseMap(const DenseMap &other) : BaseT() {
     init(0);
-    copyFrom(other);
+    this->copyFrom(other);
   }
 
   DenseMap(DenseMap &&other) : BaseT() {
@@ -761,7 +768,7 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
 
   DenseMap &operator=(const DenseMap &other) {
     if (&other != this)
-      copyFrom(other);
+      this->copyFrom(other);
     return *this;
   }
 
@@ -831,17 +838,6 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
     }
   }
 
-  void copyFrom(const DenseMap &other) {
-    this->destroyAll();
-    deallocateBuckets();
-    if (allocateBuckets(other.NumBuckets)) {
-      this->BaseT::copyFrom(other);
-    } else {
-      NumEntries = 0;
-      NumTombstones = 0;
-    }
-  }
-
   void grow(unsigned AtLeast) {
     unsigned OldNumBuckets = NumBuckets;
     BucketT *OldBuckets = Buckets;
@@ -902,7 +898,7 @@ class SmallDenseMap
 
   SmallDenseMap(const SmallDenseMap &other) : BaseT() {
     init(0);
-    copyFrom(other);
+    this->copyFrom(other);
   }
 
   SmallDenseMap(SmallDenseMap &&other) : BaseT() {
@@ -1001,7 +997,7 @@ class SmallDenseMap
 
   SmallDenseMap &operator=(const SmallDenseMap &other) {
     if (&other != this)
-      copyFrom(other);
+      this->copyFrom(other);
     return *this;
   }
 
@@ -1099,7 +1095,7 @@ class SmallDenseMap
     getLargeRep()->~LargeRep();
   }
 
-  void allocateBuckets(unsigned Num) {
+  bool allocateBuckets(unsigned Num) {
     if (Num <= InlineBuckets) {
       Small = true;
     } else {
@@ -1108,6 +1104,7 @@ class SmallDenseMap
           allocate_buffer(sizeof(BucketT) * Num, alignof(BucketT)));
       new (getLargeRep()) LargeRep{NewBuckets, Num};
     }
+    return true;
   }
 
   void init(unsigned InitNumEntries) {
@@ -1116,13 +1113,6 @@ class SmallDenseMap
     this->BaseT::initEmpty();
   }
 
-  void copyFrom(const SmallDenseMap &other) {
-    this->destroyAll();
-    deallocateBuckets();
-    allocateBuckets(other.getNumBuckets());
-    this->BaseT::copyFrom(other);
-  }
-
   void grow(unsigned AtLeast) {
     if (AtLeast > InlineBuckets)
       AtLeast = std::max<unsigned>(64, NextPowerOf2(AtLeast - 1));

@kazutakahirata kazutakahirata merged commit c3a4093 into llvm:main Oct 25, 2025
12 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20251025_ADT_DenseMap_copyFrom branch October 25, 2025 17:05
dvbuka pushed a commit to dvbuka/llvm-project that referenced this pull request Oct 27, 2025
DenseMap.h has:

- DenseMapBase::copyFrom
- DenseMap::copyFrom
- SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns.  Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

- Set NumEntries and NumTombstones to 0 unconditionally.
- Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Oct 29, 2025
DenseMap.h has:

- DenseMapBase::copyFrom
- DenseMap::copyFrom
- SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns.  Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

- Set NumEntries and NumTombstones to 0 unconditionally.
- Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.
aokblast pushed a commit to aokblast/llvm-project that referenced this pull request Oct 30, 2025
DenseMap.h has:

- DenseMapBase::copyFrom
- DenseMap::copyFrom
- SmallDenseMap::copyFrom

The latter two clear and set up the storage again before delegating
DenseMapBase::copyFrom to do the actual work of copying buckets.

This patch consolidates all these into DenseMapBase::copyFrom while
eliminating name shadowing concerns.  Note that DenseMap::copyFrom and
SmallDenseMap::copyFrom are nearly identical, and they can be made
identical with small adjustments:

- Set NumEntries and NumTombstones to 0 unconditionally.
- Teach SmallDenseMap::allocateBuckets to always return true.

This patch essentially applies these adjustments and then "inlines"
the identical function body to the beginning of
DenseMapBase::copyFrom.

This patch de-templatizes DenseMapBase::copyFrom because nobody calls
it with any type other than DerivedT.
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