Skip to content

Conversation

kazutakahirata
Copy link
Contributor

Without this patch, it's overly complicated to create iterators in
DenseMap.

  • We must specify whether to advance a newly created iterator, which
    is needed in begin().
  • We call shouldReverseIterate outside and inside DenseMapIterator.

This patch cleans up all this by creating factory methods:

  • DenseMapIterator::makeBegin
  • DenseMapIterator::makeEnd
  • DenseMapIterator::makeIterator

With these:

  • makeBegin knows that we need to advance the iterator to the first
    valid bucket.
  • Callers outside DenseMapIterator do not reference
    shouldReverseIterate at all.

Now, it's a lot simpler to call helper functions
DenseMapBase::{makeIterator,makeConstIterator}. We just have to pass
the Bucket pointer:

makeIterator(Bucket);

and they take care of the rest, including passing *this as Epoch.

Without this patch, it's overly complicated to create iterators in
DenseMap.

- We must specify whether to advance a newly created iterator, which
  is needed in begin().
- We call shouldReverseIterate outside and inside DenseMapIterator.

This patch cleans up all this by creating factory methods:

- DenseMapIterator::makeBegin
- DenseMapIterator::makeEnd
- DenseMapIterator::makeIterator

With these:

- makeBegin knows that we need to advance the iterator to the first
  valid bucket.
- Callers outside DenseMapIterator do not reference
  shouldReverseIterate at all.

Now, it's a lot simpler to call helper functions
DenseMapBase::{makeIterator,makeConstIterator}.  We just have to pass
the Bucket pointer:

  makeIterator(Bucket);

and they take care of the rest, including passing *this as Epoch.
@llvmbot
Copy link
Member

llvmbot commented Aug 31, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

Without this patch, it's overly complicated to create iterators in
DenseMap.

  • We must specify whether to advance a newly created iterator, which
    is needed in begin().
  • We call shouldReverseIterate outside and inside DenseMapIterator.

This patch cleans up all this by creating factory methods:

  • DenseMapIterator::makeBegin
  • DenseMapIterator::makeEnd
  • DenseMapIterator::makeIterator

With these:

  • makeBegin knows that we need to advance the iterator to the first
    valid bucket.
  • Callers outside DenseMapIterator do not reference
    shouldReverseIterate at all.

Now, it's a lot simpler to call helper functions
DenseMapBase::{makeIterator,makeConstIterator}. We just have to pass
the Bucket pointer:

makeIterator(Bucket);

