[llvm] r278600 - [ADT] Add a reserve method to DenseSet as well as an insert() for R-value

[Mehdi Amini via llvm-commits]

Thanks Yaron, reverted in r278603. I didn’t get an email from the bot though?

I’ve been bitten by this in the past with std::make_pair and MSVC introducing an extra move.
Will rework this…

— 
Mehdi




> On Aug 13, 2016, at 1:07 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> 
> following this commit a bot is failing:
> 
> http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux <http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux>
> http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/41483 <http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/41483>
> 
> 
> 2016-08-13 22:40 GMT+03:00 Mehdi Amini via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>>:
> Author: mehdi_amini
> Date: Sat Aug 13 14:40:13 2016
> New Revision: 278600
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=278600&view=rev <http://llvm.org/viewvc/llvm-project?rev=278600&view=rev>
> Log:
> [ADT] Add a reserve method to DenseSet as well as an insert() for R-value
> 
> Modified:
>     llvm/trunk/include/llvm/ADT/DenseSet.h
>     llvm/trunk/unittests/ADT/DenseSetTest.cpp
> 
> Modified: llvm/trunk/include/llvm/ADT/DenseSet.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseSet.h?rev=278600&r1=278599&r2=278600&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseSet.h?rev=278600&r1=278599&r2=278600&view=diff>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/DenseSet.h (original)
> +++ llvm/trunk/include/llvm/ADT/DenseSet.h Sat Aug 13 14:40:13 2016
> @@ -58,6 +58,10 @@ public:
>    /// the Size of the set.
>    void resize(size_t Size) { TheMap.resize(Size); }
> 
> +  /// Grow the DenseSet so that it can contain at least \p NumEntries items
> +  /// before resizing again.
> +  void reserve(size_t Size) { TheMap.reserve(Size); }
> +
>    void clear() {
>      TheMap.clear();
>    }
> @@ -154,6 +158,11 @@ public:
>      return TheMap.insert(std::make_pair(V, Empty));
>    }
> 
> +  std::pair<iterator, bool> insert(ValueT &&V) {
> +    detail::DenseSetEmpty Empty;
> +    return TheMap.insert(std::make_pair(std::move(V), Empty));
> +  }
> +
>    /// Alternative version of insert that uses a different (and possibly less
>    /// expensive) key type.
>    template <typename LookupKeyT>
> 
> Modified: llvm/trunk/unittests/ADT/DenseSetTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/DenseSetTest.cpp?rev=278600&r1=278599&r2=278600&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/DenseSetTest.cpp?rev=278600&r1=278599&r2=278600&view=diff>
> ==============================================================================
> --- llvm/trunk/unittests/ADT/DenseSetTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/DenseSetTest.cpp Sat Aug 13 14:40:13 2016
> @@ -65,4 +65,67 @@ TEST(DenseSetCustomTest, FindAsTest) {
>    EXPECT_TRUE(set.find_as("d") == set.end());
>  }
> 
> +// Simple class that counts how many moves and copy happens when growing a map
> +struct CountCopyAndMove {
> +  static int Move;
> +  static int Copy;
> +  int Value;
> +  CountCopyAndMove(int Value) : Value(Value) {}
> +
> +  CountCopyAndMove(const CountCopyAndMove &) { Copy++; }
> +  CountCopyAndMove &operator=(const CountCopyAndMove &) {
> +    Copy++;
> +    return *this;
> +  }
> +  CountCopyAndMove(CountCopyAndMove &&) { Move++; }
> +  CountCopyAndMove &operator=(const CountCopyAndMove &&) {
> +    Move++;
> +    return *this;
> +  }
> +};
> +int CountCopyAndMove::Copy = 0;
> +int CountCopyAndMove::Move = 0;
> +} // anonymous namespace
> +
> +namespace llvm {
> +// Specialization required to insert a CountCopyAndMove into a DenseSet.
> +template <> struct DenseMapInfo<CountCopyAndMove> {
> +  static inline CountCopyAndMove getEmptyKey() { return CountCopyAndMove(-1); };
> +  static inline CountCopyAndMove getTombstoneKey() {
> +    return CountCopyAndMove(-2);
> +  };
> +  static unsigned getHashValue(const CountCopyAndMove &Val) {
> +    return Val.Value;
> +  }
> +  static bool isEqual(const CountCopyAndMove &LHS,
> +                      const CountCopyAndMove &RHS) {
> +    return LHS.Value == RHS.Value;
> +  }
> +};
> +}
> +
> +namespace {
> +// Make sure reserve actually gives us enough buckets to insert N items
> +// without increasing allocation size.
> +TEST(DenseSetCustomTest, ReserveTest) {
> +  // Test a few different size, 48 is *not* a random choice: we need a value
> +  // that is 2/3 of a power of two to stress the grow() condition, and the power
> +  // of two has to be at least 64 because of minimum size allocation in the
> +  // DenseMa. 66 is a value just above the 64 default init.
> +  for (auto Size : {1, 2, 48, 66}) {
> +    DenseSet<CountCopyAndMove> Set;
> +    Set.reserve(Size);
> +    unsigned MemorySize = Set.getMemorySize();
> +    CountCopyAndMove::Copy = 0;
> +    CountCopyAndMove::Move = 0;
> +    for (int i = 0; i < Size; ++i)
> +      Set.insert(CountCopyAndMove(i));
> +    // Check that we didn't grow
> +    EXPECT_EQ(MemorySize, Set.getMemorySize());
> +    // Check that move was called the expected number of times
> +    EXPECT_EQ(Size, CountCopyAndMove::Move);
> +    // Check that no copy occured
> +    EXPECT_EQ(0, CountCopyAndMove::Copy);
> +  }
> +}
>  }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160813/4b96737b/attachment.html>