[libcxx-commits] [PATCH] D130695: [libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Aug 31 01:53:15 PDT 2022
var-const added inline comments.
================
Comment at: libcxx/include/__algorithm/copy.h:27
-template <class _InIter, class _Sent, class _OutIter>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
-pair<_InIter, _OutIter> __copy_impl(_InIter __first, _Sent __last, _OutIter __result) {
- while (__first != __last) {
- *__result = *__first;
- ++__first;
- ++__result;
+ template <class _InIter, class _Sentinel, class _OutIter>
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
----------------
huixie90 wrote:
> philnik wrote:
> > Why did you rename `_Sent` to `_Sentinel`? AFAIK we settled on `_Sent` for the naming in the ranges algorithms. Same for `_InputIterator` and similar stuff.
> I understand it is not a problem but putting a completely unconstrained overload into the `__overload` struct makes me a bit concerned. I understand it is working right now because the other overload's signature takes a pointer, which is more specific than this one. Having said that, I don't think putting a constraint helps it in any way.
Restored `_Sent`. I didn't think this qualifies as a range algorithm, but now that you mention it, I presume it does (I still don't think having two different name conventions is a good idea).
================
Comment at: libcxx/include/__algorithm/copy.h:27-30
+ template <class _InIter, class _Sentinel, class _OutIter>
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
+ pair<_InIter, _OutIter>
+ operator()(_InIter __first, _Sentinel __last, _OutIter __result) const {
----------------
var-const wrote:
> huixie90 wrote:
> > philnik wrote:
> > > Why did you rename `_Sent` to `_Sentinel`? AFAIK we settled on `_Sent` for the naming in the ranges algorithms. Same for `_InputIterator` and similar stuff.
> > I understand it is not a problem but putting a completely unconstrained overload into the `__overload` struct makes me a bit concerned. I understand it is working right now because the other overload's signature takes a pointer, which is more specific than this one. Having said that, I don't think putting a constraint helps it in any way.
> Restored `_Sent`. I didn't think this qualifies as a range algorithm, but now that you mention it, I presume it does (I still don't think having two different name conventions is a good idea).
Yes, the idea is that pointers are more specialized. I actually had mutually-exclusive constraints in an earlier revision, but relying on pointers seems to provide a nice simplification.
================
Comment at: libcxx/include/__algorithm/copy.h:46
+__copy(_InIter __first, _Sent __last, _OutIter __result) {
+ return std::__dispatch_copy_or_move<__copy_loop<_AlgPolicy>, __copy_trivial<_AlgPolicy> >(
+ std::move(__first), std::move(__last), std::move(__result));
----------------
huixie90 wrote:
> would it be possible to put `__copy_trivial` definition inside this header? AFAICT, the common header only reuses `__copy_trivial_impl` not `__copy_trivial`
I'd rather keep everything related to the `memmove` optimization in one place. One of the goals of this patch was to make these details leak out as little as possible into the actual algorithms.
================
Comment at: libcxx/include/__algorithm/copy_backward.h:38
+
+ return pair<_InIter, _OutIter>(std::move(__original_last_iter), std::move(__result));
+ }
----------------
philnik wrote:
> `std::make_pair`?
I think @huixie90 pointed out previously that `make_pair` creates copies of the arguments in C++03 mode.
================
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
----------------
philnik wrote:
> ?!
Thanks for noticing! I started working on this patch a while ago, this is probably a rudiment from that time.
================
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));
+
----------------
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.
================
Comment at: libcxx/include/__algorithm/copy_move_common.h:109-118
+template <class _InIter, class _Sent, class _OutIter, class = void>
+struct __can_rewrap : false_type {};
+
+template <class _InIter, class _Sent, class _OutIter>
+struct __can_rewrap<_InIter, _Sent, _OutIter, __enable_if_t<
+ is_copy_constructible<_InIter>::value &&
+ is_copy_constructible<_Sent>::value &&
----------------
huixie90 wrote:
> nit: would `_Lazy<_And<...` work here?
Probably. What would be the advantage of using `_Lazy` here?
================
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
----------------
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.
================
Comment at: libcxx/include/__algorithm/move.h:32
+ while (__first != __last) {
+ *__result = _IterOps<_AlgPolicy>::__iter_move(__first);
+ ++__first;
----------------
huixie90 wrote:
> question: if the input is a contiguous iterator pointing to a trivially copyable/moveable type, but the iterator has specialised `iter_move` ( yes it is very contrived), would the `move` dispatch to the overload that unwraps the contiguous iterator to pointer and call `memmove` without going through the specialised `iter_move`?
I think this is a great point, thanks. I'm inclined to leave it as is for this patch because it's a preexistent thing, but I will address it in a follow-up.
================
Comment at: libcxx/include/__algorithm/move_backward.h:44
+template <class _AlgPolicy, class _BidirectionalIterator1, class _Sentinel, class _BidirectionalIterator2>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+pair<_BidirectionalIterator1, _BidirectionalIterator2>
----------------
philnik wrote:
> Why constexpr since 20? Others are marked as constexpr since 14.
This is preexisting and I'd rather not change it in this patch.
================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:36
static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __unwrap(_Iter __i) _NOEXCEPT { return __i; }
+ static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Iter __rewrap(_Iter, _Iter __iter) { return __iter; }
};
----------------
philnik wrote:
> Why did you move this code?
@philnik I find that logically, *re*wrapping has to go after unwrapping, since *re* implies adding something that was previously removed. I originally had more changes to this file, but those went away eventually. I reverted the change from this patch and will submit it separately.
================
Comment at: libcxx/include/__algorithm/unwrap_range.h:34
template <class _Iter, class _Sent>
+requires is_copy_constructible<_Iter>::value && is_copy_constructible<_Sent>::value
struct __unwrap_range_impl {
----------------
philnik wrote:
> AFAICT this isn't correct. Lines 43-45 should handle this case just fine. Also, why not `_v`?
Hmm, I remember adding this to avoid a compilation error, but it looks like that error no longer applies. Removed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130695/new/
https://reviews.llvm.org/D130695
More information about the libcxx-commits
mailing list