and they take care of the rest, including passing *this as Epoch.


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

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMap.h (+49-57)
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 4a2da7f630663..0650dc2044ca9 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -76,26 +76,14 @@ class DenseMapBase : public DebugEpochBase {
       DenseMapIterator<KeyT, ValueT, KeyInfoT, BucketT, true>;
 
   inline iterator begin() {
-    // When the map is empty, avoid the overhead of advancing/retreating past
-    // empty buckets.
-    if (empty())
-      return end();
-    if (shouldReverseIterate<KeyT>())
-      return makeIterator(getBucketsEnd() - 1, getBuckets(), *this);
-    return makeIterator(getBuckets(), getBucketsEnd(), *this);
-  }
-  inline iterator end() {
-    return makeIterator(getBucketsEnd(), getBucketsEnd(), *this, true);
+    return iterator::makeBegin(buckets(), empty(), *this);
   }
+  inline iterator end() { return iterator::makeEnd(buckets(), *this); }
   inline const_iterator begin() const {
-    if (empty())
-      return end();
-    if (shouldReverseIterate<KeyT>())
-      return makeConstIterator(getBucketsEnd() - 1, getBuckets(), *this);
-    return makeConstIterator(getBuckets(), getBucketsEnd(), *this);
+    return const_iterator::makeBegin(buckets(), empty(), *this);
   }
   inline const_iterator end() const {
-    return makeConstIterator(getBucketsEnd(), getBucketsEnd(), *this, true);
+    return const_iterator::makeEnd(buckets(), *this);
   }
 
   // Return an iterator to iterate over keys in the map.
@@ -184,17 +172,13 @@ class DenseMapBase : public DebugEpochBase {
   /// type used.
   template <class LookupKeyT> iterator find_as(const LookupKeyT &Val) {
     if (BucketT *Bucket = doFind(Val))
-      return makeIterator(
-          Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
-          *this, true);
+      return makeIterator(Bucket);
     return end();
   }
   template <class LookupKeyT>
   const_iterator find_as(const LookupKeyT &Val) const {
     if (const BucketT *Bucket = doFind(Val))
-      return makeConstIterator(
-          Bucket, shouldReverseIterate<KeyT>() ? getBuckets() : getBucketsEnd(),
-          *this, true);
+      return makeConstIterator(Bucket);
     return end();
   }
 
@@ -264,13 +248,13 @@ class DenseMapBase : public DebugEpochBase {
                                       const LookupKeyT &Val) {
     BucketT *TheBucket;
     if (LookupBucketFor(Val, TheBucket))
-      return {makeInsertIterator(TheBucket), false}; // Already in map.
+      return {makeIterator(TheBucket), false}; // Already in map.
 
     // Otherwise, insert the new element.
     TheBucket = findBucketForInsertion(Val, TheBucket);
     TheBucket->getFirst() = std::move(KV.first);
     ::new (&TheBucket->getSecond()) ValueT(std::move(KV.second));
-    return {makeInsertIterator(TheBucket), true};
+    return {makeIterator(TheBucket), true};
   }
 
   /// insert - Range insertion of pairs.
@@ -482,33 +466,15 @@ class DenseMapBase : public DebugEpochBase {
   std::pair<iterator, bool> try_emplace_impl(KeyArgT &&Key, Ts &&...Args) {
     auto [Bucket, Inserted] = lookupOrInsertIntoBucket(
         std::forward<KeyArgT>(Key), std::forward<Ts>(Args)...);
-    return {makeInsertIterator(Bucket), Inserted};
+    return {makeIterator(Bucket), Inserted};
   }
 
-  iterator makeIterator(BucketT *P, BucketT *E, DebugEpochBase &Epoch,
-                        bool NoAdvance = false) {
-    if (shouldReverseIterate<KeyT>()) {
-      BucketT *B = P == getBucketsEnd() ? getBuckets() : P + 1;
-      return iterator(B, E, Epoch, NoAdvance);
-    }
-    return iterator(P, E, Epoch, NoAdvance);
+  iterator makeIterator(BucketT *TheBucket) {
+    return iterator::makeIterator(TheBucket, buckets(), *this);
   }
 
-  const_iterator makeConstIterator(const BucketT *P, const BucketT *E,
-                                   const DebugEpochBase &Epoch,
-                                   const bool NoAdvance = false) const {
-    if (shouldReverseIterate<KeyT>()) {
-      const BucketT *B = P == getBucketsEnd() ? getBuckets() : P + 1;
-      return const_iterator(B, E, Epoch, NoAdvance);
-    }
-    return const_iterator(P, E, Epoch, NoAdvance);
-  }
-
-  iterator makeInsertIterator(BucketT *TheBucket) {
-    return makeIterator(TheBucket,
-                        shouldReverseIterate<KeyT>() ? getBuckets()
-                                                     : getBucketsEnd(),
-                        *this, true);
+  const_iterator makeConstIterator(const BucketT *TheBucket) const {
+    return const_iterator::makeIterator(TheBucket, buckets(), *this);
   }
 
   unsigned getNumEntries() const {
@@ -555,6 +521,10 @@ class DenseMapBase : public DebugEpochBase {
     return llvm::make_range(getBuckets(), getBucketsEnd());
   }
 
+  iterator_range<const BucketT *> buckets() const {
+    return llvm::make_range(getBuckets(), getBucketsEnd());
+  }
+
   void grow(unsigned AtLeast) { static_cast<DerivedT *>(this)->grow(AtLeast); }
 
   void shrink_and_clear() { static_cast<DerivedT *>(this)->shrink_and_clear(); }
@@ -1217,21 +1187,43 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
   pointer Ptr = nullptr;
   pointer End = nullptr;
 
-public:
-  DenseMapIterator() = default;
-
-  DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch,
-                   bool NoAdvance = false)
+  DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch)
       : DebugEpochBase::HandleBase(&Epoch), Ptr(Pos), End(E) {
     assert(isHandleInSync() && "invalid construction!");
+  }
 
-    if (NoAdvance)
-      return;
+public:
+  DenseMapIterator() = default;
+
+  static DenseMapIterator makeBegin(iterator_range<pointer> Buckets,
+                                    bool IsEmpty, const DebugEpochBase &Epoch) {
+    // When the map is empty, avoid the overhead of advancing/retreating past
+    // empty buckets.
+    if (IsEmpty)
+      return makeEnd(Buckets, Epoch);
     if (shouldReverseIterate<KeyT>()) {
-      RetreatPastEmptyBuckets();
-      return;
+      DenseMapIterator Iter(Buckets.end(), Buckets.begin(), Epoch);
+      Iter.RetreatPastEmptyBuckets();
+      return Iter;
     }
-    AdvancePastEmptyBuckets();
+    DenseMapIterator Iter(Buckets.begin(), Buckets.end(), Epoch);
+    Iter.AdvancePastEmptyBuckets();
+    return Iter;
+  }
+
+  static DenseMapIterator makeEnd(iterator_range<pointer> Buckets,
+                                  const DebugEpochBase &Epoch) {
+    if (shouldReverseIterate<KeyT>())
+      return DenseMapIterator(Buckets.begin(), Buckets.begin(), Epoch);
+    return DenseMapIterator(Buckets.end(), Buckets.end(), Epoch);
+  }
+
+  static DenseMapIterator makeIterator(pointer P,
+                                       iterator_range<pointer> Buckets,
+                                       const DebugEpochBase &Epoch) {
+    if (shouldReverseIterate<KeyT>())
+      return DenseMapIterator(P + 1, Buckets.begin(), Epoch);
+    return DenseMapIterator(P, Buckets.end(), Epoch);
   }
 
   // Converting ctor from non-const iterators to const iterators. SFINAE'd out

Copy link
Member

@MacDue MacDue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup 👍 Assuming there's no performance difference this LGTM

@kazutakahirata kazutakahirata merged commit f502bab into llvm:main Sep 1, 2025
11 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20250830_DenseMap_makeBegin branch September 1, 2025 15:05
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