[llvm] Revert "[StringMap] Invalidate iterators in remove() (PR #203003)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 10 07:57:25 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-adt
Author: Adrian Prantl (adrian-prantl)
<details>
<summary>Changes</summary>
This reverts commit https://github.com/llvm/llvm-project/commit/bccd1b9cb744e5dd96ee59baa4bf4583457feea3.
This breaks the LLDB bots:
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/22124/
---
Patch is 21.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/203003.diff
6 Files Affected:
- (modified) llvm/docs/ProgrammersManual.rst (+2-2)
- (modified) llvm/include/llvm/ADT/StringMap.h (+48-35)
- (modified) llvm/lib/Support/StringMap.cpp (+50-28)
- (modified) llvm/lib/Transforms/IPO/StripSymbols.cpp (+7-10)
- (modified) llvm/unittests/ADT/StringMapTest.cpp (-88)
- (modified) llvm/utils/gdb-scripts/prettyprinters.py (+2-1)
``````````diff
diff --git a/llvm/docs/ProgrammersManual.rst b/llvm/docs/ProgrammersManual.rst
index 26a6c13edd094..6ee4badf6286b 100644
--- a/llvm/docs/ProgrammersManual.rst
+++ b/llvm/docs/ProgrammersManual.rst
@@ -2375,7 +2375,7 @@ long, expensive to copy, etc. ``StringMap`` is a specialized container designed
cope with these issues. It supports mapping an arbitrary range of bytes to an
arbitrary other object.
-The ``StringMap`` implementation uses a linear-probed hash table, where the
+The ``StringMap`` implementation uses a quadratically-probed hash table, where the
buckets store a pointer to the heap allocated entries (and some other stuff).
The entries in the map must be heap allocated because the strings are variable
length. The string data (key) and the element object (value) are stored in the
@@ -2383,7 +2383,7 @@ same allocation with the string data immediately after the element object.
This container guarantees the "``(char*)(&Value+1)``" points to the key string
for a value.
-The ``StringMap`` is very fast for several reasons: linear probing is very cache
+The ``StringMap`` is very fast for several reasons: quadratic probing is very cache
efficient for lookups, the hash value of strings in buckets is not recomputed
when looking up an element, ``StringMap`` rarely has to touch the memory for
unrelated objects when looking up a value (even when hash collisions happen),
diff --git a/llvm/include/llvm/ADT/StringMap.h b/llvm/include/llvm/ADT/StringMap.h
index af96b397eb969..5ce35f5c7a6ce 100644
--- a/llvm/include/llvm/ADT/StringMap.h
+++ b/llvm/include/llvm/ADT/StringMap.h
@@ -39,16 +39,19 @@ class StringMapImpl : public DebugEpochBase {
StringMapEntryBase **TheTable = nullptr;
unsigned NumBuckets = 0;
unsigned NumItems = 0;
+ unsigned NumTombstones = 0;
unsigned ItemSize;
protected:
explicit StringMapImpl(unsigned itemSize) : ItemSize(itemSize) {}
StringMapImpl(StringMapImpl &&RHS)
: TheTable(RHS.TheTable), NumBuckets(RHS.NumBuckets),
- NumItems(RHS.NumItems), ItemSize(RHS.ItemSize) {
+ NumItems(RHS.NumItems), NumTombstones(RHS.NumTombstones),
+ ItemSize(RHS.ItemSize) {
RHS.TheTable = nullptr;
RHS.NumBuckets = 0;
RHS.NumItems = 0;
+ RHS.NumTombstones = 0;
}
LLVM_ABI StringMapImpl(unsigned InitSize, unsigned ItemSize);
@@ -83,10 +86,6 @@ class StringMapImpl : public DebugEpochBase {
/// table, returning it. If the key is not in the table, this returns null.
LLVM_ABI StringMapEntryBase *RemoveKey(StringRef Key);
- /// Remove the entry at the given (live) bucket, whose value the caller has
- /// already destroyed, and close the hole via Algorithm R backward shifting.
- LLVM_ABI void removeBucket(unsigned Bucket);
-
/// Allocate the table with the specified number of buckets and otherwise
/// setup the map as empty.
LLVM_ABI void init(unsigned Size);
@@ -96,6 +95,14 @@ class StringMapImpl : public DebugEpochBase {
}
public:
+ static constexpr uintptr_t TombstoneIntVal =
+ static_cast<uintptr_t>(-1)
+ << PointerLikeTypeTraits<StringMapEntryBase *>::NumLowBitsAvailable;
+
+ static StringMapEntryBase *getTombstoneVal() {
+ return reinterpret_cast<StringMapEntryBase *>(TombstoneIntVal);
+ }
+
[[nodiscard]] unsigned getNumBuckets() const { return NumBuckets; }
[[nodiscard]] unsigned getNumItems() const { return NumItems; }
@@ -115,6 +122,7 @@ class StringMapImpl : public DebugEpochBase {
std::swap(TheTable, Other.TheTable);
std::swap(NumBuckets, Other.NumBuckets);
std::swap(NumItems, Other.NumItems);
+ std::swap(NumTombstones, Other.NumTombstones);
}
};
@@ -164,19 +172,26 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
*RHSHashTable = (unsigned *)(RHS.TheTable + NumBuckets + 1);
NumItems = RHS.NumItems;
- // Copy the bucket layout verbatim. Because RHS was built with linear
- // probing, preserving each entry's slot keeps the probe-sequence invariant
- // intact without re-probing.
+ NumTombstones = RHS.NumTombstones;
for (unsigned I = 0, E = NumBuckets; I != E; ++I) {
StringMapEntryBase *Bucket = RHS.TheTable[I];
- if (!Bucket)
+ if (!Bucket || Bucket == getTombstoneVal()) {
+ TheTable[I] = Bucket;
continue;
+ }
TheTable[I] = MapEntryTy::create(
static_cast<MapEntryTy *>(Bucket)->getKey(), getAllocator(),
static_cast<MapEntryTy *>(Bucket)->getValue());
HashTable[I] = RHSHashTable[I];
}
+
+ // Note that here we've copied everything from the RHS into this object,
+ // tombstones included. We could, instead, have re-probed for each key to
+ // instantiate this new object without any tombstone buckets. The
+ // assumption here is that items are rarely deleted from most StringMaps,
+ // and so tombstones are rare, so the cost of re-probing for all inputs is
+ // not worthwhile.
}
StringMap &operator=(StringMap RHS) {
@@ -191,7 +206,7 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
// work not required in the destructor.
if (!empty()) {
for (StringMapEntryBase *Bucket : buckets()) {
- if (Bucket) {
+ if (Bucket && Bucket != getTombstoneVal()) {
static_cast<MapEntryTy *>(Bucket)->Destroy(getAllocator());
}
}
@@ -311,13 +326,15 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
bool insert(MapEntryTy *KeyValue) {
unsigned BucketNo = LookupBucketFor(KeyValue->getKey());
StringMapEntryBase *&Bucket = TheTable[BucketNo];
- if (Bucket)
+ if (Bucket && Bucket != getTombstoneVal())
return false; // Already exists in map.
incrementEpoch();
+ if (Bucket == getTombstoneVal())
+ --NumTombstones;
Bucket = KeyValue;
++NumItems;
- assert(NumItems <= NumBuckets);
+ assert(NumItems + NumTombstones <= NumBuckets);
RehashTable();
return true;
@@ -377,14 +394,16 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
ArgsTy &&...Args) {
unsigned BucketNo = LookupBucketFor(Key, FullHashValue);
StringMapEntryBase *&Bucket = TheTable[BucketNo];
- if (Bucket)
+ if (Bucket && Bucket != getTombstoneVal())
return {iterator(this, TheTable + BucketNo), false}; // Already in map.
incrementEpoch();
+ if (Bucket == getTombstoneVal())
+ --NumTombstones;
Bucket =
MapEntryTy::create(Key, getAllocator(), std::forward<ArgsTy>(Args)...);
++NumItems;
- assert(NumItems <= NumBuckets);
+ assert(NumItems + NumTombstones <= NumBuckets);
BucketNo = RehashTable(BucketNo);
return {iterator(this, TheTable + BucketNo), true};
@@ -396,27 +415,26 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
if (empty())
return;
- // Zap all values, resetting the keys back to non-present, which is safe
- // because we're removing all elements.
+ // Zap all values, resetting the keys back to non-present (not tombstone),
+ // which is safe because we're removing all elements.
for (StringMapEntryBase *&Bucket : buckets()) {
- if (Bucket) {
+ if (Bucket && Bucket != getTombstoneVal()) {
static_cast<MapEntryTy *>(Bucket)->Destroy(getAllocator());
}
Bucket = nullptr;
}
NumItems = 0;
+ NumTombstones = 0;
}
/// remove - Remove the specified key/value pair from the map, but do not
/// erase it. This aborts if the key is not in the map.
- void remove(MapEntryTy *KeyValue) {
- incrementEpoch();
- RemoveKey(KeyValue);
- }
+ void remove(MapEntryTy *KeyValue) { RemoveKey(KeyValue); }
void erase(iterator I) {
MapEntryTy &V = *I;
+ incrementEpoch();
remove(&V);
V.Destroy(getAllocator());
}
@@ -437,22 +455,17 @@ class LLVM_ALLOCATORHOLDER_EMPTYBASE StringMap
/// into the map are invalidated.
template <typename Predicate> bool remove_if(Predicate Pred) {
bool Removed = false;
- for (unsigned I = 0; I != NumBuckets;) {
- StringMapEntryBase *Bucket = TheTable[I];
- if (!Bucket) {
- ++I;
+ for (StringMapEntryBase *&Bucket : buckets()) {
+ if (!Bucket || Bucket == getTombstoneVal())
continue;
- }
auto *Entry = static_cast<MapEntryTy *>(Bucket);
- if (!Pred(*Entry)) {
- ++I;
- continue;
+ if (Pred(*Entry)) {
+ Entry->Destroy(getAllocator());
+ Bucket = getTombstoneVal();
+ --NumItems;
+ ++NumTombstones;
+ Removed = true;
}
- Entry->Destroy(getAllocator());
- // This may relocate a following entry into this slot to close the hole,
- // so re-examine the same index rather than advancing past it.
- removeBucket(I);
- Removed = true;
}
if (Removed)
incrementEpoch();
@@ -529,7 +542,7 @@ class StringMapIterBase : DebugEpochBase::HandleBase {
private:
void AdvancePastEmptyBuckets() {
- while (*Ptr == nullptr)
+ while (*Ptr == nullptr || *Ptr == StringMapImpl::getTombstoneVal())
++Ptr;
}
};
diff --git a/llvm/lib/Support/StringMap.cpp b/llvm/lib/Support/StringMap.cpp
index fe59d4485a422..4aee30cd484e0 100644
--- a/llvm/lib/Support/StringMap.cpp
+++ b/llvm/lib/Support/StringMap.cpp
@@ -62,6 +62,7 @@ void StringMapImpl::init(unsigned InitSize) {
unsigned NewNumBuckets = InitSize ? InitSize : 16;
NumItems = 0;
+ NumTombstones = 0;
TheTable = createTable(NewNumBuckets);
@@ -87,15 +88,28 @@ unsigned StringMapImpl::LookupBucketFor(StringRef Name,
unsigned BucketNo = FullHashValue & (NumBuckets - 1);
unsigned *HashTable = getHashTable(TheTable, NumBuckets);
+ unsigned ProbeAmt = 1;
+ int FirstTombstone = -1;
while (true) {
StringMapEntryBase *BucketItem = TheTable[BucketNo];
// If we found an empty bucket, this key isn't in the table yet, return it.
if (LLVM_LIKELY(!BucketItem)) {
+ // If we found a tombstone, we want to reuse the tombstone instead of an
+ // empty bucket. This reduces probing.
+ if (FirstTombstone != -1) {
+ HashTable[FirstTombstone] = FullHashValue;
+ return FirstTombstone;
+ }
+
HashTable[BucketNo] = FullHashValue;
return BucketNo;
}
- if (LLVM_LIKELY(HashTable[BucketNo] == FullHashValue)) {
+ if (BucketItem == getTombstoneVal()) {
+ // Skip over tombstones. However, remember the first one we see.
+ if (FirstTombstone == -1)
+ FirstTombstone = BucketNo;
+ } else if (LLVM_LIKELY(HashTable[BucketNo] == FullHashValue)) {
// If the full hash value matches, check deeply for a match. The common
// case here is that we are only looking at the buckets (for item info
// being non-null and for the full hash value) not at the items. This
@@ -111,7 +125,11 @@ unsigned StringMapImpl::LookupBucketFor(StringRef Name,
}
// Okay, we didn't find the item. Probe to the next bucket.
- BucketNo = (BucketNo + 1) & (NumBuckets - 1);
+ BucketNo = (BucketNo + ProbeAmt) & (NumBuckets - 1);
+
+ // Use quadratic probing, it has fewer clumping artifacts than linear
+ // probing and has good cache behavior in the common case.
+ ++ProbeAmt;
}
}
@@ -129,13 +147,16 @@ int StringMapImpl::FindKey(StringRef Key, uint32_t FullHashValue) const {
unsigned BucketNo = FullHashValue & (NumBuckets - 1);
unsigned *HashTable = getHashTable(TheTable, NumBuckets);
+ unsigned ProbeAmt = 1;
while (true) {
StringMapEntryBase *BucketItem = TheTable[BucketNo];
// If we found an empty bucket, this key isn't in the table yet, return.
if (LLVM_LIKELY(!BucketItem))
return -1;
- if (LLVM_LIKELY(HashTable[BucketNo] == FullHashValue)) {
+ if (BucketItem == getTombstoneVal()) {
+ // Ignore tombstones.
+ } else if (LLVM_LIKELY(HashTable[BucketNo] == FullHashValue)) {
// If the full hash value matches, check deeply for a match. The common
// case here is that we are only looking at the buckets (for item info
// being non-null and for the full hash value) not at the items. This
@@ -151,7 +172,11 @@ int StringMapImpl::FindKey(StringRef Key, uint32_t FullHashValue) const {
}
// Okay, we didn't find the item. Probe to the next bucket.
- BucketNo = (BucketNo + 1) & (NumBuckets - 1);
+ BucketNo = (BucketNo + ProbeAmt) & (NumBuckets - 1);
+
+ // Use quadratic probing, it has fewer clumping artifacts than linear
+ // probing and has good cache behavior in the common case.
+ ++ProbeAmt;
}
}
@@ -164,32 +189,19 @@ void StringMapImpl::RemoveKey(StringMapEntryBase *V) {
assert(V == V2 && "Didn't find key?");
}
-// Remove the StringMapEntry for the specified key from the table. Knuth
-// TAOCP 6.4 Algorithm R: walk forward sliding each following entry whose probe
-// path crosses the hole.
-void StringMapImpl::removeBucket(unsigned Bucket) {
- unsigned *HashTable = getHashTable(TheTable, NumBuckets);
- unsigned Mask = NumBuckets - 1;
- unsigned I = Bucket, J = I;
- while ((J = (J + 1) & Mask), TheTable[J]) {
- unsigned Ideal = HashTable[J];
- if (((I - Ideal) & Mask) < ((J - Ideal) & Mask)) {
- TheTable[I] = TheTable[J];
- HashTable[I] = HashTable[J];
- I = J;
- }
- }
- TheTable[I] = nullptr;
- --NumItems;
-}
-
+/// RemoveKey - Remove the StringMapEntry for the specified key from the
+/// table, returning it. If the key is not in the table, this returns null.
StringMapEntryBase *StringMapImpl::RemoveKey(StringRef Key) {
int Bucket = FindKey(Key);
if (Bucket == -1)
return nullptr;
StringMapEntryBase *Result = TheTable[Bucket];
- removeBucket(Bucket);
+ TheTable[Bucket] = getTombstoneVal();
+ --NumItems;
+ ++NumTombstones;
+ assert(NumItems + NumTombstones <= NumBuckets);
+
return Result;
}
@@ -197,9 +209,14 @@ StringMapEntryBase *StringMapImpl::RemoveKey(StringRef Key) {
/// the appropriate mod-of-hashtable-size.
unsigned StringMapImpl::RehashTable(unsigned BucketNo) {
unsigned NewSize;
- // If the hash table is now more than 3/4 full, grow the table.
+ // If the hash table is now more than 3/4 full, or if fewer than 1/8 of
+ // the buckets are empty (meaning that many are filled with tombstones),
+ // grow/rehash the table.
if (LLVM_UNLIKELY(NumItems * 4 > NumBuckets * 3)) {
NewSize = NumBuckets * 2;
+ } else if (LLVM_UNLIKELY(NumBuckets - (NumItems + NumTombstones) <=
+ NumBuckets / 8)) {
+ NewSize = NumBuckets;
} else {
return BucketNo;
}
@@ -213,12 +230,16 @@ unsigned StringMapImpl::RehashTable(unsigned BucketNo) {
// the hash values available, so we don't have to rehash any strings.
for (unsigned I = 0, E = NumBuckets; I != E; ++I) {
StringMapEntryBase *Bucket = TheTable[I];
- if (Bucket) {
+ if (Bucket && Bucket != getTombstoneVal()) {
// If the bucket is not available, probe for a spot.
unsigned FullHash = HashTable[I];
unsigned NewBucket = FullHash & (NewSize - 1);
- while (NewTableArray[NewBucket])
- NewBucket = (NewBucket + 1) & (NewSize - 1);
+ if (NewTableArray[NewBucket]) {
+ unsigned ProbeSize = 1;
+ do {
+ NewBucket = (NewBucket + ProbeSize++) & (NewSize - 1);
+ } while (NewTableArray[NewBucket]);
+ }
// Finally found a slot. Fill it in.
NewTableArray[NewBucket] = Bucket;
@@ -232,5 +253,6 @@ unsigned StringMapImpl::RehashTable(unsigned BucketNo) {
TheTable = NewTableArray;
NumBuckets = NewSize;
+ NumTombstones = 0;
return NewBucketNo;
}
diff --git a/llvm/lib/Transforms/IPO/StripSymbols.cpp b/llvm/lib/Transforms/IPO/StripSymbols.cpp
index 5e00ef468f9ef..ec701b6d0037b 100644
--- a/llvm/lib/Transforms/IPO/StripSymbols.cpp
+++ b/llvm/lib/Transforms/IPO/StripSymbols.cpp
@@ -22,7 +22,6 @@
#include "llvm/Transforms/IPO/StripSymbols.h"
#include "llvm/ADT/SmallPtrSet.h"
-#include "llvm/ADT/SmallVector.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DebugInfo.h"
#include "llvm/IR/DerivedTypes.h"
@@ -77,17 +76,15 @@ static void RemoveDeadConstant(Constant *C) {
// Strip the symbol table of its names.
//
static void StripSymtab(ValueSymbolTable &ST, bool PreserveDbgInfo) {
- // Collect the values to rename first: setName("") removes the value from the
- // symbol table, which invalidates iterators into it.
- SmallVector<Value *, 0> ToStrip;
- for (const ValueName &VN : ST) {
- Value *V = VN.getValue();
- if (!isa<GlobalValue>(V) || cast<GlobalValue>(V)->hasLocalLinkage())
+ for (ValueSymbolTable::iterator VI = ST.begin(), VE = ST.end(); VI != VE; ) {
+ Value *V = VI->getValue();
+ ++VI;
+ if (!isa<GlobalValue>(V) || cast<GlobalValue>(V)->hasLocalLinkage()) {
if (!PreserveDbgInfo || !V->getName().starts_with("llvm.dbg"))
- ToStrip.push_back(V);
+ // Set name to "", removing from symbol table!
+ V->setName("");
+ }
}
- for (Value *V : ToStrip)
- V->setName("");
}
// Strip any named types of their names.
diff --git a/llvm/unittests/ADT/StringMapTest.cpp b/llvm/unittests/ADT/StringMapTest.cpp
index f9e20e6da925d..d350b46525ba0 100644
--- a/llvm/unittests/ADT/StringMapTest.cpp
+++ b/llvm/unittests/ADT/StringMapTest.cpp
@@ -12,7 +12,6 @@
#include "llvm/Support/DataTypes.h"
#include "gtest/gtest.h"
#include <limits>
-#include <map>
#include <tuple>
using namespace llvm;
@@ -181,42 +180,6 @@ TEST_F(StringMapTest, SmallFullMapTest) {
EXPECT_EQ(5, Map.lookup("funf"));
}
-// Stress test erase. Interleave inserts and erases so that probe clusters form,
-// shrink, and straddle the wrap-around, then verify every surviving key is
-// still findable and every erased key is gone.
-TEST_F(StringMapTest, EraseStressTest) {
- llvm::StringMap<unsigned> Map;
- std::map<std::string, unsigned> Ref;
- // 64-bit Linear Congruential Generator.
- uint64_t State = 1;
- auto Next = [&] {
- return (State = State * 6364136223846793005ULL + 1) >> 33;
- };
- for (unsigned I = 0; I != 4000; ++I) {
- std::string Key = Twine(Next() % 100).str();
- if (Next() & 1) {
- Map[Key] = I;
- Ref[Key] = I;
- } else {
- EXPECT_EQ(Map.erase(Key), Ref.erase(Key) != 0);
- }
-
- // Periodically cross-check the whole map against the reference.
- if (I % 200 == 0) {
- EXPECT_EQ(Map.size(), Ref.size());
- for (const auto &KV : Ref) {
- auto It = Map.find(KV.first);
- ASSERT_NE(It, Map.end()) << "missing key " << KV.first;
- EXPECT_EQ(It->second, KV.second);
- }
- }
- }
-
- EXPECT_EQ(Map.size(), Ref.size());
- for (const auto &KV : Ref)
- EXPECT_EQ(Map.lookup(KV.first), KV.second);
-}
-
TEST_F(StringMapTest, CopyCtorTest) {
llvm::StringMap<int> Map;
@@ -804,46 +767,6 @@ TEST(StringMapCustomTest, RemoveIf) {
EXPECT_EQ(2u, Map.size());
}
-// Stress remove_if: it deletes in place using Algorithm R backward shifting and
-// re-examines each slot after a removal, which is subtle around probe clusters
-// that wrap past the end of the table. Remove a subset from a large map and
-// verify the survivors exactly match the reference.
-TEST(StringMapCustomTest, RemoveIfStress) {
- llvm::StringMap<unsigned> Map;
- std::map<std::string, unsigned> Ref;
-
- // Deterministic LCG so the sequence is reproducible across platforms.
- uint64_t State = 0x9e3779b9;
- auto Next = [&] {
- return (State = State * 6364136223846793005ULL + 1) >> 33;
- };
-
- for (unsigned I = 0; I != 4000; ++I) {
- std::string Key = Twine(Next() % 1500).str();
- Map[Key] = I;
- Ref[Key] = I;
- }
-
- auto IsEven = [](unsigned V) { return V % 2 == 0; };
- Map.remove_if(
- [&](const StringMapEntry<unsigned> &E) { return IsEven(E.getValue()); });
- for (auto It = Ref.begin(); It != Ref.end();) {
- if (IsEven(It->second))
- It = Ref.erase(It);
- else
- ++It;
- }
-
- EXPECT_EQ(Map.size(), Ref.size());
- for (const auto &KV : Ref) {
- auto It = Map.find(KV.first);
- ASSERT_NE(It, Map.end()) << "missing key " << KV.first;
- EXPECT_EQ(It->second, KV.second);
- }
- for (const auto &E : Map)
- EXPECT_FALSE(IsEven(E.getValue())) << "stale key " << E.getKey();
-}
-
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
TEST(StringMapCustomTest, InsertInvalidatesIterators) {
StringMap<int> Map;
@@ -862,17 +785,6 @@ TEST(StringMapCustomTest, EraseIn...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/203003
More information about the llvm-commits
mailing list