[clang] [compiler-rt] [llvm] [lld] [libc] [clang-tools-extra] [flang] [lldb] [libcxx] MapVector: add C++17-style try_emplace and insert_or_assign (PR #71969)
Fangrui Song via cfe-commits
cfe-commits at lists.llvm.org
Sun Nov 12 20:39:46 PST 2023
https://github.com/MaskRay updated https://github.com/llvm/llvm-project/pull/71969
>From ceb7d2236df5ffedcddc2c5b0bbed1c3d0db7c43 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Thu, 9 Nov 2023 23:51:54 -0800
Subject: [PATCH 1/4] MapVector: add C++17-style try_emplace and
insert_or_assign
They can avoid a copy/move for many insert/operator[] uses.
For example, insert_or_assign can be used to simplify
CodeGenModule::AddDeferredUnusedCoverageMapping in
clang/lib/CodeGen/CodeGenModule.cpp
---
llvm/include/llvm/ADT/MapVector.h | 45 ++++++++---
llvm/unittests/ADT/MapVectorTest.cpp | 107 +++++++++++++++++++++++++++
2 files changed, 142 insertions(+), 10 deletions(-)
diff --git a/llvm/include/llvm/ADT/MapVector.h b/llvm/include/llvm/ADT/MapVector.h
index c45779c0ce8e0cf..56d02d73bb0fc30 100644
--- a/llvm/include/llvm/ADT/MapVector.h
+++ b/llvm/include/llvm/ADT/MapVector.h
@@ -114,31 +114,56 @@ class MapVector {
return Pos == Map.end()? ValueT() : Vector[Pos->second].second;
}
- std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
- std::pair<KeyT, typename MapType::mapped_type> Pair = std::make_pair(KV.first, 0);
- std::pair<typename MapType::iterator, bool> Result = Map.insert(Pair);
+ template <typename... Ts>
+ std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&...Args) {
+ std::pair<typename MapType::iterator, bool> Result =
+ Map.insert(std::make_pair(Key, 0));
auto &I = Result.first->second;
if (Result.second) {
- Vector.push_back(std::make_pair(KV.first, KV.second));
+ Vector.emplace_back(std::piecewise_construct, std::forward_as_tuple(Key),
+ std::forward_as_tuple(std::forward<Ts>(Args)...));
I = Vector.size() - 1;
return std::make_pair(std::prev(end()), true);
}
return std::make_pair(begin() + I, false);
}
-
- std::pair<iterator, bool> insert(std::pair<KeyT, ValueT> &&KV) {
- // Copy KV.first into the map, then move it into the vector.
- std::pair<KeyT, typename MapType::mapped_type> Pair = std::make_pair(KV.first, 0);
- std::pair<typename MapType::iterator, bool> Result = Map.insert(Pair);
+ template <typename... Ts>
+ std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&...Args) {
+ std::pair<typename MapType::iterator, bool> Result =
+ Map.insert(std::make_pair(Key, 0));
auto &I = Result.first->second;
if (Result.second) {
- Vector.push_back(std::move(KV));
+ Vector.emplace_back(std::piecewise_construct,
+ std::forward_as_tuple(static_cast<KeyT &&>(Key)),
+ std::forward_as_tuple(std::forward<Ts>(Args)...));
I = Vector.size() - 1;
return std::make_pair(std::prev(end()), true);
}
return std::make_pair(begin() + I, false);
}
+ std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
+ return try_emplace(KV.first, KV.second);
+ }
+ std::pair<iterator, bool> insert(std::pair<KeyT, ValueT> &&KV) {
+ return try_emplace(std::move(KV.first), std::move(KV.second));
+ }
+
+ template <typename V>
+ std::pair<iterator, bool> insert_or_assign(const KeyT &Key, V &&Val) {
+ auto Ret = try_emplace(Key, std::forward<V>(Val));
+ if (!Ret.second)
+ Ret.first->second = std::forward<V>(Val);
+ return Ret;
+ }
+ template <typename V>
+ std::pair<iterator, bool> insert_or_assign(KeyT &&Key, V &&Val) {
+ auto Ret = try_emplace(std::move(Key), std::forward<V>(Val));
+ if (!Ret.second)
+ Ret.first->second = std::forward<V>(Val);
+ return Ret;
+ }
+
bool contains(const KeyT &Key) const { return Map.find(Key) != Map.end(); }
size_type count(const KeyT &Key) const { return contains(Key) ? 1 : 0; }
diff --git a/llvm/unittests/ADT/MapVectorTest.cpp b/llvm/unittests/ADT/MapVectorTest.cpp
index 1a371cbfba81e8e..40645b9a4de07b7 100644
--- a/llvm/unittests/ADT/MapVectorTest.cpp
+++ b/llvm/unittests/ADT/MapVectorTest.cpp
@@ -9,10 +9,36 @@
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/iterator_range.h"
#include "gtest/gtest.h"
+#include <memory>
#include <utility>
using namespace llvm;
+namespace {
+struct CountCopyAndMove {
+ CountCopyAndMove() = default;
+ CountCopyAndMove(const CountCopyAndMove &) { copy = 1; }
+ CountCopyAndMove(CountCopyAndMove &&) { move = 1; }
+ void operator=(const CountCopyAndMove &) { ++copy; }
+ void operator=(CountCopyAndMove &&) { ++move; }
+ int copy = 0;
+ int move = 0;
+};
+
+struct A : CountCopyAndMove {
+ A(int v) : v(v) {}
+ int v;
+};
+} // namespace
+
+template <> struct DenseMapInfo<A> {
+ static inline A getEmptyKey() { return 0x7fffffff; }
+ static inline A getTombstoneKey() { return -0x7fffffff - 1; }
+ static unsigned getHashValue(const A &Val) { return (unsigned)(Val.v * 37U); }
+ static bool isEqual(const A &LHS, const A &RHS) { return LHS.v == RHS.v; }
+};
+
+namespace {
TEST(MapVectorTest, swap) {
MapVector<int, int> MV1, MV2;
std::pair<MapVector<int, int>::iterator, bool> R;
@@ -79,6 +105,86 @@ TEST(MapVectorTest, insert_pop) {
EXPECT_EQ(MV[4], 7);
}
+TEST(MapVectorTest, try_emplace) {
+ struct AAndU {
+ A a;
+ std::unique_ptr<int> b;
+ AAndU(A a, std::unique_ptr<int> b) : a(a), b(std::move(b)) {}
+ };
+ MapVector<A, AAndU> mv;
+
+ A zero(0);
+ auto try0 = mv.try_emplace(zero, zero, nullptr);
+ EXPECT_TRUE(try0.second);
+ EXPECT_EQ(0, try0.first->second.a.v);
+ EXPECT_EQ(1, try0.first->second.a.copy);
+ EXPECT_EQ(0, try0.first->second.a.move);
+
+ auto try1 = mv.try_emplace(zero, zero, nullptr);
+ EXPECT_FALSE(try1.second);
+ EXPECT_EQ(0, try1.first->second.a.v);
+ EXPECT_EQ(1, try1.first->second.a.copy);
+ EXPECT_EQ(0, try1.first->second.a.move);
+
+ EXPECT_EQ(try0.first, try1.first);
+ EXPECT_EQ(1, try1.first->first.copy);
+ EXPECT_EQ(0, try1.first->first.move);
+
+ A two(2);
+ auto try2 = mv.try_emplace(2, (A &&)two, std::make_unique<int>(2));
+ EXPECT_TRUE(try2.second);
+ EXPECT_EQ(2, try2.first->second.a.v);
+ EXPECT_EQ(0, try2.first->second.a.move);
+
+ std::unique_ptr<int> p(new int(3));
+ auto try3 = mv.try_emplace((A &&)two, 3, std::move(p));
+ EXPECT_FALSE(try3.second);
+ EXPECT_EQ(2, try3.first->second.a.v);
+ EXPECT_EQ(1, try3.first->second.a.copy);
+ EXPECT_EQ(0, try3.first->second.a.move);
+
+ EXPECT_EQ(try2.first, try3.first);
+ EXPECT_EQ(0, try3.first->first.copy);
+ EXPECT_EQ(1, try3.first->first.move);
+ EXPECT_NE(nullptr, p);
+}
+
+TEST(MapVectorTest, insert_or_assign) {
+ MapVector<A, A> mv;
+
+ A zero(0);
+ auto try0 = mv.insert_or_assign(zero, zero);
+ EXPECT_TRUE(try0.second);
+ EXPECT_EQ(0, try0.first->second.v);
+ EXPECT_EQ(1, try0.first->second.copy);
+ EXPECT_EQ(0, try0.first->second.move);
+
+ auto try1 = mv.insert_or_assign(zero, zero);
+ EXPECT_FALSE(try1.second);
+ EXPECT_EQ(0, try1.first->second.v);
+ EXPECT_EQ(2, try1.first->second.copy);
+ EXPECT_EQ(0, try1.first->second.move);
+
+ EXPECT_EQ(try0.first, try1.first);
+ EXPECT_EQ(1, try1.first->first.copy);
+ EXPECT_EQ(0, try1.first->first.move);
+
+ A two(2);
+ auto try2 = mv.try_emplace(2, (A &&)two);
+ EXPECT_TRUE(try2.second);
+ EXPECT_EQ(2, try2.first->second.v);
+ EXPECT_EQ(1, try2.first->second.move);
+
+ auto try3 = mv.insert_or_assign((A &&)two, 3);
+ EXPECT_FALSE(try3.second);
+ EXPECT_EQ(3, try3.first->second.v);
+ EXPECT_EQ(0, try3.first->second.copy);
+ EXPECT_EQ(2, try3.first->second.move);
+
+ EXPECT_EQ(try2.first, try3.first);
+ EXPECT_EQ(1, try3.first->first.move);
+}
+
TEST(MapVectorTest, erase) {
MapVector<int, int> MV;
@@ -423,3 +529,4 @@ TEST(SmallMapVectorLargeTest, iteration_test) {
count--;
}
}
+} // namespace
>From 21c627961f4dbbcf0475e3e19aee3facae4db5eb Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Fri, 10 Nov 2023 12:16:01 -0800
Subject: [PATCH 2/4] test try3.first->first.copy
---
llvm/unittests/ADT/MapVectorTest.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/llvm/unittests/ADT/MapVectorTest.cpp b/llvm/unittests/ADT/MapVectorTest.cpp
index 40645b9a4de07b7..9581623bcbe9141 100644
--- a/llvm/unittests/ADT/MapVectorTest.cpp
+++ b/llvm/unittests/ADT/MapVectorTest.cpp
@@ -182,6 +182,7 @@ TEST(MapVectorTest, insert_or_assign) {
EXPECT_EQ(2, try3.first->second.move);
EXPECT_EQ(try2.first, try3.first);
+ EXPECT_EQ(0, try3.first->first.copy);
EXPECT_EQ(1, try3.first->first.move);
}
>From ae1c9738dbfaa5fcf25a24b211cdc71089b484e7 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Fri, 10 Nov 2023 16:41:30 -0800
Subject: [PATCH 3/4] static_cast<KeyT &&>(Key) => std::move(Key) to be
idiomatic
---
llvm/include/llvm/ADT/MapVector.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/include/llvm/ADT/MapVector.h b/llvm/include/llvm/ADT/MapVector.h
index 56d02d73bb0fc30..c97bf3073e62c0b 100644
--- a/llvm/include/llvm/ADT/MapVector.h
+++ b/llvm/include/llvm/ADT/MapVector.h
@@ -134,7 +134,7 @@ class MapVector {
auto &I = Result.first->second;
if (Result.second) {
Vector.emplace_back(std::piecewise_construct,
- std::forward_as_tuple(static_cast<KeyT &&>(Key)),
+ std::forward_as_tuple(std::move(Key)),
std::forward_as_tuple(std::forward<Ts>(Args)...));
I = Vector.size() - 1;
return std::make_pair(std::prev(end()), true);
>From 253448f52cfd2f16d1c077a5f888069e524c8a43 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i at maskray.me>
Date: Sun, 12 Nov 2023 20:33:27 -0800
Subject: [PATCH 4/4] simplify using structured binding
---
llvm/include/llvm/ADT/MapVector.h | 20 ++++++++------------
llvm/unittests/ADT/MapVectorTest.cpp | 10 ++++++----
2 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/llvm/include/llvm/ADT/MapVector.h b/llvm/include/llvm/ADT/MapVector.h
index c97bf3073e62c0b..c11617a81c97d55 100644
--- a/llvm/include/llvm/ADT/MapVector.h
+++ b/llvm/include/llvm/ADT/MapVector.h
@@ -116,30 +116,26 @@ class MapVector {
template <typename... Ts>
std::pair<iterator, bool> try_emplace(const KeyT &Key, Ts &&...Args) {
- std::pair<typename MapType::iterator, bool> Result =
- Map.insert(std::make_pair(Key, 0));
- auto &I = Result.first->second;
- if (Result.second) {
+ auto [It, Inserted] = Map.insert(std::make_pair(Key, 0));
+ if (Inserted) {
+ It->second = Vector.size();
Vector.emplace_back(std::piecewise_construct, std::forward_as_tuple(Key),
std::forward_as_tuple(std::forward<Ts>(Args)...));
- I = Vector.size() - 1;
return std::make_pair(std::prev(end()), true);
}
- return std::make_pair(begin() + I, false);
+ return std::make_pair(begin() + It->second, false);
}
template <typename... Ts>
std::pair<iterator, bool> try_emplace(KeyT &&Key, Ts &&...Args) {
- std::pair<typename MapType::iterator, bool> Result =
- Map.insert(std::make_pair(Key, 0));
- auto &I = Result.first->second;
- if (Result.second) {
+ auto [It, Inserted] = Map.insert(std::make_pair(Key, 0));
+ if (Inserted) {
+ It->second = Vector.size();
Vector.emplace_back(std::piecewise_construct,
std::forward_as_tuple(std::move(Key)),
std::forward_as_tuple(std::forward<Ts>(Args)...));
- I = Vector.size() - 1;
return std::make_pair(std::prev(end()), true);
}
- return std::make_pair(begin() + I, false);
+ return std::make_pair(begin() + It->second, false);
}
std::pair<iterator, bool> insert(const std::pair<KeyT, ValueT> &KV) {
diff --git a/llvm/unittests/ADT/MapVectorTest.cpp b/llvm/unittests/ADT/MapVectorTest.cpp
index 9581623bcbe9141..e0f11b60a0223da 100644
--- a/llvm/unittests/ADT/MapVectorTest.cpp
+++ b/llvm/unittests/ADT/MapVectorTest.cpp
@@ -31,12 +31,14 @@ struct A : CountCopyAndMove {
};
} // namespace
+namespace llvm {
template <> struct DenseMapInfo<A> {
static inline A getEmptyKey() { return 0x7fffffff; }
static inline A getTombstoneKey() { return -0x7fffffff - 1; }
static unsigned getHashValue(const A &Val) { return (unsigned)(Val.v * 37U); }
static bool isEqual(const A &LHS, const A &RHS) { return LHS.v == RHS.v; }
};
+} // namespace llvm
namespace {
TEST(MapVectorTest, swap) {
@@ -131,13 +133,13 @@ TEST(MapVectorTest, try_emplace) {
EXPECT_EQ(0, try1.first->first.move);
A two(2);
- auto try2 = mv.try_emplace(2, (A &&)two, std::make_unique<int>(2));
+ auto try2 = mv.try_emplace(2, std::move(two), std::make_unique<int>(2));
EXPECT_TRUE(try2.second);
EXPECT_EQ(2, try2.first->second.a.v);
EXPECT_EQ(0, try2.first->second.a.move);
std::unique_ptr<int> p(new int(3));
- auto try3 = mv.try_emplace((A &&)two, 3, std::move(p));
+ auto try3 = mv.try_emplace(std::move(two), 3, std::move(p));
EXPECT_FALSE(try3.second);
EXPECT_EQ(2, try3.first->second.a.v);
EXPECT_EQ(1, try3.first->second.a.copy);
@@ -170,12 +172,12 @@ TEST(MapVectorTest, insert_or_assign) {
EXPECT_EQ(0, try1.first->first.move);
A two(2);
- auto try2 = mv.try_emplace(2, (A &&)two);
+ auto try2 = mv.try_emplace(2, std::move(two));
EXPECT_TRUE(try2.second);
EXPECT_EQ(2, try2.first->second.v);
EXPECT_EQ(1, try2.first->second.move);
- auto try3 = mv.insert_or_assign((A &&)two, 3);
+ auto try3 = mv.insert_or_assign(std::move(two), 3);
EXPECT_FALSE(try3.second);
EXPECT_EQ(3, try3.first->second.v);
EXPECT_EQ(0, try3.first->second.copy);
More information about the cfe-commits
mailing list