Skip to content

Conversation

kazutakahirata
Copy link
Contributor

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.

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.
@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2025

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/155204.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMap.h (+18-27)
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>()) {

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

@kazutakahirata kazutakahirata merged commit 2e89627 into llvm:main Aug 25, 2025
8 of 9 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_20250824_DenseMap_try_emplace_impl branch August 25, 2025 16:37
@nikic
Copy link
Contributor

nikic commented Aug 25, 2025

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%)

@nikic
Copy link
Contributor

nikic commented Aug 27, 2025

If you don't plan to mitigate the regression in the near future, please revert the change.

MacDue added a commit to MacDue/llvm-project that referenced this pull request Aug 28, 2025
MacDue added a commit to MacDue/llvm-project that referenced this pull request Aug 28, 2025
MacDue added a commit that referenced this pull request Aug 29, 2025
@MacDue
Copy link
Member

MacDue commented Aug 29, 2025

If you don't plan to mitigate the regression in the near future, please revert the change.

Slowdown resolved by #155862

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants