[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:38
+
+    return pair<_InIter, _OutIter>(std::move(__original_last_iter), std::move(__result));
+  }
----------------
var-const wrote:
> philnik wrote:
> > `std::make_pair`?
> I think @huixie90 pointed out previously that `make_pair` creates copies of the arguments in C++03 mode.
Then we should just remove the `#ifndef _LIBCPP_CXX03_LANG`, since we require rvalue support in C++03. See D133013.


================
Comment at: libcxx/include/__algorithm/copy_backward.h:52
 template <class _BidirectionalIterator1, class _BidirectionalIterator2>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _BidirectionalIterator2
-copy_backward(_BidirectionalIterator1 __first, _BidirectionalIterator1 __last, _BidirectionalIterator2 __result) {
-  return std::__copy_backward<_ClassicAlgPolicy>(__first, __last, __result).second;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX20
+_BidirectionalIterator2
----------------
var-const wrote:
> philnik wrote:
> > ?!
> Thanks for noticing! I started working on this patch a while ago, this is probably a rudiment from that time.
Ah OK, so it was just a merge conflict probably.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:34
+  const size_t __n = static_cast<size_t>(__last - __first);
+  std::memmove(__result, __first, __n * sizeof(_Out));
+
----------------
var-const wrote:
> philnik wrote:
> > ldionne wrote:
> > > philnik wrote:
> > > > Why did you get rid of calling `__builtin_memmove` during constant evaluation?
> > > GCC didn't support it, which added a bunch of complexity. So now what we do instead is unwrap the iterators and use the loop version when we're inside constant evaluation. The performance during constant evaluation is less of a concern than performance at runtime.
> > AFAICT the only complexity added by GCC not supporting it is an extra `#ifndef _LIBCPP_COMPILER_GCC`. How does calling `__builtin_memmove` result in worse runtime performance? IMO we shouldn't massively pessimize constant evaluation performance for removing a few lines of code.
> > 
> > For example:
> > ```
> > #include <cstddef>
> > 
> > constexpr void copy_arrays(int* begin, int* end, int* out) {
> > #ifdef USE_MEMMOVE
> >   ::__builtin_memmove(out, begin, static_cast<std::size_t>(end - begin));
> > #else
> >   while (begin != end) {
> >     *out = *begin;
> >     ++begin;
> >     ++out;
> >   }
> > #endif
> > }
> > 
> > constexpr bool use_data() {
> >   int a[100000] = {};
> >   int b[100000] = {};
> >   ::copy_arrays(a, a + 100000, b);
> > 
> >   return true;
> > }
> > 
> > static_assert(use_data());
> > ```
> > Without using `__builtin_memmove` takes 200ms to compile, with `__builtin_memmove` is takes 30ms.
> The original form of this code was (with slight NFC reformatting):
> ```
> if (__libcpp_is_constant_evaluated()
> #ifndef _LIBCPP_COMPILER_GCC
>       && !is_trivially_copyable<_InValueT>::value
> #endif
>      ) {
>     return std::__copy_impl<_InValueT*, _InValueT*, _OutValueT*>(__first, __last, __result);
>   }
>   const size_t __n = static_cast<size_t>(__last - __first);
>   if (__n > 0)
>     ::__builtin_memmove(__result, __first, __n * sizeof(_OutValueT));
>   return std::make_pair(__first + __n, __result + __n);
> ```
> Unless I'm missing something obvious, `__builtin_memmove` was never called during constant evaluation.
To me it looks like it was called when the type `is_trivially_copyable`.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:115
+    is_copy_constructible<_InIter>::value &&
+    is_copy_constructible<_Sent>::value &&
+    is_copy_constructible<_OutIter>::value
----------------
var-const wrote:
> philnik wrote:
> > Sentinels are always copy-constructible.
> This isn't necessarily a sentinel in the C++20-concept sense. This can be instantiated with e.g. a pair of input iterators that aren't copyable.
C++17 input iterators are always copyable and `input_iterator`s can't be their own sentinel if they aren't copyable .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130695