[libcxx-commits] [PATCH] D147741: [libc++, std::vector] call the optimized version of __uninitialized_allocator_copy for trivial types

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

The implementation itself looks good, but the benchmarks need some work.



================
Comment at: libcxx/benchmarks/ContainerBenchmarks.h:30-34
+__attribute__((noinline))
+int CopyContainer(Container v1) {
+    auto v = v1;
+    return 0;
+}
----------------
It looks like this doesn't do what it's supposed to: https://godbolt.org/z/vdqGrcM3P. At least with your new version, the container never get copied. You have to add `DoNotOptimize(v)` or something similar, so the optimizer doesn't just remove the code. Then you can also drop the `__attribute__((noinline))`. Same applies to `CopyContainerInto`. I would just remove the functions and make the copy construction and assignment inline.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:593
+template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 _Iter2 __uninitialized_allocator_copy(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1, _Iter2 __first2) {
+    return std::__rewrap_iter(__first2, std::__uninitialized_allocator_copy_impl(__alloc, std::__unwrap_iter(__first1), std::__unwrap_iter(__last1), std::__unwrap_iter(__first2)));
----------------
You're missing a `_LIBCPP_HIDE_FROM_ABI`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147741/new/

https://reviews.llvm.org/D147741