[libcxx-commits] [PATCH] D150736: [libc++][PSTL] Implement std::reduce and std::transform_reduce

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/pstl_backend.h:61
+  template <class _ExecutionPolicy, class _Iterator, class _Tp, class _BinaryOperation, class _UnaryOperation>
+  _Tp __pstl_transform_reduce(_Iterator __first,
+                              _Iterator __last,
----------------
`_Backend` parameter missing


================
Comment at: libcxx/include/__algorithm/pstl_backend.h:98
+  template <class _ExecutionPolicy, class _Iterator, class _Tp, class _BinaryOperation>
+  _Tp __pstl_reduce(_Iterator __first, _Iterator __last, _Tp __init, _BinaryOperation __op);
+
----------------
`_Backend` missing here and below.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/serial.h:23
 
 namespace __par_backend {
 inline namespace __serial_cpu_backend {
----------------
You will need to also implement this for the `std::thread` backend.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h:116
+          },
+          __init,
+          __reduce,
----------------
Once you add a test for move-only types, I would assume that this line is going to fail cause you're missing a `std::move`.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h:134
+    return std::__simd_transform_reduce(
+        __last1 - __first1, __init, __reduce, [&](__iter_diff_t<_ForwardIterator1> __i) {
+          return __transform(__first1[__i], __first2[__i]);
----------------
This should also fail with move-only types. If it doesn't, we are missing coverage.


================
Comment at: libcxx/include/__numeric/pstl_reduce.h:43
+      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_reduce),
+      [&](_ForwardIterator __g_first, _ForwardIterator __g_last, _Tp __g_init, _BinaryOperation __g_op) {
+        return std::transform_reduce(__policy, __g_first, __g_last, __g_init, __g_op, __identity{});
----------------
Can you capture the `__policy` by name explicitly? Otherwise one might wonder whether you're capturing a variable that you're moving in the lines below.


================
Comment at: libcxx/include/__numeric/pstl_reduce.h:60
+      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_reduce),
+      [&](_ForwardIterator __g_first, _ForwardIterator __g_last) {
+        return std::reduce(__policy, __g_first, __g_last, __iter_value_type<_ForwardIterator>());
----------------
Same comment as above for capturing.


================
Comment at: libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.binary.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you please take a look at the stuff we test for the serial version of these algorithms and make sure we have coverage for that? For example, `std::transform_reduce` has tests for move-only types but we don't seem to test that here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150736