[llvm] [DenseMap] Replace tombstone deletion with TAOCP 6.4 Algorithm R (PR #199615)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Thu May 28 07:29:37 PDT 2026
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/199615
>From b091e3b6bbbcdb7994ca5c9e53c86eee22319836 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Mon, 11 May 2026 23:37:58 -0700
Subject: [PATCH 1/4] [DenseMap] Replace tombstone deletion with TAOCP 6.4
Algorithm R
DenseMap uses quadratic probing with lazy deletion: an erased entry
becomes a tombstone, a third bucket state alongside empty and live that
every find/insert must inspect.
Switch to linear probing with backward-shift deletion (Knuth TAOCP 6.4
Algorithm R), similar to the SmallPtrSet change #197637. This removes
the tombstone state entirely.
In exchange, erase now relocates the following live entries to close the
hole, so it invalidates iterators and references other than the erased
one. For callers that cache pointers into the bucket array,
erase(Key, OnMoved) and erase(iterator, OnMoved) fire a callback once per
shifted bucket, so fix-ups cost O(cluster) rather than O(NumEntries).
ValueHandleBase::RemoveFromUseList uses this to refresh each moved
handle's PrevPtr.
Linear probing is more vulnerable to primary clustering than quadratic
probing, so this relies on the stronger DenseMapInfo<T*>::getHashValue
mixer from #197390.
Operation distribution when compiling CGExpr.cpp/ScalarEvolution.cpp:
62.8% lookups, 34.3% inserts, 2.9% erases. The heaviest DenseMap
specializations have pointer keys and 16-byte key/value pairs.
Alternatives such as Robin Hood hashing, Verstable, and Boost's
unordered_flat_map were evaluated; they are slower and have a larger
code footprint. I believe the current in-band sentinel value approach,
despite the pain (#146595), is the best, or at least very difficult to
beat.
Non-core cleanups aided by Claude Opus 4.7.
---
llvm/include/llvm/ADT/DenseMap.h | 202 +++++++++++----------------
llvm/lib/IR/Value.cpp | 4 +-
llvm/unittests/ADT/BitVectorTest.cpp | 3 +-
3 files changed, 88 insertions(+), 121 deletions(-)
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 081d7e198e94c..2a8bc8e6b08e9 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -120,7 +120,7 @@ class DenseMapBase : public DebugEpochBase {
void clear() {
incrementEpoch();
- if (getNumEntries() == 0 && getNumTombstones() == 0)
+ if (getNumEntries() == 0)
return;
// If the capacity of the array is huge, and the # elements used is small,
@@ -136,14 +136,11 @@ class DenseMapBase : public DebugEpochBase {
for (BucketT &B : buckets())
B.getFirst() = EmptyKey;
} else {
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
unsigned NumEntries = getNumEntries();
for (BucketT &B : buckets()) {
if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey)) {
- if (!KeyInfoT::isEqual(B.getFirst(), TombstoneKey)) {
- B.getSecond().~ValueT();
- --NumEntries;
- }
+ B.getSecond().~ValueT();
+ --NumEntries;
B.getFirst() = EmptyKey;
}
}
@@ -151,7 +148,6 @@ class DenseMapBase : public DebugEpochBase {
(void)NumEntries;
}
setNumEntries(0);
- setNumTombstones(0);
}
void shrink_and_clear() {
@@ -325,25 +321,67 @@ class DenseMapBase : public DebugEpochBase {
return Ret;
}
+ void eraseFromFilledBucket(BucketT *TheBucket) {
+ eraseFromFilledBucket(TheBucket, [](BucketT &) {});
+ }
+
+ /// Erase the entry at \p TheBucket and close the resulting hole via Knuth
+ /// TAOCP 6.4 Algorithm R. For callers that cache pointers into the bucket
+ /// array, call \p OnMoved per shifted bucket.
+ template <typename OnMovedT>
+ void eraseFromFilledBucket(BucketT *TheBucket, OnMovedT &&OnMoved) {
+ incrementEpoch();
+ TheBucket->getSecond().~ValueT();
+ decrementNumEntries();
+
+ BucketT *BucketsPtr = getBuckets();
+ const unsigned NumBuckets = getNumBuckets();
+ const unsigned Mask = NumBuckets - 1;
+ const KeyT EmptyKey = KeyInfoT::getEmptyKey();
+ unsigned I = static_cast<unsigned>(TheBucket - BucketsPtr);
+ unsigned J = I;
+ while (true) {
+ J = (J + 1) & Mask;
+ BucketT &BJ = BucketsPtr[J];
+ if (KeyInfoT::isEqual(BJ.getFirst(), EmptyKey))
+ break;
+ auto Ideal = KeyInfoT::getHashValue(BJ.getFirst());
+ // If the hole (I) lies on the linear-probe chain from the home bucket
+ // (Ideal) to J, shift J into the hole and make J the new hole.
+ if (((I - Ideal) & Mask) < ((J - Ideal) & Mask)) {
+ BucketT &BI = BucketsPtr[I];
+ BI.getFirst() = std::move(BJ.getFirst());
+ ::new (&BI.getSecond()) ValueT(std::move(BJ.getSecond()));
+ BJ.getSecond().~ValueT();
+ OnMoved(BI);
+ I = J;
+ }
+ }
+ BucketsPtr[I].getFirst() = EmptyKey;
+ }
+
bool erase(const KeyT &Val) {
BucketT *TheBucket = doFind(Val);
if (!TheBucket)
return false; // not in map.
- incrementEpoch();
- TheBucket->getSecond().~ValueT();
- TheBucket->getFirst() = KeyInfoT::getTombstoneKey();
- decrementNumEntries();
- incrementNumTombstones();
+ eraseFromFilledBucket(TheBucket);
return true;
}
- void erase(iterator I) {
- BucketT *TheBucket = &*I;
- incrementEpoch();
- TheBucket->getSecond().~ValueT();
- TheBucket->getFirst() = KeyInfoT::getTombstoneKey();
- decrementNumEntries();
- incrementNumTombstones();
+ void erase(iterator I) { eraseFromFilledBucket(&*I); }
+
+ /// Erase \p Val and close the resulting hole by potentially shifting other
+ /// entries into it. For callers that cache pointers into the bucket array,
+ /// call \p OnMoved per shifted bucket.
+ template <typename OnMovedT> bool erase(const KeyT &Val, OnMovedT &&OnMoved) {
+ BucketT *TheBucket = doFind(Val);
+ if (!TheBucket)
+ return false;
+ eraseFromFilledBucket(TheBucket, std::forward<OnMovedT>(OnMoved));
+ return true;
+ }
+ template <typename OnMovedT> void erase(iterator I, OnMovedT &&OnMoved) {
+ eraseFromFilledBucket(&*I, std::forward<OnMovedT>(OnMoved));
}
/// Remove entries that match the given predicate. \p Pred is invoked
@@ -354,22 +392,22 @@ class DenseMapBase : public DebugEpochBase {
/// into the map are invalidated.
template <typename Predicate> bool remove_if(Predicate Pred) {
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
+ unsigned NumBuckets = getNumBuckets();
bool Removed = false;
for (BucketT &B : buckets()) {
- if (KeyInfoT::isEqual(B.getFirst(), EmptyKey) ||
- KeyInfoT::isEqual(B.getFirst(), TombstoneKey))
+ if (KeyInfoT::isEqual(B.getFirst(), EmptyKey))
continue;
if (Pred(B)) {
B.getSecond().~ValueT();
- B.getFirst() = TombstoneKey;
+ B.getFirst() = EmptyKey;
decrementNumEntries();
- incrementNumTombstones();
Removed = true;
}
}
- if (Removed)
+ if (Removed) {
incrementEpoch();
+ this->grow(NumBuckets);
+ }
return Removed;
}
@@ -406,12 +444,10 @@ class DenseMapBase : public DebugEpochBase {
struct ExactBucketCount {};
void initWithExactBucketCount(unsigned NewNumBuckets) {
- if (derived().allocateBuckets(NewNumBuckets)) {
+ if (derived().allocateBuckets(NewNumBuckets))
initEmpty();
- } else {
+ else
setNumEntries(0);
- setNumTombstones(0);
- }
}
void destroyAll() {
@@ -425,10 +461,8 @@ class DenseMapBase : public DebugEpochBase {
return;
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
for (BucketT &B : buckets()) {
- if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(B.getFirst(), TombstoneKey))
+ if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey))
B.getSecond().~ValueT();
B.getFirst().~KeyT();
}
@@ -438,7 +472,6 @@ class DenseMapBase : public DebugEpochBase {
static_assert(std::is_base_of_v<DenseMapBase, DerivedT>,
"Must pass the derived type to this template!");
setNumEntries(0);
- setNumTombstones(0);
assert((getNumBuckets() & (getNumBuckets() - 1)) == 0 &&
"# initial buckets must be a power of two!");
@@ -463,10 +496,8 @@ class DenseMapBase : public DebugEpochBase {
void moveFrom(DerivedT &Other) {
// Insert all the old elements.
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
for (BucketT &B : Other.buckets()) {
- if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(B.getFirst(), TombstoneKey)) {
+ if (!KeyInfoT::isEqual(B.getFirst(), EmptyKey)) {
// Insert the key/value into the new table.
BucketT *DestBucket;
bool FoundVal = LookupBucketFor(B.getFirst(), DestBucket);
@@ -488,7 +519,6 @@ class DenseMapBase : public DebugEpochBase {
this->destroyAll();
derived().deallocateBuckets();
setNumEntries(0);
- setNumTombstones(0);
if (!derived().allocateBuckets(other.getNumBuckets())) {
// The bucket list is empty. No work to do.
return;
@@ -498,7 +528,6 @@ class DenseMapBase : public DebugEpochBase {
assert(getNumBuckets() == other.getNumBuckets());
setNumEntries(other.getNumEntries());
- setNumTombstones(other.getNumTombstones());
BucketT *Buckets = getBuckets();
const BucketT *OtherBuckets = other.getBuckets();
@@ -509,11 +538,9 @@ class DenseMapBase : public DebugEpochBase {
NumBuckets * sizeof(BucketT));
} else {
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
for (size_t I = 0; I < NumBuckets; ++I) {
::new (&Buckets[I].getFirst()) KeyT(OtherBuckets[I].getFirst());
- if (!KeyInfoT::isEqual(Buckets[I].getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(Buckets[I].getFirst(), TombstoneKey))
+ if (!KeyInfoT::isEqual(Buckets[I].getFirst(), EmptyKey))
::new (&Buckets[I].getSecond()) ValueT(OtherBuckets[I].getSecond());
}
}
@@ -562,14 +589,6 @@ class DenseMapBase : public DebugEpochBase {
void decrementNumEntries() { setNumEntries(getNumEntries() - 1); }
- unsigned getNumTombstones() const { return derived().getNumTombstones(); }
-
- void setNumTombstones(unsigned Num) { derived().setNumTombstones(Num); }
-
- void incrementNumTombstones() { setNumTombstones(getNumTombstones() + 1); }
-
- void decrementNumTombstones() { setNumTombstones(getNumTombstones() - 1); }
-
const BucketT *getBuckets() const { return derived().getBuckets(); }
BucketT *getBuckets() { return derived().getBuckets(); }
@@ -605,37 +624,21 @@ class DenseMapBase : public DebugEpochBase {
BucketT *TheBucket) {
incrementEpoch();
- // If the load of the hash table is more than 3/4, or if fewer than 1/8 of
- // the buckets are empty (meaning that many are filled with tombstones),
- // grow the table.
- //
- // The later case is tricky. For example, if we had one empty bucket with
- // tons of tombstones, failing lookups (e.g. for insertion) would have to
- // probe almost the entire table until it found the empty bucket. If the
- // table completely filled with tombstones, no lookup would ever succeed,
- // causing infinite loops in lookup.
+ // Grow the table if the load factor would exceed 3/4 after insertion.
+ // Linear probing with gap-closing deletion (Knuth Algorithm R) keeps
+ // every chain compact and bounded by the table's empty-bucket count,
+ // so no tombstone-driven resize is needed.
unsigned NewNumEntries = getNumEntries() + 1;
unsigned NumBuckets = getNumBuckets();
if (LLVM_UNLIKELY(NewNumEntries * 4 >= NumBuckets * 3)) {
this->grow(NumBuckets * 2);
LookupBucketFor(Lookup, TheBucket);
- } else if (LLVM_UNLIKELY(NumBuckets -
- (NewNumEntries + getNumTombstones()) <=
- NumBuckets / 8)) {
- this->grow(NumBuckets);
- LookupBucketFor(Lookup, TheBucket);
}
assert(TheBucket);
// Only update the state after we've grown our bucket space appropriately
// so that when growing buckets we have self-consistent entry count.
incrementNumEntries();
-
- // If we are writing over a tombstone, remember this.
- const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- if (!KeyInfoT::isEqual(TheBucket->getFirst(), EmptyKey))
- decrementNumTombstones();
-
return TheBucket;
}
@@ -648,7 +651,6 @@ class DenseMapBase : public DebugEpochBase {
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
unsigned BucketNo = KeyInfoT::getHashValue(Val) & (NumBuckets - 1);
- unsigned ProbeAmt = 1;
while (true) {
const BucketT *Bucket = BucketsPtr + BucketNo;
if (LLVM_LIKELY(KeyInfoT::isEqual(Val, Bucket->getFirst())))
@@ -656,10 +658,8 @@ class DenseMapBase : public DebugEpochBase {
if (LLVM_LIKELY(KeyInfoT::isEqual(Bucket->getFirst(), EmptyKey)))
return nullptr;
- // Otherwise, it's a hash collision or a tombstone, continue quadratic
- // probing.
- BucketNo += ProbeAmt++;
- BucketNo &= NumBuckets - 1;
+ // Hash collision: continue linear probing.
+ BucketNo = (BucketNo + 1) & (NumBuckets - 1);
}
}
@@ -670,7 +670,7 @@ class DenseMapBase : public DebugEpochBase {
/// Lookup the appropriate bucket for Val, returning it in FoundBucket. If the
/// bucket contains the key and a value, this returns true, otherwise it
- /// returns a bucket with an empty marker or tombstone and returns false.
+ /// returns a bucket with an empty marker and returns false.
template <typename LookupKeyT>
bool LookupBucketFor(const LookupKeyT &Val, BucketT *&FoundBucket) {
BucketT *BucketsPtr = getBuckets();
@@ -681,16 +681,11 @@ class DenseMapBase : public DebugEpochBase {
return false;
}
- // FoundTombstone - Keep track of whether we find a tombstone while probing.
- BucketT *FoundTombstone = nullptr;
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
assert(!KeyInfoT::isEqual(Val, EmptyKey) &&
- !KeyInfoT::isEqual(Val, TombstoneKey) &&
- "Empty/Tombstone value shouldn't be inserted into map!");
+ "Empty value shouldn't be inserted into map!");
unsigned BucketNo = KeyInfoT::getHashValue(Val) & (NumBuckets - 1);
- unsigned ProbeAmt = 1;
while (true) {
BucketT *ThisBucket = BucketsPtr + BucketNo;
// Found Val's bucket? If so, return it.
@@ -700,24 +695,14 @@ class DenseMapBase : public DebugEpochBase {
}
// If we found an empty bucket, the key doesn't exist in the set.
- // Insert it and return the default value.
+ // Return it as the insertion point.
if (LLVM_LIKELY(KeyInfoT::isEqual(ThisBucket->getFirst(), EmptyKey))) {
- // If we've already seen a tombstone while probing, fill it in instead
- // of the empty bucket we eventually probed to.
- FoundBucket = FoundTombstone ? FoundTombstone : ThisBucket;
+ FoundBucket = ThisBucket;
return false;
}
- // If this is a tombstone, remember it. If Val ends up not in the map, we
- // prefer to return it than something that would require more probing.
- if (KeyInfoT::isEqual(ThisBucket->getFirst(), TombstoneKey) &&
- !FoundTombstone)
- FoundTombstone = ThisBucket; // Remember the first tombstone found.
-
- // Otherwise, it's a hash collision or a tombstone, continue quadratic
- // probing.
- BucketNo += ProbeAmt++;
- BucketNo &= (NumBuckets - 1);
+ // Hash collision: continue linear probing.
+ BucketNo = (BucketNo + 1) & (NumBuckets - 1);
}
}
@@ -778,7 +763,6 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
BucketT *Buckets = nullptr;
unsigned NumEntries = 0;
- unsigned NumTombstones = 0;
unsigned NumBuckets = 0;
explicit DenseMap(unsigned NumBuckets, typename BaseT::ExactBucketCount) {
@@ -831,7 +815,6 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
void swapImpl(DenseMap &RHS) {
std::swap(Buckets, RHS.Buckets);
std::swap(NumEntries, RHS.NumEntries);
- std::swap(NumTombstones, RHS.NumTombstones);
std::swap(NumBuckets, RHS.NumBuckets);
}
@@ -839,10 +822,6 @@ class DenseMap : public DenseMapBase<DenseMap<KeyT, ValueT, KeyInfoT, BucketT>,
void setNumEntries(unsigned Num) { NumEntries = Num; }
- unsigned getNumTombstones() const { return NumTombstones; }
-
- void setNumTombstones(unsigned Num) { NumTombstones = Num; }
-
BucketT *getBuckets() const { return Buckets; }
unsigned getNumBuckets() const { return NumBuckets; }
@@ -911,7 +890,6 @@ class SmallDenseMap
unsigned Small : 1;
unsigned NumEntries : 31;
- unsigned NumTombstones;
struct LargeRep {
BucketT *Buckets;
@@ -978,10 +956,8 @@ class SmallDenseMap
unsigned TmpNumEntries = RHS.NumEntries;
RHS.NumEntries = NumEntries;
NumEntries = TmpNumEntries;
- std::swap(NumTombstones, RHS.NumTombstones);
const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- const KeyT TombstoneKey = KeyInfoT::getTombstoneKey();
if (Small && RHS.Small) {
// If we're swapping inline bucket arrays, we have to cope with some of
// the tricky bits of DenseMap's storage system: the buckets are not
@@ -990,10 +966,8 @@ class SmallDenseMap
for (unsigned i = 0, e = InlineBuckets; i != e; ++i) {
BucketT *LHSB = &getInlineBuckets()[i],
*RHSB = &RHS.getInlineBuckets()[i];
- bool hasLHSValue = (!KeyInfoT::isEqual(LHSB->getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(LHSB->getFirst(), TombstoneKey));
- bool hasRHSValue = (!KeyInfoT::isEqual(RHSB->getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(RHSB->getFirst(), TombstoneKey));
+ bool hasLHSValue = !KeyInfoT::isEqual(LHSB->getFirst(), EmptyKey);
+ bool hasRHSValue = !KeyInfoT::isEqual(RHSB->getFirst(), EmptyKey);
if (hasLHSValue && hasRHSValue) {
// Swap together if we can...
std::swap(*LHSB, *RHSB);
@@ -1032,8 +1006,7 @@ class SmallDenseMap
*OldB = &SmallSide.getInlineBuckets()[i];
::new (&NewB->getFirst()) KeyT(std::move(OldB->getFirst()));
OldB->getFirst().~KeyT();
- if (!KeyInfoT::isEqual(NewB->getFirst(), EmptyKey) &&
- !KeyInfoT::isEqual(NewB->getFirst(), TombstoneKey)) {
+ if (!KeyInfoT::isEqual(NewB->getFirst(), EmptyKey)) {
::new (&NewB->getSecond()) ValueT(std::move(OldB->getSecond()));
OldB->getSecond().~ValueT();
}
@@ -1053,10 +1026,6 @@ class SmallDenseMap
NumEntries = Num;
}
- unsigned getNumTombstones() const { return NumTombstones; }
-
- void setNumTombstones(unsigned Num) { NumTombstones = Num; }
-
const BucketT *getInlineBuckets() const {
assert(Small);
// Note that this cast does not violate aliasing rules as we assert that
@@ -1144,7 +1113,6 @@ class SmallDenseMap
Small = false;
NumEntries = Other.NumEntries;
- NumTombstones = Other.NumTombstones;
*getLargeRep() = std::move(*Other.getLargeRep());
Other.getLargeRep()->NumBuckets = 0;
return true;
@@ -1273,10 +1241,8 @@ class DenseMapIterator : DebugEpochBase::HandleBase {
void AdvancePastEmptyBuckets() {
assert(Ptr <= End);
const KeyT Empty = KeyInfoT::getEmptyKey();
- const KeyT Tombstone = KeyInfoT::getTombstoneKey();
- while (Ptr != End && (KeyInfoT::isEqual(Ptr->getFirst(), Empty) ||
- KeyInfoT::isEqual(Ptr->getFirst(), Tombstone)))
+ while (Ptr != End && KeyInfoT::isEqual(Ptr->getFirst(), Empty))
++Ptr;
}
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 7246adf7ec651..a1991e444ac29 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -1223,7 +1223,9 @@ void ValueHandleBase::RemoveFromUseList() {
LLVMContextImpl *pImpl = getValPtr()->getContext().pImpl;
DenseMap<Value*, ValueHandleBase*> &Handles = pImpl->ValueHandles;
if (Handles.isPointerIntoBucketsArray(PrevPtr)) {
- Handles.erase(getValPtr());
+ Handles.erase(getValPtr(), [](auto &Bucket) {
+ Bucket.second->setPrevPtr(&Bucket.second);
+ });
getValPtr()->HasValueHandle = false;
}
}
diff --git a/llvm/unittests/ADT/BitVectorTest.cpp b/llvm/unittests/ADT/BitVectorTest.cpp
index d3200b7722ee3..adf788b8f9661 100644
--- a/llvm/unittests/ADT/BitVectorTest.cpp
+++ b/llvm/unittests/ADT/BitVectorTest.cpp
@@ -1434,8 +1434,7 @@ TYPED_TEST(BitVectorTest, DenseSet) {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
TypeParam D;
- EXPECT_DEATH(Set.insert(D),
- "Empty/Tombstone value shouldn't be inserted into map!");
+ EXPECT_DEATH(Set.insert(D), "Empty value shouldn't be inserted into map!");
#endif
EXPECT_EQ(3U, Set.size());
>From 87e6df681fa4e841baf7805566105a8efd759529 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Wed, 27 May 2026 18:59:55 -0700
Subject: [PATCH 2/4] add TODO
---
llvm/include/llvm/ADT/DenseMap.h | 104 ++++++++++++++++---------------
llvm/lib/IR/Value.cpp | 1 +
2 files changed, 56 insertions(+), 49 deletions(-)
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 2a8bc8e6b08e9..b52906a215a7c 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -53,6 +53,10 @@ struct DenseMapPair : std::pair<KeyT, ValueT> {
} // end namespace detail
+// Befriended below so DenseMapBase can expose its bucket-relocation callback
+// erase to ValueHandleBase, the only caller that caches bucket pointers.
+class ValueHandleBase;
+
template <typename KeyT, typename ValueT,
typename KeyInfoT = DenseMapInfo<KeyT>,
typename Bucket = llvm::detail::DenseMapPair<KeyT, ValueT>,
@@ -325,41 +329,6 @@ class DenseMapBase : public DebugEpochBase {
eraseFromFilledBucket(TheBucket, [](BucketT &) {});
}
- /// Erase the entry at \p TheBucket and close the resulting hole via Knuth
- /// TAOCP 6.4 Algorithm R. For callers that cache pointers into the bucket
- /// array, call \p OnMoved per shifted bucket.
- template <typename OnMovedT>
- void eraseFromFilledBucket(BucketT *TheBucket, OnMovedT &&OnMoved) {
- incrementEpoch();
- TheBucket->getSecond().~ValueT();
- decrementNumEntries();
-
- BucketT *BucketsPtr = getBuckets();
- const unsigned NumBuckets = getNumBuckets();
- const unsigned Mask = NumBuckets - 1;
- const KeyT EmptyKey = KeyInfoT::getEmptyKey();
- unsigned I = static_cast<unsigned>(TheBucket - BucketsPtr);
- unsigned J = I;
- while (true) {
- J = (J + 1) & Mask;
- BucketT &BJ = BucketsPtr[J];
- if (KeyInfoT::isEqual(BJ.getFirst(), EmptyKey))
- break;
- auto Ideal = KeyInfoT::getHashValue(BJ.getFirst());
- // If the hole (I) lies on the linear-probe chain from the home bucket
- // (Ideal) to J, shift J into the hole and make J the new hole.
- if (((I - Ideal) & Mask) < ((J - Ideal) & Mask)) {
- BucketT &BI = BucketsPtr[I];
- BI.getFirst() = std::move(BJ.getFirst());
- ::new (&BI.getSecond()) ValueT(std::move(BJ.getSecond()));
- BJ.getSecond().~ValueT();
- OnMoved(BI);
- I = J;
- }
- }
- BucketsPtr[I].getFirst() = EmptyKey;
- }
-
bool erase(const KeyT &Val) {
BucketT *TheBucket = doFind(Val);
if (!TheBucket)
@@ -370,20 +339,6 @@ class DenseMapBase : public DebugEpochBase {
}
void erase(iterator I) { eraseFromFilledBucket(&*I); }
- /// Erase \p Val and close the resulting hole by potentially shifting other
- /// entries into it. For callers that cache pointers into the bucket array,
- /// call \p OnMoved per shifted bucket.
- template <typename OnMovedT> bool erase(const KeyT &Val, OnMovedT &&OnMoved) {
- BucketT *TheBucket = doFind(Val);
- if (!TheBucket)
- return false;
- eraseFromFilledBucket(TheBucket, std::forward<OnMovedT>(OnMoved));
- return true;
- }
- template <typename OnMovedT> void erase(iterator I, OnMovedT &&OnMoved) {
- eraseFromFilledBucket(&*I, std::forward<OnMovedT>(OnMoved));
- }
-
/// Remove entries that match the given predicate. \p Pred is invoked
/// with a reference to each live bucket and must not access the map being
/// modified. This is the safe replacement for erase-while-iterating.
@@ -547,6 +502,57 @@ class DenseMapBase : public DebugEpochBase {
}
private:
+ // ValueHandleBase caches pointers into the bucket array, so it needs the
+ // callback erase below to fix them up as entries shift. It is the only
+ // intended caller; do not add new ones.
+ friend class ValueHandleBase;
+
+ /// Erase the entry at \p TheBucket and close the resulting hole via Knuth
+ /// TAOCP 6.4 Algorithm R. For callers that cache pointers into the bucket
+ /// array, call \p OnMoved per shifted bucket.
+ template <typename OnMovedT>
+ void eraseFromFilledBucket(BucketT *TheBucket, OnMovedT &&OnMoved) {
+ incrementEpoch();
+ TheBucket->getSecond().~ValueT();
+ decrementNumEntries();
+
+ BucketT *BucketsPtr = getBuckets();
+ const unsigned NumBuckets = getNumBuckets();
+ const unsigned Mask = NumBuckets - 1;
+ const KeyT EmptyKey = KeyInfoT::getEmptyKey();
+ unsigned I = static_cast<unsigned>(TheBucket - BucketsPtr);
+ unsigned J = I;
+ while (true) {
+ J = (J + 1) & Mask;
+ BucketT &BJ = BucketsPtr[J];
+ if (KeyInfoT::isEqual(BJ.getFirst(), EmptyKey))
+ break;
+ auto Ideal = KeyInfoT::getHashValue(BJ.getFirst());
+ // If the hole (I) lies on the linear-probe chain from the home bucket
+ // (Ideal) to J, shift J into the hole and make J the new hole.
+ if (((I - Ideal) & Mask) < ((J - Ideal) & Mask)) {
+ BucketT &BI = BucketsPtr[I];
+ BI.getFirst() = std::move(BJ.getFirst());
+ ::new (&BI.getSecond()) ValueT(std::move(BJ.getSecond()));
+ BJ.getSecond().~ValueT();
+ OnMoved(BI);
+ I = J;
+ }
+ }
+ BucketsPtr[I].getFirst() = EmptyKey;
+ }
+
+ /// Erase \p Val and close the resulting hole by potentially shifting other
+ /// entries into it. For callers that cache pointers into the bucket array,
+ /// call \p OnMoved per shifted bucket.
+ template <typename OnMovedT> bool erase(const KeyT &Val, OnMovedT &&OnMoved) {
+ BucketT *TheBucket = doFind(Val);
+ if (!TheBucket)
+ return false;
+ eraseFromFilledBucket(TheBucket, std::forward<OnMovedT>(OnMoved));
+ return true;
+ }
+
DerivedT &derived() { return *static_cast<DerivedT *>(this); }
const DerivedT &derived() const {
return *static_cast<const DerivedT *>(this);
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index a1991e444ac29..673eddb4586fd 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -1223,6 +1223,7 @@ void ValueHandleBase::RemoveFromUseList() {
LLVMContextImpl *pImpl = getValPtr()->getContext().pImpl;
DenseMap<Value*, ValueHandleBase*> &Handles = pImpl->ValueHandles;
if (Handles.isPointerIntoBucketsArray(PrevPtr)) {
+ // TODO: Remove the only user of DenseMap's callback erase.
Handles.erase(getValPtr(), [](auto &Bucket) {
Bucket.second->setPrevPtr(&Bucket.second);
});
>From d79fc59c8939a58b2b4876ee315099478780f656 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Wed, 27 May 2026 22:35:08 -0700
Subject: [PATCH 3/4] Fix IR/Constants.cpp
---
llvm/lib/IR/Constants.cpp | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index a680b9791c75e..9670dd39d2ec2 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -2111,16 +2111,14 @@ Value *BlockAddress::handleOperandChangeImpl(Value *From, Value *To) {
// See if the 'new' entry already exists, if not, just update this in place
// and return early.
- BlockAddress *&NewBA = getContext().pImpl->BlockAddresses[NewBB];
- if (NewBA)
+ if (BlockAddress *NewBA = getContext().pImpl->BlockAddresses.lookup(NewBB))
return NewBA;
getBasicBlock()->setHasAddressTaken(false);
- // Remove the old entry, this can't cause the map to rehash (just a
- // tombstone will get added).
+ // Erase before insert; backward-shift deletion may relocate buckets.
getContext().pImpl->BlockAddresses.erase(getBasicBlock());
- NewBA = this;
+ getContext().pImpl->BlockAddresses[NewBB] = this;
setOperand(0, NewBB);
getBasicBlock()->setHasAddressTaken(true);
@@ -2156,9 +2154,8 @@ Value *DSOLocalEquivalent::handleOperandChangeImpl(Value *From, Value *To) {
// The replacement is with another global value.
if (const auto *ToObj = dyn_cast<GlobalValue>(To)) {
- DSOLocalEquivalent *&NewEquiv =
- getContext().pImpl->DSOLocalEquivalents[ToObj];
- if (NewEquiv)
+ if (DSOLocalEquivalent *NewEquiv =
+ getContext().pImpl->DSOLocalEquivalents.lookup(ToObj))
return llvm::ConstantExpr::getBitCast(NewEquiv, getType());
}
@@ -2170,13 +2167,13 @@ Value *DSOLocalEquivalent::handleOperandChangeImpl(Value *From, Value *To) {
// The replacement could be a bitcast or an alias to another function. We can
// replace it with a bitcast to the dso_local_equivalent of that function.
auto *Func = cast<Function>(To->stripPointerCastsAndAliases());
- DSOLocalEquivalent *&NewEquiv = getContext().pImpl->DSOLocalEquivalents[Func];
- if (NewEquiv)
+ if (DSOLocalEquivalent *NewEquiv =
+ getContext().pImpl->DSOLocalEquivalents.lookup(Func))
return llvm::ConstantExpr::getBitCast(NewEquiv, getType());
- // Replace this with the new one.
+ // Erase before insert; backward-shift deletion may relocate buckets.
getContext().pImpl->DSOLocalEquivalents.erase(getGlobalValue());
- NewEquiv = this;
+ getContext().pImpl->DSOLocalEquivalents[Func] = this;
setOperand(0, Func);
if (Func->getType() != getType()) {
@@ -2214,12 +2211,12 @@ Value *NoCFIValue::handleOperandChangeImpl(Value *From, Value *To) {
GlobalValue *GV = dyn_cast<GlobalValue>(To->stripPointerCasts());
assert(GV && "Can only replace the operands with a global value");
- NoCFIValue *&NewNC = getContext().pImpl->NoCFIValues[GV];
- if (NewNC)
+ if (NoCFIValue *NewNC = getContext().pImpl->NoCFIValues.lookup(GV))
return llvm::ConstantExpr::getBitCast(NewNC, getType());
+ // Erase before insert; backward-shift deletion may relocate buckets.
getContext().pImpl->NoCFIValues.erase(getGlobalValue());
- NewNC = this;
+ getContext().pImpl->NoCFIValues[GV] = this;
setOperand(0, GV);
if (GV->getType() != getType())
>From db7270335e0726929726bd9fdcd5327ad0ebf182 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Thu, 28 May 2026 06:14:44 -0700
Subject: [PATCH 4/4] simplify comments in Constants.cpp
---
llvm/lib/IR/Constants.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index 9670dd39d2ec2..9fa7863e4bbb5 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -2116,7 +2116,7 @@ Value *BlockAddress::handleOperandChangeImpl(Value *From, Value *To) {
getBasicBlock()->setHasAddressTaken(false);
- // Erase before insert; backward-shift deletion may relocate buckets.
+ // erase invalidates iterators/references, hence the duplicate NewBB lookup.
getContext().pImpl->BlockAddresses.erase(getBasicBlock());
getContext().pImpl->BlockAddresses[NewBB] = this;
setOperand(0, NewBB);
@@ -2171,7 +2171,7 @@ Value *DSOLocalEquivalent::handleOperandChangeImpl(Value *From, Value *To) {
getContext().pImpl->DSOLocalEquivalents.lookup(Func))
return llvm::ConstantExpr::getBitCast(NewEquiv, getType());
- // Erase before insert; backward-shift deletion may relocate buckets.
+ // erase invalidates iterators/references, hence the duplicate Func lookup.
getContext().pImpl->DSOLocalEquivalents.erase(getGlobalValue());
getContext().pImpl->DSOLocalEquivalents[Func] = this;
setOperand(0, Func);
@@ -2214,7 +2214,7 @@ Value *NoCFIValue::handleOperandChangeImpl(Value *From, Value *To) {
if (NoCFIValue *NewNC = getContext().pImpl->NoCFIValues.lookup(GV))
return llvm::ConstantExpr::getBitCast(NewNC, getType());
- // Erase before insert; backward-shift deletion may relocate buckets.
+ // erase invalidates iterators/references, hence the duplicate GV lookup.
getContext().pImpl->NoCFIValues.erase(getGlobalValue());
getContext().pImpl->NoCFIValues[GV] = this;
setOperand(0, GV);
More information about the llvm-commits
mailing list