[libc++][NFC] Simplify copy and move lowering to memmove a bit

[philnik777]

We've introduced __constexpr_memmove a while ago, which simplified the implementation of the copy and move lowering a bit. This allows us to remove some of the boilerplate.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

We've introduced __constexpr_memmove a while ago, which simplified the implementation of the copy and move lowering a bit. This allows us to remove some of the boilerplate.

---

Full diff: https://github.com/llvm/llvm-project/pull/83574.diff

5 Files Affected:

• (modified) libcxx/include/__algorithm/copy.h (+1-3)
• (modified) libcxx/include/__algorithm/copy_backward.h (+1-3)
• (modified) libcxx/include/__algorithm/copy_move_common.h (+4-29)
• (modified) libcxx/include/__algorithm/move.h (+1-3)
• (modified) libcxx/include/__algorithm/move_backward.h (+1-3)

diff --git a/libcxx/include/__algorithm/copy.h b/libcxx/include/__algorithm/copy.h
index 4c3815405af0cf..caa11df645122b 100644
--- a/libcxx/include/__algorithm/copy.h
+++ b/libcxx/include/__algorithm/copy.h
@@ -94,9 +94,7 @@ struct __copy_loop {
       __local_first = _Traits::__begin(++__segment_iterator);
     }
   }
-};
 
