RFC: [ADT] Use a storage policy in DenseMap/SmallDenseMap (NFC) #168255
+202
−323
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgymnich
reviewed
Nov 16, 2025
Member
tgymnich
left a comment
tgymnich
left a comment
There was a problem hiding this comment.
Good improvement. Thank you!
kazutakahirata
added a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Nov 16, 2025
This patch moves initWithExactBucketCount and ExactBucketCount to
DenseMapBase to share more code.
Since SmallDenseMap::allocateBuckets always returns true,
initWithExactBucketCount is equivalent to:
void initWithExactBucketCount(unsigned NewNumBuckets) {
allocateBuckets(NewNumBuckets);
initEmpty();
}
for SmallDenseMap.
Note that ExactBucketCount is not used within DenseMapBase yet.
This moves us closer to the storage policy idea outlined in llvm#168255.
kazutakahirata
added a commit
that referenced
this pull request
Nov 16, 2025
This patch moves initWithExactBucketCount and ExactBucketCount to
DenseMapBase to share more code.
Since SmallDenseMap::allocateBuckets always returns true,
initWithExactBucketCount is equivalent to:
void initWithExactBucketCount(unsigned NewNumBuckets) {
allocateBuckets(NewNumBuckets);
initEmpty();
}
for SmallDenseMap.
Note that ExactBucketCount is not used within DenseMapBase yet.
This moves us closer to the storage policy idea outlined in #168255.
kazutakahirata
added a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Nov 17, 2025
This patch adds computeNumBuckets, a helper function to compute the number of buckets. This is part of the effort outlined in llvm#168255. This makes it easier to move the core logic of grow() to DenseMapBase::grow().
This patch refactors DenseMap and SmallDenseMap to a policy-based design, moving away from the previous CRTP-based implementation. The primary motivation is to improve the architecture by centralizing logic and removing code duplication. In the current implementation, DenseMap and SmallDenseMap use CRTP, which splits responsibilities in a way that forces redundant code. Specifically, all lifetime management logic (constructors, destructor, copy/move assignment operators) is implemented separately in both DenseMap and SmallDenseMap, while the core container logic resides in DenseMapBase. This patch resolves this by introducing a clean separation of concerns using a storage policy design: - DenseMapBase: Is now a generic, policy-driven implementation that contains all user-facing logic, including constructors and lifetime management. It is parameterized by a storage policy. - DenseMapStorage / SmallDenseMapStorage: Are dumb storage policies responsible only for memory management (allocation/deallocation) and metadata storage. They are owned by DenseMapBase via composition. - DenseMap / SmallDenseMap: Become simple, user-facing forwarding classes (akin to type aliases) that instantiate DenseMapBase with the appropriate storage policy. Key Benefits of this Refactoring: - Improved Code Reuse: All constructors, the destructor, and assignment operators are now implemented once in DenseMapBase, eliminating code duplication. - Clear Separation of Concerns: The logic for hashing and iteration is now cleanly separated from memory allocation and storage layout. - Reduced Code Size: This refactoring reduces the line count of DenseMap.h by 137 lines (~10%). Comments would be greatly appreciated. (Of course, I'm not planning to post this as a single giant pull request.)
kazutakahirata
force-pushed
the
cleanup_20251115z_ADT_DenseMap_policy
branch
from
7774e23 to
0491bdd
Compare
kazutakahirata
added a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Nov 17, 2025
This patch teaches DenseMap constructors to delegate to other DenseMap constructors where we can. The intent is for these constructors to build on top of a higher-level concept like the default-constructed instance instead of calling init on our own. This is part of the effort outlined in llvm#168255.
kazutakahirata
added a commit
that referenced
this pull request
Nov 17, 2025
This patch adds computeNumBuckets, a helper function to compute the number of buckets. This is part of the effort outlined in #168255. This makes it easier to move the core logic of grow() to DenseMapBase::grow().
kazutakahirata
added a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Nov 17, 2025
This patch consolidates the grow() logic in DenseMapBase::grow. With this patch, DenseMapBase::grow() creates a temporary grown instance and then lets DenseMap/SmallDenseMap attempt to move the instance back to *this. If it doesn't work, we move again. The "attempt to move" always succeeds for DenseMap. For SmallDenseMap, it succeeds only in the large mode. This is part of the effort outlined in llvm#168255.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch refactors DenseMap and SmallDenseMap to a policy-based
design, moving away from the previous CRTP-based implementation.
The primary motivation is to improve the architecture by centralizing
logic and removing code duplication. In the current implementation,
DenseMap and SmallDenseMap use CRTP, which splits responsibilities in
a way that forces redundant code. Specifically, all lifetime
management logic (constructors, destructor, copy/move assignment
operators) is implemented separately in both DenseMap and
SmallDenseMap, while the core container logic resides in DenseMapBase.
This patch resolves this by introducing a clean separation of concerns
using a storage policy design:
DenseMapBase: Is now a generic, policy-driven implementation that
contains all user-facing logic, including constructors and lifetime
management. It is parameterized by a storage policy.
DenseMapStorage / SmallDenseMapStorage: Are dumb storage policies
responsible only for memory management (allocation/deallocation) and
metadata storage. They are owned by DenseMapBase via composition.
DenseMap / SmallDenseMap: Become simple, user-facing forwarding
classes (akin to type aliases) that instantiate DenseMapBase with
the appropriate storage policy.
Key Benefits of this Refactoring:
Improved Code Reuse: All constructors, the destructor, and
assignment operators are now implemented once in DenseMapBase,
eliminating code duplication.
Clear Separation of Concerns: The logic for hashing and iteration is
now cleanly separated from memory allocation and storage layout.
Reduced Code Size: This refactoring reduces the line count of
DenseMap.h by 137 lines (~10%).
Comments would be greatly appreciated. (Of course, I'm not planning to
post this as a single giant pull request.)