From 4737b538a5ee7fa530df996354d06b555886010b Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Sat, 30 Aug 2025 16:43:02 -0700 Subject: [PATCH] [ADT] Overhaul the DenseMapIterator creation logic (NFC) 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. --- llvm/include/llvm/ADT/DenseMap.h | 106 ++++++++++++++----------------- 1 file changed, 49 insertions(+), 57 deletions(-) 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; inline iterator begin() { - // When the map is empty, avoid the overhead of advancing/retreating past - // empty buckets. - if (empty()) - return end(); - if (shouldReverseIterate()) - 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()) - 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 iterator find_as(const LookupKeyT &Val) { if (BucketT *Bucket = doFind(Val)) - return makeIterator( - Bucket, shouldReverseIterate() ? getBuckets() : getBucketsEnd(), - *this, true); + return makeIterator(Bucket); return end(); } template const_iterator find_as(const LookupKeyT &Val) const { if (const BucketT *Bucket = doFind(Val)) - return makeConstIterator( - Bucket, shouldReverseIterate() ? 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 try_emplace_impl(KeyArgT &&Key, Ts &&...Args) { auto [Bucket, Inserted] = lookupOrInsertIntoBucket( std::forward(Key), std::forward(Args)...); - return {makeInsertIterator(Bucket), Inserted}; + return {makeIterator(Bucket), Inserted}; } - iterator makeIterator(BucketT *P, BucketT *E, DebugEpochBase &Epoch, - bool NoAdvance = false) { - if (shouldReverseIterate()) { - 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()) { - 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() ? 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 buckets() const { + return llvm::make_range(getBuckets(), getBucketsEnd()); + } + void grow(unsigned AtLeast) { static_cast(this)->grow(AtLeast); } void shrink_and_clear() { static_cast(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 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()) { - 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 Buckets, + const DebugEpochBase &Epoch) { + if (shouldReverseIterate()) + return DenseMapIterator(Buckets.begin(), Buckets.begin(), Epoch); + return DenseMapIterator(Buckets.end(), Buckets.end(), Epoch); + } + + static DenseMapIterator makeIterator(pointer P, + iterator_range Buckets, + const DebugEpochBase &Epoch) { + if (shouldReverseIterate()) + 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