Skip to content

Commit 2a29d24

Browse files
authored
[ADT] Use perfect forwarding in SmallSet::insert() (#108590)
Previously this method took arguments by const-ref. This patch changes the implementation to take perfectly forwarded arguments in the form of a universal reference. Now, the insertion method will take advantage of arguments passed as rvalue, potentially leading to performance improvements.
1 parent 9583215 commit 2a29d24

File tree

2 files changed

+61
-19
lines changed

2 files changed

+61
-19
lines changed

llvm/include/llvm/ADT/SmallSet.h

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -161,26 +161,10 @@ class SmallSet {
161161
/// Returns a pair. The first value of it is an iterator to the inserted
162162
/// element or the existing element in the set. The second value is true
163163
/// if the element is inserted (it was not in the set before).
164-
std::pair<const_iterator, bool> insert(const T &V) {
165-
if (!isSmall()) {
166-
auto [I, Inserted] = Set.insert(V);
167-
return std::make_pair(const_iterator(I), Inserted);
168-
}
169-
170-
auto I = std::find(Vector.begin(), Vector.end(), V);
171-
if (I != Vector.end()) // Don't reinsert if it already exists.
172-
return std::make_pair(const_iterator(I), false);
173-
if (Vector.size() < N) {
174-
Vector.push_back(V);
175-
return std::make_pair(const_iterator(std::prev(Vector.end())), true);
176-
}
164+
std::pair<const_iterator, bool> insert(const T &V) { return insertImpl(V); }
177165

178-
// Otherwise, grow from vector to set.
179-
while (!Vector.empty()) {
180-
Set.insert(Vector.back());
181-
Vector.pop_back();
182-
}
183-
return std::make_pair(const_iterator(Set.insert(V).first), true);
166+
std::pair<const_iterator, bool> insert(T &&V) {
167+
return insertImpl(std::move(V));
184168
}
185169

186170
template <typename IterT>
@@ -226,6 +210,30 @@ class SmallSet {
226210

227211
private:
228212
bool isSmall() const { return Set.empty(); }
213+
214+
template <typename ArgType>
215+
std::pair<const_iterator, bool> insertImpl(ArgType &&V) {
216+
static_assert(std::is_convertible_v<ArgType, T>,
217+
"ArgType must be convertible to T!");
218+
if (!isSmall()) {
219+
auto [I, Inserted] = Set.insert(std::forward<ArgType>(V));
220+
return {const_iterator(I), Inserted};
221+
}
222+
223+
auto I = std::find(Vector.begin(), Vector.end(), V);
224+
if (I != Vector.end()) // Don't reinsert if it already exists.
225+
return {const_iterator(I), false};
226+
if (Vector.size() < N) {
227+
Vector.push_back(std::forward<ArgType>(V));
228+
return {const_iterator(std::prev(Vector.end())), true};
229+
}
230+
231+
// Otherwise, grow from vector to set.
232+
Set.insert(std::make_move_iterator(Vector.begin()),
233+
std::make_move_iterator(Vector.end()));
234+
Vector.clear();
235+
return {const_iterator(Set.insert(std::forward<ArgType>(V)).first), true};
236+
}
229237
};
230238

231239
/// If this set is of pointer values, transparently switch over to using

llvm/unittests/ADT/SmallSetTest.cpp

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,40 @@ TEST(SmallSetTest, Insert) {
4141
EXPECT_EQ(0u, s1.count(4));
4242
}
4343

44+
TEST(SmallSetTest, InsertPerfectFwd) {
45+
struct Value {
46+
int Key;
47+
bool Moved;
48+
49+
Value(int Key) : Key(Key), Moved(false) {}
50+
Value(const Value &) = default;
51+
Value(Value &&Other) : Key(Other.Key), Moved(false) { Other.Moved = true; }
52+
bool operator==(const Value &Other) const { return Key == Other.Key; }
53+
bool operator<(const Value &Other) const { return Key < Other.Key; }
54+
};
55+
56+
{
57+
SmallSet<Value, 4> S;
58+
Value V1(1), V2(2);
59+
60+
S.insert(V1);
61+
EXPECT_EQ(V1.Moved, false);
62+
63+
S.insert(std::move(V2));
64+
EXPECT_EQ(V2.Moved, true);
65+
}
66+
{
67+
SmallSet<Value, 1> S;
68+
Value V1(1), V2(2);
69+
70+
S.insert(V1);
71+
EXPECT_EQ(V1.Moved, false);
72+
73+
S.insert(std::move(V2));
74+
EXPECT_EQ(V2.Moved, true);
75+
}
76+
}
77+
4478
TEST(SmallSetTest, Grow) {
4579
SmallSet<int, 4> s1;
4680

0 commit comments

Comments
 (0)