-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[ADT] Overhaul the DenseMapIterator creation logic (NFC) #156221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ADT] Overhaul the DenseMapIterator creation logic (NFC) #156221
Conversation
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/pr-subscribers-llvm-adt Author: Kazu Hirata (kazutakahirata) ChangesWithout this patch, it's overly complicated to create iterators in
This patch cleans up all this by creating factory methods:
With these:
Now, it's a lot simpler to call helper functions 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:
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
|
There was a problem hiding this 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
Without this patch, it's overly complicated to create iterators in
DenseMap.
is needed in begin().
This patch cleans up all this by creating factory methods:
With these:
valid bucket.
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.