[llvm] [ADT] Document the UniquingSet contracts. NFC (PR #221860)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 7 21:06:47 PDT 2026
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/221860
>From 4dfe4fde7daa531afb6b63d8147d8aad3499f7fe Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Mon, 7 Sep 2026 17:19:20 -0700
Subject: [PATCH 1/2] [ADT] Document the UniquingSet contracts. NFC
Record what the FoldingSet migrations turned up: a key should alias what
it describes rather than copy it, there is no contextual variant, and a
custom isEqual runs against every colliding node whatever shape its key
has.
Extend an existing test to cover that last contract, and getOrInsert.
Aided by Opus 5
---
llvm/docs/ProgrammersManual.md | 13 +++++++------
llvm/unittests/ADT/FoldingSet.cpp | 29 +++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/llvm/docs/ProgrammersManual.md b/llvm/docs/ProgrammersManual.md
index 02a56bab13957..8a1e9bde35759 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,11 @@ 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). Keep `FoldingSet`
+for keys that are wide or variable-length, which a `FoldingSetNodeID` represents
+naturally. `getKey` and a lookup site are two hand-maintained sides that can
+disagree, though the key type pins their arity and `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..0508a410c4328 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);
@@ -708,17 +712,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 differing 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 +754,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));
}
>From 24217b10fa8d50783c5688c560d19d0fdb6fc9bd Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Mon, 7 Sep 2026 21:06:30 -0700
Subject: [PATCH 2/2] improve test
---
llvm/docs/ProgrammersManual.md | 9 +++++----
llvm/unittests/ADT/FoldingSet.cpp | 13 +++++++++++--
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/llvm/docs/ProgrammersManual.md b/llvm/docs/ProgrammersManual.md
index 8a1e9bde35759..0deb31426597a 100644
--- a/llvm/docs/ProgrammersManual.md
+++ b/llvm/docs/ProgrammersManual.md
@@ -2110,10 +2110,11 @@ if (FooNode *N = Pool.lookup({Opcode, LHS, RHS}, Token))
Pool.insert(new (Allocator) FooNode(Opcode, LHS, RHS), Token);
```
-Prefer `UniquingSet` when a node can yield its key in O(1). Keep `FoldingSet`
-for keys that are wide or variable-length, which a `FoldingSetNodeID` represents
-naturally. `getKey` and a lookup site are two hand-maintained sides that can
-disagree, though the key type pins their arity and `insert` asserts that a node
+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 0508a410c4328..377020c914a54 100644
--- a/llvm/unittests/ADT/FoldingSet.cpp
+++ b/llvm/unittests/ADT/FoldingSet.cpp
@@ -651,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
More information about the llvm-commits
mailing list