-struct __copy_trivial {
   // At this point, the iterators have been unwrapped so any `contiguous_iterator` has been unwrapped to a pointer.
   template <class _In, class _Out, __enable_if_t<__can_lower_copy_assignment_to_memmove<_In, _Out>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_In*, _Out*>
@@ -108,7 +106,7 @@ struct __copy_trivial {
 template <class _AlgPolicy, class _InIter, class _Sent, class _OutIter>
 pair<_InIter, _OutIter> inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14
 __copy(_InIter __first, _Sent __last, _OutIter __result) {
-  return std::__dispatch_copy_or_move<_AlgPolicy, __copy_loop<_AlgPolicy>, __copy_trivial>(
+  return std::__dispatch_copy_or_move<__copy_loop<_AlgPolicy> >(
       std::move(__first), std::move(__last), std::move(__result));
 }
 
diff --git a/libcxx/include/__algorithm/copy_backward.h b/libcxx/include/__algorithm/copy_backward.h
index 3ec88d8bd5cc3a..cc4701db842bbb 100644
--- a/libcxx/include/__algorithm/copy_backward.h
+++ b/libcxx/include/__algorithm/copy_backward.h
@@ -104,9 +104,7 @@ struct __copy_backward_loop {
       __local_last = _Traits::__end(__segment_iterator);
     }
   }
-};
 
-struct __copy_backward_trivial {
   // At this point, the iterators have been unwrapped so any `contiguous_iterator` has been unwrapped to a pointer.
   template <class _In, class _Out, __enable_if_t<__can_lower_copy_assignment_to_memmove<_In, _Out>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_In*, _Out*>
@@ -118,7 +116,7 @@ struct __copy_backward_trivial {
 template <class _AlgPolicy, class _BidirectionalIterator1, class _Sentinel, class _BidirectionalIterator2>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair<_BidirectionalIterator1, _BidirectionalIterator2>
 __copy_backward(_BidirectionalIterator1 __first, _Sentinel __last, _BidirectionalIterator2 __result) {
-  return std::__dispatch_copy_or_move<_AlgPolicy, __copy_backward_loop<_AlgPolicy>, __copy_backward_trivial>(
+  return std::__dispatch_copy_or_move<__copy_backward_loop<_AlgPolicy> >(
       std::move(__first), std::move(__last), std::move(__result));
 }
 
diff --git a/libcxx/include/__algorithm/copy_move_common.h b/libcxx/include/__algorithm/copy_move_common.h
index 0fc7a5e3cee700..baca6ccace4ce7 100644
--- a/libcxx/include/__algorithm/copy_move_common.h
+++ b/libcxx/include/__algorithm/copy_move_common.h
@@ -81,22 +81,9 @@ __copy_backward_trivial_impl(_In* __first, _In* __last, _Out* __result) {
 
 // Iterator unwrapping and dispatching to the correct overload.
 
-template <class _F1, class _F2>
-struct __overload : _F1, _F2 {
-  using _F1::operator();
-  using _F2::operator();
-};
-
 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,
-                    // Note that sentinels are always copy-constructible.
-                    __enable_if_t< is_copy_constructible<_InIter>::value && is_copy_constructible<_OutIter>::value > >
-    : true_type {};
+struct __can_rewrap
+    : integral_constant<bool, is_copy_constructible<_InIter>::value && is_copy_constructible<_OutIter>::value> {};
 
 template <class _Algorithm,
           class _InIter,
@@ -104,7 +91,7 @@ template <class _Algorithm,
           class _OutIter,
           __enable_if_t<__can_rewrap<_InIter, _Sent, _OutIter>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 pair<_InIter, _OutIter>
-__unwrap_and_dispatch(_InIter __first, _Sent __last, _OutIter __out_first) {
+__dispatch_copy_or_move(_InIter __first, _Sent __last, _OutIter __out_first) {
   auto __range  = std::__unwrap_range(__first, std::move(__last));
   auto __result = _Algorithm()(std::move(__range.first), std::move(__range.second), std::__unwrap_iter(__out_first));
   return std::make_pair(std::__rewrap_range<_Sent>(std::move(__first), std::move(__result.first)),
@@ -117,20 +104,8 @@ template <class _Algorithm,
           class _OutIter,
           __enable_if_t<!__can_rewrap<_InIter, _Sent, _OutIter>::value, int> = 0>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 pair<_InIter, _OutIter>
-__unwrap_and_dispatch(_InIter __first, _Sent __last, _OutIter __out_first) {
-  return _Algorithm()(std::move(__first), std::move(__last), std::move(__out_first));
-}
-
-template <class _AlgPolicy,
-          class _NaiveAlgorithm,
-          class _OptimizedAlgorithm,
-          class _InIter,
-          class _Sent,
-          class _OutIter>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 pair<_InIter, _OutIter>
 __dispatch_copy_or_move(_InIter __first, _Sent __last, _OutIter __out_first) {
-  using _Algorithm = __overload<_NaiveAlgorithm, _OptimizedAlgorithm>;
-  return std::__unwrap_and_dispatch<_Algorithm>(std::move(__first), std::move(__last), std::move(__out_first));
+  return _Algorithm()(std::move(__first), std::move(__last), std::move(__out_first));
 }
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/__algorithm/move.h b/libcxx/include/__algorithm/move.h
index dba6d487fff77e..baefb5b53314e8 100644
--- a/libcxx/include/__algorithm/move.h
+++ b/libcxx/include/__algorithm/move.h
@@ -95,9 +95,7 @@ struct __move_loop {
       __local_first = _Traits::__begin(++__segment_iterator);
     }
   }
-};
 
-struct __move_trivial {
   // At this point, the iterators have been unwrapped so any `contiguous_iterator` has been unwrapped to a pointer.
   template <class _In, class _Out, __enable_if_t<__can_lower_move_assignment_to_memmove<_In, _Out>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_In*, _Out*>
@@ -109,7 +107,7 @@ struct __move_trivial {
 template <class _AlgPolicy, class _InIter, class _Sent, class _OutIter>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_InIter, _OutIter>
 __move(_InIter __first, _Sent __last, _OutIter __result) {
-  return std::__dispatch_copy_or_move<_AlgPolicy, __move_loop<_AlgPolicy>, __move_trivial>(
+  return std::__dispatch_copy_or_move<__move_loop<_AlgPolicy> >(
       std::move(__first), std::move(__last), std::move(__result));
 }
 
diff --git a/libcxx/include/__algorithm/move_backward.h b/libcxx/include/__algorithm/move_backward.h
index aeedf4241dce9c..4f5f2470147830 100644
--- a/libcxx/include/__algorithm/move_backward.h
+++ b/libcxx/include/__algorithm/move_backward.h
@@ -104,9 +104,7 @@ struct __move_backward_loop {
       __local_last = _Traits::__end(--__segment_iterator);
     }
   }
-};
 
-struct __move_backward_trivial {
   // At this point, the iterators have been unwrapped so any `contiguous_iterator` has been unwrapped to a pointer.
   template <class _In, class _Out, __enable_if_t<__can_lower_move_assignment_to_memmove<_In, _Out>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_In*, _Out*>
@@ -122,7 +120,7 @@ __move_backward(_BidirectionalIterator1 __first, _Sentinel __last, _Bidirectiona
                     std::is_copy_constructible<_BidirectionalIterator1>::value,
                 "Iterators must be copy constructible.");
 
-  return std::__dispatch_copy_or_move<_AlgPolicy, __move_backward_loop<_AlgPolicy>, __move_backward_trivial>(
+  return std::__dispatch_copy_or_move<__move_backward_loop<_AlgPolicy> >(
       std::move(__first), std::move(__last), std::move(__result));
 }
 

[ldionne]

I've looked at this patch a couple of times and I don't fully understand it, despite its apparent simplicity.

Why is this called __dispatch_copy_or_move? It seems to me that this doesn't actually perform any dispatching, right?

[var-const]

First, thanks a lot for this refactoring! It removes quite a bit of boilerplate. (And I really hope that we can simplify further using if constexpr in the midterm future)

Let me make sure I understand the refactoring correctly. All the logic for overload resolution is now in the algorithm-specific functor (e.g., inside __copy_loop), therefore copy_move_common.h no longer needs to deal with selecting the right algorithm, which ends up being less code. Is that right? If that's the case, we need to update some of the names -- IMO, they made sense in the earlier state but became a little stale with this refactoring. I initially found the diff pretty confusing, and I think the leftover names are a big reason for that, they're almost misleading at this point:

• __dispatch_copy_or_move -- this function used to be the central point dispatching to the right algorithm (whether it's the "naive" for-loop implementation or the optimized implementation). It no longer really dispatches much. I'd suggest something like __run_with_optional_unwrapping, __maybe_unwrap_and_run, etc.;
• names like __copy_loop and __copy_trivial used to complement each other, describing two alternative implementations of a given algorithm. Now that __copy_trivial is no longer a thing, __copy_loop doesn't really make sense, especially considering that the functor now contains both the loop and non-loop implementations. I'd be fine with something very generic like __copy_func, __copy_impl, etc., but IMO we should drop the _loop part, it's redundant at best and I'd say even misleading at this point.

With that, we would have something like __copy_func that contains 3 mutually-SFINAE'd implementation alternatives and is invoked via a __maybe_unwrap_iters_and_run helper, which IMO is easier to follow. Of course, all of these names can be further improved, the important part is changing the stale names.

Other than the names, pretty much LGTM.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[var-const]

LGTM. Please check with @ldionne before submitting in case he has more feedback.

[ldionne]

LGTM now. The naming changes made it a lot clearer. Thanks!