-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[ADT] Refactor DenseMap::insert, try_emplace, and operator[] (NFC) #155204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ADT] Refactor DenseMap::insert, try_emplace, and operator[] (NFC) #155204
Conversation
try_emplace and operator[] contain nearly identical code, and the code is duplicated for l-value and r-value reference variants. This patch introduces a templated helper function, try_emplace_impl, and uses it in all of DenseMap::insert, try_emplace, and operator[]. The helper function uses perfect forwarding to preserve the exact key type.
@llvm/pr-subscribers-llvm-adt Author: Kazu Hirata (kazutakahirata) Changestry_emplace and operator[] contain nearly identical code, and the code This patch introduces a templated helper function, try_emplace_impl, Full diff: https://github.com/llvm/llvm-project/pull/155204.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index ab1bc6356dcb9..4960c85138436 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -240,14 +240,14 @@ class DenseMapBase : public DebugEpochBase {
// If the key is already in the map, it returns false and doesn't update the
// value.
std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
- return try_emplace(KV.first, KV.second);
+ return try_emplace_impl(KV.first, KV.second);
}
// Inserts key,value pair into the map if the key isn't already in the map.
// If the key is already in the map, it returns false and doesn't update the
// value.
std::pair<iterator, bool> insert(std::pair<KeyT, ValueT> &&KV) {
- return try_emplace(std::move(KV.first), std::move(KV.second));
+ return try_emplace_impl(std::move(KV.first), std::move(KV.second));
}
// Inserts key,value pair into the map if the key isn't already in the map.
@@ -255,14 +255,7 @@ class DenseMapBase : public DebugEpochBase {
// it is not moved.
template <typename... Ts>
std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&...Args) {
- BucketT *TheBucket;
- if (LookupBucketFor(Key, TheBucket))
- return {makeInsertIterator(TheBucket), false}; // Already in map.
-
- // Otherwise, insert the new element.
- TheBucket =
- InsertIntoBucket(TheBucket, std::move(Key), std::forward<Ts>(Args)...);
- return {makeInsertIterator(TheBucket), true};
+ return try_emplace_impl(std::move(Key), std::forward<Ts>(Args)...);
}
// Inserts key,value pair into the map if the key isn't already in the map.
@@ -270,13 +263,7 @@ class DenseMapBase : public DebugEpochBase {
// it is not moved.
template <typename... Ts>
std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&...Args) {
- BucketT *TheBucket;
- if (LookupBucketFor(Key, TheBucket))
- return {makeInsertIterator(TheBucket), false}; // Already in map.
-
- // Otherwise, insert the new element.
- TheBucket = InsertIntoBucket(TheBucket, Key, std::forward<Ts>(Args)...);
- return {makeInsertIterator(TheBucket), true};
+ return try_emplace_impl(Key, std::forward<Ts>(Args)...);
}
/// Alternate version of insert() which allows a different, and possibly
@@ -360,19 +347,11 @@ class DenseMapBase : public DebugEpochBase {
}
ValueT &operator[](const KeyT &Key) {
- BucketT *TheBucket;
- if (LookupBucketFor(Key, TheBucket))
- return TheBucket->second;
-
- return InsertIntoBucket(TheBucket, Key)->second;
+ return try_emplace_impl(Key).first->second;
}
ValueT &operator[](KeyT &&Key) {
- BucketT *TheBucket;
- if (LookupBucketFor(Key, TheBucket))
- return TheBucket->second;
-
- return InsertIntoBucket(TheBucket, std::move(Key))->second;
+ return try_emplace_impl(std::move(Key)).first->second;
}
/// isPointerIntoBucketsArray - Return true if the specified pointer points
@@ -496,6 +475,18 @@ class DenseMapBase : public DebugEpochBase {
static const KeyT getTombstoneKey() { return KeyInfoT::getTombstoneKey(); }
private:
+ template <typename KeyArgT, typename... Ts>
+ std::pair<iterator, bool> try_emplace_impl(KeyArgT &&Key, Ts &&...Args) {
+ BucketT *TheBucket;
+ if (LookupBucketFor(Key, TheBucket))
+ return {makeInsertIterator(TheBucket), false}; // Already in map.
+
+ // Otherwise, insert the new element.
+ TheBucket = InsertIntoBucket(TheBucket, std::forward<KeyArgT>(Key),
+ std::forward<Ts>(Args)...);
+ return {makeInsertIterator(TheBucket), true};
+ }
+
iterator makeIterator(BucketT *P, BucketT *E, DebugEpochBase &Epoch,
bool NoAdvance = false) {
if (shouldReverseIterate<KeyT>()) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
This causes a large compile-time regression, especially when building with clang: https://llvm-compile-time-tracker.com/compare.php?from=790a132b8535e28d118ba3c9f5e02dd7853bbac4&to=2e896274bd4e61891824fce35f7e0552b2f4be4b&stat=instructions:u (+0.5%) |
If you don't plan to mitigate the regression in the near future, please revert the change. |
An attempt to resolve the slowdown from llvm#155204
An attempt to resolve the slowdown from llvm#155204
An attempt to resolve the slowdown from #155204
Slowdown resolved by #155862 |
try_emplace and operator[] contain nearly identical code, and the code
is duplicated for l-value and r-value reference variants.
This patch introduces a templated helper function, try_emplace_impl,
and uses it in all of DenseMap::insert, try_emplace, and operator[].
The helper function uses perfect forwarding to preserve the exact key
type.