[llvm] r195239 - Give SmallPtrSet move semantics when we have R-value references.
David Blaikie
dblaikie at gmail.com
Wed Nov 20 08:34:14 PST 2013
By the looks of it, if I removed the move operations these tests would
still pass, right? (as they do in C++98 mode)
Since move semantics are an optimization I suppose this is sort of to be
expected, though the tests could
On Wed, Nov 20, 2013 at 3:14 AM, Chandler Carruth <chandlerc at gmail.com>wrote:
> Author: chandlerc
> Date: Wed Nov 20 05:14:33 2013
> New Revision: 195239
>
> URL: http://llvm.org/viewvc/llvm-project?rev=195239&view=rev
> Log:
> Give SmallPtrSet move semantics when we have R-value references.
> Somehow, this ADT got missed which is moderately terrifying considering
> the efficiency of move for it.
>
> The code to implement move semantics for it is pretty horrible
> currently but was written to reasonably closely match the rest of the
> code. Unittests that cover both copying and moving (at a basic level)
> added.
>
> Modified:
> llvm/trunk/include/llvm/ADT/SmallPtrSet.h
> llvm/trunk/lib/Support/SmallPtrSet.cpp
> llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp
>
> Modified: llvm/trunk/include/llvm/ADT/SmallPtrSet.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallPtrSet.h?rev=195239&r1=195238&r2=195239&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/SmallPtrSet.h (original)
> +++ llvm/trunk/include/llvm/ADT/SmallPtrSet.h Wed Nov 20 05:14:33 2013
> @@ -60,8 +60,12 @@ protected:
> unsigned NumElements;
> unsigned NumTombstones;
>
> - // Helper to copy construct a SmallPtrSet.
> - SmallPtrSetImpl(const void **SmallStorage, const SmallPtrSetImpl& that);
> + // Helpers to copy and move construct a SmallPtrSet.
> + SmallPtrSetImpl(const void **SmallStorage, const SmallPtrSetImpl &that);
> +#if LLVM_HAS_RVALUE_REFERENCES
> + SmallPtrSetImpl(const void **SmallStorage, unsigned SmallSize,
> + SmallPtrSetImpl &&that);
> +#endif
> explicit SmallPtrSetImpl(const void **SmallStorage, unsigned SmallSize)
> :
> SmallArray(SmallStorage), CurArray(SmallStorage),
> CurArraySize(SmallSize) {
> assert(SmallSize && (SmallSize & (SmallSize-1)) == 0 &&
> @@ -135,6 +139,9 @@ protected:
> void swap(SmallPtrSetImpl &RHS);
>
> void CopyFrom(const SmallPtrSetImpl &RHS);
> +#if LLVM_HAS_RVALUE_REFERENCES
> + void MoveFrom(SmallPtrSetImpl &&RHS);
> +#endif
> };
>
> /// SmallPtrSetIteratorImpl - This is the common base class shared
> between all
> @@ -242,6 +249,10 @@ class SmallPtrSet : public SmallPtrSetIm
> public:
> SmallPtrSet() : SmallPtrSetImpl(SmallStorage, SmallSizePowTwo) {}
> SmallPtrSet(const SmallPtrSet &that) : SmallPtrSetImpl(SmallStorage,
> that) {}
> +#if LLVM_HAS_RVALUE_REFERENCES
> + SmallPtrSet(SmallPtrSet &&that)
> + : SmallPtrSetImpl(SmallStorage, SmallSizePowTwo, std::move(that)) {}
> +#endif
>
> template<typename It>
> SmallPtrSet(It I, It E) : SmallPtrSetImpl(SmallStorage,
> SmallSizePowTwo) {
> @@ -280,14 +291,20 @@ public:
> return iterator(CurArray+CurArraySize, CurArray+CurArraySize);
> }
>
> - // Allow assignment from any smallptrset with the same element type
> even if it
> - // doesn't have the same smallsize.
> - const SmallPtrSet<PtrType, SmallSize>&
> + SmallPtrSet<PtrType, SmallSize> &
>
We could have a test for this ((a = b).insert(...) for example).
> operator=(const SmallPtrSet<PtrType, SmallSize> &RHS) {
> CopyFrom(RHS);
> return *this;
> }
>
> +#if LLVM_HAS_RVALUE_REFERENCES
> + SmallPtrSet<PtrType, SmallSize>&
> + operator=(SmallPtrSet<PtrType, SmallSize> &&RHS) {
> + MoveFrom(std::move(RHS));
> + return *this;
> + }
> +#endif
> +
> /// swap - Swaps the elements of two sets.
> void swap(SmallPtrSet<PtrType, SmallSize> &RHS) {
> SmallPtrSetImpl::swap(RHS);
>
> Modified: llvm/trunk/lib/Support/SmallPtrSet.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/SmallPtrSet.cpp?rev=195239&r1=195238&r2=195239&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/SmallPtrSet.cpp (original)
> +++ llvm/trunk/lib/Support/SmallPtrSet.cpp Wed Nov 20 05:14:33 2013
> @@ -186,6 +186,29 @@ SmallPtrSetImpl::SmallPtrSetImpl(const v
> NumTombstones = that.NumTombstones;
> }
>
> +#if LLVM_HAS_RVALUE_REFERENCES
> +SmallPtrSetImpl::SmallPtrSetImpl(const void **SmallStorage, unsigned
> SmallSize,
> + SmallPtrSetImpl &&that) {
> + SmallArray = SmallStorage;
> +
> + // Copy over the basic members.
> + CurArraySize = that.CurArraySize;
> + NumElements = that.NumElements;
> + NumTombstones = that.NumTombstones;
> +
> + // When small, just copy into our small buffer.
> + if (that.isSmall()) {
> + CurArray = SmallArray;
> + memcpy(CurArray, that.CurArray, sizeof(void *) * CurArraySize);
> + return;
> + }
> +
> + // Otherwise, we steal the large memory allocation and no copy is
> needed.
> + CurArray = that.CurArray;
> + that.CurArray = that.SmallArray;
>
I haven't looked at this closely yet, but do you need to reset
that.CurArraySize/NumElements/NumTombstones or anything else? You have no
test coverage on the moved-from objects, what guarantees, if any, are
provided? (empty? valid-but-unspecified? invalid-but-destructible and
move-assignable-to?)
> +}
> +#endif
> +
> /// CopyFrom - implement operator= from a smallptrset that has the same
> pointer
> /// type, but may have a different small size.
> void SmallPtrSetImpl::CopyFrom(const SmallPtrSetImpl &RHS) {
> @@ -222,6 +245,27 @@ void SmallPtrSetImpl::CopyFrom(const Sma
> NumTombstones = RHS.NumTombstones;
> }
>
> +#if LLVM_HAS_RVALUE_REFERENCES
> +void SmallPtrSetImpl::MoveFrom(SmallPtrSetImpl &&RHS) {
> + if (!isSmall())
> + free(CurArray);
> +
> + if (RHS.isSmall()) {
> + // Copy a small RHS rather than moving.
> + CurArray = SmallArray;
> + memcpy(CurArray, RHS.CurArray, sizeof(void*)*RHS.CurArraySize);
> + } else {
> + CurArray = RHS.CurArray;
> + RHS.CurArray = RHS.SmallArray;
> + }
> +
> + // Copy the rest of the trivial members.
> + CurArraySize = RHS.CurArraySize;
> + NumElements = RHS.NumElements;
> + NumTombstones = RHS.NumTombstones;
>
Similarly here, I'm wondering what state the moved-from object ends up in.
I'm guessing the RHS.CurArray = RHS.SmallArray is enough to make the object
invalid-but-destructible (& move-assignable to?) and that's all you want to
guarantee?
> +}
> +#endif
> +
> void SmallPtrSetImpl::swap(SmallPtrSetImpl &RHS) {
> if (this == &RHS) return;
>
>
> Modified: llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp?rev=195239&r1=195238&r2=195239&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/SmallPtrSetTest.cpp Wed Nov 20 05:14:33 2013
> @@ -71,6 +71,55 @@ TEST(SmallPtrSetTest, GrowthTest) {
> EXPECT_EQ(1,buf[i]);
> }
>
> +TEST(SmallPtrSetTest, CopyAndMoveTest) {
> + int buf[8];
> + for (int i = 0; i < 8; ++i)
> + buf[i] = 0;
> +
> + SmallPtrSet<int *, 4> s1;
> + s1.insert(&buf[0]);
> + s1.insert(&buf[1]);
> + s1.insert(&buf[2]);
> + s1.insert(&buf[3]);
> + EXPECT_EQ(4U, s1.size());
> + for (int i = 0; i < 8; ++i)
> + if (i < 4)
> + EXPECT_TRUE(s1.count(&buf[i]));
> + else
> + EXPECT_FALSE(s1.count(&buf[i]));
> +
> + SmallPtrSet<int *, 4> s2(s1);
> + EXPECT_EQ(4U, s2.size());
> + for (int i = 0; i < 8; ++i)
> + if (i < 4)
> + EXPECT_TRUE(s2.count(&buf[i]));
> + else
> + EXPECT_FALSE(s2.count(&buf[i]));
> +
> + s1 = s2;
> + EXPECT_EQ(4U, s1.size());
> + for (int i = 0; i < 8; ++i)
> + if (i < 4)
> + EXPECT_TRUE(s1.count(&buf[i]));
> + else
> + EXPECT_FALSE(s1.count(&buf[i]));
> +
> + SmallPtrSet<int *, 4> s3(llvm_move(s1));
> + EXPECT_EQ(4U, s3.size());
> + for (int i = 0; i < 8; ++i)
> + if (i < 4)
> + EXPECT_TRUE(s3.count(&buf[i]));
> + else
> + EXPECT_FALSE(s3.count(&buf[i]));
> +
> + s1 = llvm_move(s3);
> + EXPECT_EQ(4U, s1.size());
> + for (int i = 0; i < 8; ++i)
> + if (i < 4)
> + EXPECT_TRUE(s1.count(&buf[i]));
> + else
> + EXPECT_FALSE(s1.count(&buf[i]));
> +}
>
> TEST(SmallPtrSetTest, SwapTest) {
> int buf[10];
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131120/12dfa7b3/attachment.html>
More information about the llvm-commits
mailing list