[llvm] r195239 - Give SmallPtrSet move semantics when we have R-value references.

Chandler Carruth chandlerc at gmail.com
Wed Nov 20 10:34:29 PST 2013


As per IRC conversation, addressed and then some in r195260 & r195261.
Thanks for the review.


On Wed, Nov 20, 2013 at 8:34 AM, David Blaikie <dblaikie at gmail.com> wrote:

> 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
>>
>
>
> _______________________________________________
> 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/429327d8/attachment.html>


More information about the llvm-commits mailing list