[libc++][PSTL] Implement std::move

[philnik]

Try to fix CI

[ldionne]

libcxx/include/__algorithm/pstl_move.h

33

44

This needs to be updated for exception safety, including tests.

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/pstl.copy.pass.cpp

11 ↗

(On Diff #546908)

You have two tests that still use // REQUIRES: with-pstl, which means they are always disabled. Please fix in a separate patch.

[ldionne]

libcxx/include/__algorithm/pstl_move.h

2

__pstl_move is missing from the documentation in pstl_backend.h

[philnik]

Address comments

[ldionne]

libcxx/include/__algorithm/pstl_move.h

23

Missing <optional> include.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp

2

You need to add a test that __move is noexcept, we said we would add such tests in the PSTL / exception safety patch.

23–27

This is never used.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp

83–86

This is never used.

[philnik]

Address comments

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp

2

I thought we didn't need the test after all, since noexcept isn't necessary on the backend anymore and the death tests caught missing noexcepts quite well (and a std tests seems generally preferable to libc++-specific ones).

[ldionne]

LGTM w/ comments applied.

libcxx/include/__algorithm/pstl_move.h

55

Right now, we perform a move-construction + an assignment for every element (https://godbolt.org/z/vTs9ofrrT).

This can be fixed with -> decltype(auto) on the lambda (https://godbolt.org/z/WKeMdEc3W)

Let's fix this and add a test. This is an important property of std::move!

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp

2

I think you're right, I just got confused with all the recent discussions we had about exception safety in PSTL.

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp

2

Can you add tests with move-only types? This should definitely work since it's std::move.

Can you also take a look at the serial std::move tests to make sure we test it there? We should, but just to be certain.

[philnik]

Rebased

[philnik]

Try to fix CI

libcxx/docs/Status/PSTLPaper.csv

 | `[alg.sort] <https://wg21.link/alg.sort>`_,std::is_sorted,Nikolas Klauser,|Not Started|
 | `[alg.sort] <https://wg21.link/alg.sort>`_,std::is_sorted_until,Nikolas Klauser,|Not Started|
 | `[alg.lex.comparison] <https://wg21.link/alg.lex.comparison>`_,std::lexicographical_compare,Nikolas Klauser,|Not Started|
 | `[alg.min.max] <https://wg21.link/alg.min.max>`_,std::max_element,Nikolas Klauser,|Not Started|
 | `[alg.merge] <https://wg21.link/alg.merge>`_,std::merge,Nikolas Klauser,|Complete|
 | `[alg.min.max] <https://wg21.link/alg.min.max>`_,std::min_element,Nikolas Klauser,|Not Started|
 | `[alg.min.max] <https://wg21.link/alg.min.max>`_,std::minmax_element,Nikolas Klauser,|Not Started|
 | `[mismatch] <https://wg21.link/mismatch>`_,std::mismatch,Nikolas Klauser,|Not Started|
-| `[alg.move] <https://wg21.link/alg.move>`_,std::move,Nikolas Klauser,|In Progress|
+| `[alg.move] <https://wg21.link/alg.move>`_,std::move,Nikolas Klauser,|Complete|
 | `[alg.none.of] <https://wg21.link/alg.none.of>`_,std::none_of,Nikolas Klauser,|Complete|
 | `[alg.nth.element] <https://wg21.link/alg.nth.element>`_,std::nth_element,Nikolas Klauser,|Not Started|
 | `[alg.sort] <https://wg21.link/alg.sort>`_,std::partial_sort,Nikolas Klauser,|Not Started|
 | `[alg.sort] <https://wg21.link/alg.sort>`_,std::partial_sort_copy,Nikolas Klauser,|Not Started|
 | `[alg.partitions] <https://wg21.link/alg.partitions>`_,std::partition,Nikolas Klauser,|Not Started|
 | `[alg.partitions] <https://wg21.link/alg.partitions>`_,std::partition_copy,Nikolas Klauser,|Not Started|
 | `[reduce] <https://wg21.link/reduce>`_,std::reduce,Nikolas Klauser,|Complete|
 | `[alg.remove] <https://wg21.link/alg.remove>`_,std::remove,Nikolas Klauser,|Not Started|

libcxx/include/CMakeLists.txt

set(files
   __algorithm/pstl_count.h
   __algorithm/pstl_fill.h
   __algorithm/pstl_find.h
   __algorithm/pstl_for_each.h
   __algorithm/pstl_frontend_dispatch.h
   __algorithm/pstl_generate.h
   __algorithm/pstl_is_partitioned.h
   __algorithm/pstl_merge.h
+  __algorithm/pstl_move.h
   __algorithm/pstl_replace.h
   __algorithm/pstl_sort.h
   __algorithm/pstl_stable_sort.h
   __algorithm/pstl_transform.h
   __algorithm/push_heap.h
   __algorithm/ranges_adjacent_find.h
   __algorithm/ranges_all_of.h
   __algorithm/ranges_any_of.h

libcxx/include/__algorithm/pstl_backend.h

implemented, all the algorithms will eventually forward to the basis algorithms listed above:
   optional<_OutIterator> __pstl_merge(_Backend,
                                       _Iterator1 __first1,
                                       _Iterator1 __last1,
                                       _Iterator2 __first2,
                                       _Iterator2 __last2,
                                       _OutIterator __result,
                                       _Comp __comp);
 
+  template <class _ExecutionPolicy, class _Iterator, class _OutIterator>
+  optional<_OutIterator> __pstl_move(_Backend, _Iterator __first, _Iterator __last, _OutIterator __result);
+
   template <class _ExecutionPolicy, class _Iterator, class _Tp, class _BinaryOperation>
   optional<_Tp> __pstl_reduce(_Backend, _Iterator __first, _Iterator __last, _Tp __init, _BinaryOperation __op);
 
   temlate <class _ExecutionPolicy, class _Iterator>
   optional<__iter_value_type<_Iterator>> __pstl_reduce(_Backend, _Iterator __first, _Iterator __last);
 
   template <class _ExecuitonPolicy, class _Iterator, class _Tp>
   optional<__iter_diff_t<_Iterator>> __pstl_count(_Backend, _Iterator __first, _Iterator __last, const _Tp& __value);

libcxx/include/__algorithm/pstl_move.h

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___ALGORITHM_PSTL_MOVE_H
+#define _LIBCPP___ALGORITHM_PSTL_MOVE_H
+
+#include <__algorithm/copy_n.h>
+#include <__algorithm/pstl_frontend_dispatch.h>
+#include <__algorithm/pstl_transform.h>
+#include <__config>
+#include <__functional/identity.h>
+#include <__iterator/iterator_traits.h>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/is_constant_evaluated.h>
+#include <__type_traits/is_execution_policy.h>
+#include <__type_traits/is_trivially_copyable.h>
+#include <__type_traits/remove_cvref.h>
+#include <optional>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+#if !defined(_LIBCPP_HAS_NO_INCOMPLETE_PSTL) && _LIBCPP_STD_VER >= 17
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// TODO: Use the std::copy/move shenanigans to forward to std::memmove
+//       Investigate whether we want to still forward to std::transform(policy)
+//       in that case for the execution::par part, or whether we actually want
+//       to run everything serially in that case.
+
+template <class>
+void __pstl_move();
+
+template <class _ExecutionPolicy,
+          class _ForwardIterator,
+          class _ForwardOutIterator,
+          class _RawPolicy                                    = __remove_cvref_t<_ExecutionPolicy>,
+          enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI optional<_ForwardOutIterator>
+__move(_ExecutionPolicy&& __policy,
+       _ForwardIterator&& __first,
+       _ForwardIterator&& __last,
+       _ForwardOutIterator&& __result) noexcept {
+  return std::__pstl_frontend_dispatch(
+      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_move, _RawPolicy),
+      [&__policy](_ForwardIterator __g_first, _ForwardIterator __g_last, _ForwardOutIterator __g_result) {
+        return std::__transform(__policy, __g_first, __g_last, __g_result, [](auto&& __v) { return std::move(__v); });
+      },
+      std::move(__first),
+      std::move(__last),
+      std::move(__result));
+}
+
+template <class _ExecutionPolicy,
+          class _ForwardIterator,
+          class _ForwardOutIterator,
+          class _RawPolicy                                    = __remove_cvref_t<_ExecutionPolicy>,
+          enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0>
+_LIBCPP_HIDE_FROM_ABI _ForwardOutIterator
+move(_ExecutionPolicy&& __policy, _ForwardIterator __first, _ForwardIterator __last, _ForwardOutIterator __result) {
+  auto __res = std::__move(__policy, std::move(__first), std::move(__last), std::move(__result));
+  if (!__res)
+    std::__throw_bad_alloc();
+  return *__res;
+}
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // !defined(_LIBCPP_HAS_NO_INCOMPLETE_PSTL) && _LIBCPP_STD_VER >= 17
+
+#endif // _LIBCPP___ALGORITHM_PSTL_MOVE_H

libcxx/include/algorithm

 #include <__algorithm/pstl_copy.h>
 #include <__algorithm/pstl_count.h>
 #include <__algorithm/pstl_fill.h>
 #include <__algorithm/pstl_find.h>
 #include <__algorithm/pstl_for_each.h>
 #include <__algorithm/pstl_generate.h>
 #include <__algorithm/pstl_is_partitioned.h>
 #include <__algorithm/pstl_merge.h>
+#include <__algorithm/pstl_move.h>
 #include <__algorithm/pstl_replace.h>
 #include <__algorithm/pstl_sort.h>
 #include <__algorithm/pstl_stable_sort.h>
 #include <__algorithm/pstl_transform.h>
 #include <__algorithm/push_heap.h>
 #include <__algorithm/ranges_adjacent_find.h>
 #include <__algorithm/ranges_all_of.h>
 #include <__algorithm/ranges_any_of.h>

libcxx/test/libcxx/algorithms/pstl.robust_against_customization_points_not_working.pass.cpp

 
 template <class, class ForwardIterator, class Size, class Func>
 optional<__empty> __pstl_fill_n(TestBackend, ForwardIterator, Size, Func) {
   assert(!pstl_fill_n_called);
   pstl_fill_n_called = true;
   return __empty{};
 }
 
+bool pstl_move_called = false;
+
+template <class, class ForwardIterator, class Size, class Func>
+ForwardIterator __pstl_move(TestBackend, ForwardIterator, Size, Func) {
+  assert(!pstl_move_called);
+  pstl_move_called = true;
+  return 0;
+}
+
 bool pstl_is_partitioned_called = false;
 
 template <class, class ForwardIterator, class Func>
 optional<bool> __pstl_is_partitioned(TestBackend, ForwardIterator, ForwardIterator, Func) {
   assert(!pstl_is_partitioned_called);
   pstl_is_partitioned_called = true;
   return true;
 }
int main(int, char**) {
   assert(std::pstl_for_each_called);
   (void)std::for_each_n(TestPolicy{}, std::begin(a), std::size(a), pred);
   assert(std::pstl_for_each_n_called);
   (void)std::generate(TestPolicy{}, std::begin(a), std::end(a), pred);
   assert(std::pstl_generate_called);
   (void)std::generate_n(TestPolicy{}, std::begin(a), std::size(a), pred);
   assert(std::pstl_generate_n_called);
   (void)std::is_partitioned(TestPolicy{}, std::begin(a), std::end(a), pred);
-  assert(std::pstl_generate_n_called);
+  assert(std::pstl_is_partitioned_called);
+  (void)std::move(TestPolicy{}, std::begin(a), std::end(a), std::begin(a));
+  assert(std::pstl_move_called);
   (void)std::replace(TestPolicy{}, std::begin(a), std::end(a), 0, 0);
   assert(std::pstl_replace_called);
   (void)std::replace_if(TestPolicy{}, std::begin(a), std::end(a), pred, 0);
   assert(std::pstl_replace_if_called);
   (void)std::replace_copy(TestPolicy{}, std::begin(a), std::end(a), std::begin(a), 0, 0);
   assert(std::pstl_replace_copy_called);
   (void)std::replace_copy_if(TestPolicy{}, std::begin(a), std::end(a), std::begin(a), pred, 0);
   assert(std::pstl_replace_copy_if_called);

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.exception_handling.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+// UNSUPPORTED: no-exceptions
+// REQUIRES: has-unix-headers
+
+// UNSUPPORTED: libcpp-has-no-incomplete-pstl
+
+// check that std::move(ExecutionPolicy) terminates on user-thrown exceptions
+
+#include <algorithm>
+
+#include "check_assertion.h"
+#include "test_execution_policies.h"
+#include "test_iterators.h"
+
+int main(int, char**) {
+  test_execution_policies([](auto&& policy) {
+    EXPECT_STD_TERMINATE([&] {
+      try {
+        int a[] = {1, 2};
+        int b[] = {1, 2};
+        (void)std::move(policy,
+                        util::throw_on_move_iterator(std::begin(a), 1),
+                        util::throw_on_move_iterator(std::end(a), 1),
+                        util::throw_on_move_iterator(std::begin(b), 1));
+      } catch (const util::iterator_error&) {
+        assert(false);
+      }
+      std::terminate(); // make the test pass in case the algorithm didn't move the iterator
+    });
+  });
+}

libcxx/test/std/algorithms/alg.modifying.operations/alg.move/pstl.move.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+
+// UNSUPPORTED: libcpp-has-no-incomplete-pstl
+
+// <algorithm>
+
+// template<class ExecutionPolicy, class ForwardIterator1, class ForwardIterator2>
+//   ForwardIterator2 move(ExecutionPolicy&& policy,
+//                         ForwardIterator1 first, ForwardIterator1 last,
+//                         ForwardIterator2 result);
+
+#include <algorithm>
+#include <vector>
+
+#include "test_macros.h"
+#include "test_execution_policies.h"
+#include "test_iterators.h"
+
+EXECUTION_POLICY_SFINAE_TEST(move);
+
+static_assert(sfinae_test_move<int, int*, int*, int*>);
+static_assert(!sfinae_test_move<std::execution::parallel_policy, int*, int*, int*>);
+
+template <class Iter1, class Iter2>
+struct TestInt {
+  template <class Policy>
+  void operator()(Policy&& policy) {
+    // simple test
+    for (const int size : {0, 1, 2, 100, 350}) {
+      std::vector<int> a(size);
+      for (int i = 0; i != size; ++i)
+        a[i] = i + 1;
+
+      std::vector<int> out(std::size(a));
+      decltype(auto) ret =
+          std::move(policy, Iter1(std::data(a)), Iter1(std::data(a) + std::size(a)), Iter2(std::data(out)));
+      static_assert(std::is_same_v<decltype(ret), Iter2>);
+      assert(base(ret) == std::data(out) + std::size(out));
+      for (int i = 0; i != size; ++i)
+        assert(out[i] == i + 1);
+    }
+  }
+};
+
+struct MovedToTester {
+  bool moved_to   = false;
+  MovedToTester() = default;
+  MovedToTester(MovedToTester&&) {}
+  MovedToTester& operator=(MovedToTester&&) {
+    assert(!moved_to);
+    moved_to = true;
+    return *this;
+  }
+  ~MovedToTester() = default;
+};
+
+template <class Iter1, class Iter2>
+struct TestNonTrivial {
+  template <class Policy>
+  void operator()(Policy&& policy) {
+    // simple test
+    for (const int size : {0, 1, 2, 100, 350}) {
+      std::vector<MovedToTester> a(size);
+
+      std::vector<MovedToTester> out(std::size(a));
+      auto ret = std::move(policy, Iter1(std::data(a)), Iter1(std::data(a) + std::size(a)), Iter2(std::data(out)));
+      assert(base(ret) == std::data(out) + std::size(out));
+      assert(std::all_of(std::begin(out), std::end(out), [](MovedToTester& t) { return t.moved_to; }));
+      assert(std::none_of(std::begin(a), std::end(a), [](MovedToTester& t) { return t.moved_to; }));
+    }
+  }
+};
+
+int main(int, char**) {
+  types::for_each(types::forward_iterator_list<int*>{}, types::apply_type_identity{[](auto v) {
+                    using Iter = typename decltype(v)::type;
+                    types::for_each(
+                        types::forward_iterator_list<int*>{},
+                        TestIteratorWithPolicies< types::partial_instantiation<TestInt, Iter>::template apply>{});
+                  }});
+
+  types::for_each(
+      types::forward_iterator_list<MovedToTester*>{}, types::apply_type_identity{[](auto v) {
+        using Iter = typename decltype(v)::type;
+        types::for_each(
+            types::forward_iterator_list<MovedToTester*>{},
+            TestIteratorWithPolicies< types::partial_instantiation<TestNonTrivial, Iter>::template apply>{});
+      }});
+
+  return 0;
+}