Author: Fangrui Song
Date: 2026-09-08T04:47:00Z
New Revision: 6a183b8853b79a134402332cf4a776a8090d1b88
URL: https://github.com/llvm/llvm-project/commit/6a183b8853b79a134402332cf4a776a8090d1b88
DIFF: https://github.com/llvm/llvm-project/commit/6a183b8853b79a134402332cf4a776a8090d1b88.diff
LOG: [ADT] Document the UniquingSet contracts. NFC (#221860)
#220195 added an isEqual hook, so an Info can now override the
comparison as well as the key and the hash. Name the three hooks, and
correct the guidance on when to prefer UniquingSet over FoldingSet.
Cover getOrInsert, which UniquingSet had no test for: an absent key
inserts, an equal key returns the node already in the set.
Aided by Opus 5
Co-authored-by: Kazu Hirata <kazu at google.com>
Added:
Modified:
llvm/docs/ProgrammersManual.md
llvm/unittests/ADT/FoldingSet.cpp
Removed:
################################################################################
diff --git a/llvm/docs/ProgrammersManual.md b/llvm/docs/ProgrammersManual.md
index 02a56bab13957..0deb31426597a 100644
--- a/llvm/docs/ProgrammersManual.md
+++ b/llvm/docs/ProgrammersManual.md
@@ -2095,7 +2095,8 @@ supplies its key through `getKey()`; a lookup builds the same key from what it
already holds and hashes it inline with `DenseMapInfo`, and `lookup` returns the
matching node or an insertion token for `insert`. Growth and removal use the
hash cached in each node and never call `getKey`. An `Info` template argument
-can override the key type or the hash.
+can override the key type (`getKey`), the hash (`getHashValue`), or the
+comparison (`isEqual`).
```cpp
std::tuple<unsigned, const Value *, const Value *> FooNode::getKey() const {
@@ -2109,11 +2110,12 @@ if (FooNode *N = Pool.lookup({Opcode, LHS, RHS}, Token))
Pool.insert(new (Allocator) FooNode(Opcode, LHS, RHS), Token);
```
-Prefer `UniquingSet` when a key can be read out of a node in O(1) and the lookup
-key is built next to `getKey`. Keep `FoldingSet` for keys that are wide,
-polymorphic, or assembled at many call sites: one `Profile` helper then keeps
-both sides consistent, whereas `getKey` and a lookup site can silently disagree.
-`insert` asserts that a node hashes as its lookup did.
+Prefer `UniquingSet` when a node can yield its key in O(1), or when a key can
+cheaply alias storage owned by the node (such as an `ArrayRef` or `StringRef`).
+Keep `FoldingSet` when nodes are polymorphic, or when keys must be assembled
+from recursive data structures. `getKey` and a lookup site are two
+hand-maintained sides that can disagree, though `insert` asserts that a node
+hashes as its lookup did.
(dss_set)=
diff --git a/llvm/unittests/ADT/FoldingSet.cpp b/llvm/unittests/ADT/FoldingSet.cpp
index 5d6ab3efed9e4..377020c914a54 100644
--- a/llvm/unittests/ADT/FoldingSet.cpp
+++ b/llvm/unittests/ADT/FoldingSet.cpp
@@ -635,6 +635,10 @@ TEST(UniquingSetTest, Basic) {
KeyedPair B(2, 1);
Set.insert(&B, Token);
+ KeyedPair ADup(1, 2);
+ EXPECT_EQ(&A, Set.getOrInsert(&ADup));
+ EXPECT_EQ(2u, Set.size());
+
std::vector<KeyedPair *> Visited;
for (KeyedPair &N : Set)
Visited.push_back(&N);
@@ -647,10 +651,19 @@ TEST(UniquingSetTest, Basic) {
EXPECT_TRUE(Set.erase(&A));
EXPECT_FALSE(Set.erase(&A));
- KeyedPair NeverInserted(3, 4);
- EXPECT_FALSE(Set.erase(&NeverInserted));
+ KeyedPair C(3, 4);
+ EXPECT_FALSE(Set.erase(&C));
EXPECT_EQ(1u, Set.size());
EXPECT_EQ(nullptr, Set.lookup({1, 2}, Token));
+
+ // getOrInsert inserts an absent node and hands back the one in the set.
+ EXPECT_EQ(&C, Set.getOrInsert(&C));
+ EXPECT_EQ(2u, Set.size());
+ EXPECT_EQ(&C, Set.lookup({3, 4}, Token));
+ EXPECT_FALSE(bool(Token));
+ KeyedPair CDup(3, 4);
+ EXPECT_EQ(&C, Set.getOrInsert(&CDup));
+ EXPECT_EQ(2u, Set.size());
}
// Every key hashes to NotAHash, which must be remapped so that erase() does not
@@ -708,17 +721,21 @@ struct VectorNode : FoldingSetNode {
explicit VectorNode(ArrayRef<unsigned> E) : Elts(E) {}
};
+unsigned ShapeMismatches;
+
struct VectorNodeInfo {
using KeyTy = ArrayRef<unsigned>;
static KeyTy getKey(const VectorNode &N) { return N.Elts; }
+ // Hashing only the first element makes keys of
diff ering length collide.
static unsigned getHashValue(const KeyTy &Key) {
- unsigned H = 0;
- for (unsigned E : Key)
- H = detail::combineHashValue(H, DenseMapInfo<unsigned>::getHashValue(E));
- return H;
+ return Key.empty() ? 1 : DenseMapInfo<unsigned>::getHashValue(Key[0]);
}
// Compare against the node's storage rather than building a key from it.
static bool isEqual(const KeyTy &Key, const VectorNode &N) {
+ if (Key.size() != N.Elts.size()) {
+ ++ShapeMismatches;
+ return false;
+ }
return Key == KeyTy(N.Elts);
}
};
@@ -746,6 +763,19 @@ TEST(UniquingSetTest, StandaloneInfoAliasingKeyAcrossGrowth) {
SmallVector<unsigned, 4> Again = {1, 2, 3};
FoldingSetInsertToken Unused;
EXPECT_EQ(&Late, Set.lookup(Again, Unused));
+
+ // {1, 2} and {1, 2, 3} are the only keys starting with 1, so both collide
+ // with this one and isEqual must reject both on length alone.
+ SmallVector<unsigned, 4> Longer = {1, 2, 3, 4};
+ ShapeMismatches = 0;
+ EXPECT_EQ(nullptr, Set.lookup(Longer, Unused));
+ EXPECT_EQ(2u, ShapeMismatches);
+
+ // getOrInsert derives the key through Info::getKey, which lookup never calls.
+ VectorNode Dup(Lookup);
+ EXPECT_EQ(&Late, Set.getOrInsert(&Dup));
+ EXPECT_EQ(201u, Set.size());
+
EXPECT_TRUE(Set.erase(&Late));
EXPECT_EQ(nullptr, Set.lookup(Again, Unused));
}