[ADT] Simplify DenseMapIterator with std::reverse_iterator (NFC) #157389
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DenseMapIterator has two tasks: - iterate the buckets in the requested direction - skip the empty and tombstone buckets These tasks are intertwined in the current implementation. This patch cleans up DenseMapIterator by separating the two tasks. Specifically, we introduce a private middleman iterator type called BucketItTy. This is the same as the pointer-based iterator in the forward direction, but it becomes std::reverse_iterator<pointer> otherwise. Now, the user-facing iterator iterates over BucketItTy while skipping the empty and tombstone buckets. This way, AdvancePastEmptyBuckets always calls BucketItTy::operator++. If the reverse iteration is requested, the underlying raw pointer gets decremented, but that logic is hidden behind std::reverse_iterator<pointer>::operator++. As a result, we can remove RetreatPastEmptyBuckets and a couple of calls to shouldReverseIterate. Here is a data point. A couple of months ago, we were calling shouldReverseIterate from 18 places in DenseMap.h. That's down to 5. This patch reduces it further down to 3.
|
@llvm/pr-subscribers-llvm-adt Author: Kazu Hirata (kazutakahirata) ChangesDenseMapIterator has two tasks:
These tasks are intertwined in the current implementation. This patch cleans up DenseMapIterator by separating the two tasks. As a result, we can remove RetreatPastEmptyBuckets and a couple of Here is a data point. A couple of months ago, we were calling Full diff: https://github.com/llvm/llvm-project/pull/157389.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 848e76feb20c6..18dd7f30c5616 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -1184,10 +1184,14 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
using iterator_category = std::forward_iterator_tag;
private:
- pointer Ptr = nullptr;
- pointer End = nullptr;
+ using BucketItTy =
+ std::conditional_t<shouldReverseIterate<KeyT>(),
+ std::reverse_iterator<pointer>, pointer>;
- DenseMapIterator(pointer Pos, pointer E, const DebugEpochBase &Epoch)
+ BucketItTy Ptr = {};
+ BucketItTy End = {};
+
+ DenseMapIterator(BucketItTy Pos, BucketItTy E, const DebugEpochBase &Epoch)
: DebugEpochBase::HandleBase(&Epoch), Ptr(Pos), End(E) {
assert(isHandleInSync() && "invalid construction!");
}
@@ -1201,29 +1205,24 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
// empty buckets.
if (IsEmpty)
return makeEnd(Buckets, Epoch);
- if (shouldReverseIterate<KeyT>()) {
- DenseMapIterator Iter(Buckets.end(), Buckets.begin(), Epoch);
- Iter.RetreatPastEmptyBuckets();
- return Iter;
- }
- DenseMapIterator Iter(Buckets.begin(), Buckets.end(), Epoch);
+ auto R = maybeReverse(Buckets);
+ DenseMapIterator Iter(R.begin(), R.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);
+ auto R = maybeReverse(Buckets);
+ return DenseMapIterator(R.end(), R.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);
+ auto R = maybeReverse(Buckets);
+ constexpr int Offset = shouldReverseIterate<KeyT>() ? 1 : 0;
+ return DenseMapIterator(BucketItTy(P + Offset), R.end(), Epoch);
}
// Converting ctor from non-const iterators to const iterators. SFINAE'd out
@@ -1238,16 +1237,16 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
reference operator*() const {
assert(isHandleInSync() && "invalid iterator access!");
assert(Ptr != End && "dereferencing end() iterator");
- if (shouldReverseIterate<KeyT>())
- return Ptr[-1];
return *Ptr;
}
pointer operator->() const { return &operator*(); }
friend bool operator==(const DenseMapIterator &LHS,
const DenseMapIterator &RHS) {
- assert((!LHS.Ptr || LHS.isHandleInSync()) && "handle not in sync!");
- assert((!RHS.Ptr || RHS.isHandleInSync()) && "handle not in sync!");
+ assert((!LHS.getEpochAddress() || LHS.isHandleInSync()) &&
+ "handle not in sync!");
+ assert((!RHS.getEpochAddress() || RHS.isHandleInSync()) &&
+ "handle not in sync!");
assert(LHS.getEpochAddress() == RHS.getEpochAddress() &&
"comparing incomparable iterators!");
return LHS.Ptr == RHS.Ptr;
@@ -1261,11 +1260,6 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
inline DenseMapIterator &operator++() { // Preincrement
assert(isHandleInSync() && "invalid iterator access!");
assert(Ptr != End && "incrementing end() iterator");
- if (shouldReverseIterate<KeyT>()) {
- --Ptr;
- RetreatPastEmptyBuckets();
- return *this;
- }
++Ptr;
AdvancePastEmptyBuckets();
return *this;
@@ -1288,14 +1282,11 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
++Ptr;
}
- void RetreatPastEmptyBuckets() {
- assert(Ptr >= End);
- const KeyT Empty = KeyInfoT::getEmptyKey();
- const KeyT Tombstone = KeyInfoT::getTombstoneKey();
-
- while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
- KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
- --Ptr;
+ static auto maybeReverse(iterator_range<pointer> Range) {
+ if constexpr (shouldReverseIterate<KeyT>())
+ return reverse(Range);
+ else
+ return Range;
}
};
|
kuhar
reviewed
Sep 8, 2025
@kuhar I can only get noise while compiling preprocessed |
kuhar
approved these changes
Sep 10, 2025
kazutakahirata
deleted the
cleanup_20250907_ADT_DenseMapIterator_reverse_iterator
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/11414 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DenseMapIterator has two tasks:
These tasks are intertwined in the current implementation.
This patch cleans up DenseMapIterator by separating the two tasks.
Specifically, we introduce a private middleman iterator type called
BucketItTy. This is the same as the pointer-based iterator in the
forward direction, but it becomes std::reverse_iterator
otherwise. Now, the user-facing iterator iterates over BucketItTy
while skipping the empty and tombstone buckets. This way,
AdvancePastEmptyBuckets always calls BucketItTy::operator++. If the
reverse iteration is requested, the underlying raw pointer gets
decremented, but that logic is hidden behind
std::reverse_iterator::operator++.
As a result, we can remove RetreatPastEmptyBuckets and a couple of
calls to shouldReverseIterate.
Here is a data point. A couple of months ago, we were calling
shouldReverseIterate from 18 places in DenseMap.h. That's down to 5.
This patch reduces it further down to 3.