[llvm] [ADT] Simplify DenseMapIterator with std::reverse_iterator (NFC) (PR #157389)
Kazu Hirata via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 8 00:03:10 PDT 2025
https://github.com/kazutakahirata created https://github.com/llvm/llvm-project/pull/157389
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.
>From 70b73aca35bad6aef5a0782d83ee3d9b88289427 Mon Sep 17 00:00:00 2001
From: Kazu Hirata <kazu at google.com>
Date: Wed, 3 Sep 2025 12:31:44 -0700
Subject: [PATCH] [ADT] Simplify DenseMapIterator with std::reverse_iterator
(NFC)
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/include/llvm/ADT/DenseMap.h | 55 +++++++++++++-------------------
1 file changed, 23 insertions(+), 32 deletions(-)
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;
}
};
More information about the llvm-commits
mailing list