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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backend.h:19
   void __parallel_for(_RandomAccessIterator __first, _RandomAccessIterator __last, _Functor __func);
 
   // Cancel the execution of other jobs - they aren't needed anymore
----------------
You should document the new customization point here for the CPU backend.

And you should also document the new "customization points" in the general backend.


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/backend.h:29
 
+const size_t __lane_size = 64;
+
----------------
`inline constexpr`?


================
Comment at: libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h:164
+    });
+  } else {
+    return std::transform_reduce(__first, __last, __init, __reduce, __transform);
----------------
Do we not have a simd version for this one?


================
Comment at: libcxx/include/__numeric/pstl_reduce.h:36
+       _BinaryOperation __op = {}) {
+  return std::transform_reduce(__policy, __first, __last, __init, __op, __identity{});
+}
----------------
We should be dispatching here and below.

I think we should add a test that allows catching that. It would also give us confidence that our dispatching mechanism works as intended, which we don't know right now:

```
struct TestPolicy { };
struct TestBackendTag { };

template <>
struct std::__select_backend<TestPolicy> { using type = TestBackendTag; };
```

It would also confirm that we're able to add new implementation-defined policies if need be, which is clearly one of the design points of our dispatching mechanism.


================
Comment at: libcxx/include/__numeric/pstl_transform_reduce.h:56
+
+// TODO: Should this get a customization point?
+template <class _ExecutionPolicy,
----------------
I don't think so, because we are able to detect that this has been called in the generic version via the operation traits. I would leave a comment explaining why this one doesn't get a dispatch.


================
Comment at: libcxx/test/std/algorithms/numeric.ops/reduce/pstl.reduce.pass.cpp:27
+
+#include <iostream>
+
----------------
Same!


================
Comment at: libcxx/test/std/algorithms/numeric.ops/reduce/pstl.reduce.pass.cpp:49
+        static_assert(std::is_same_v<decltype(ret), int>);
+        std::cout << ret << '\n';
+        assert(ret == expected);
----------------
Probably a leftover from debugging?


================
Comment at: libcxx/test/std/algorithms/numeric.ops/reduce/pstl.reduce.pass.cpp:77
+int main(int, char**) {
+  types::for_each(types::forward_iterator_list<int*>{}, TestIterators{});
+
----------------
We can do with lambdas?


================
Comment at: libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.binary.pass.cpp:90
+int main(int, char**) {
+  types::for_each(types::forward_iterator_list<int*>{}, TestIterators{});
+
----------------
Can be done with lambdas.


================
Comment at: libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.unary.pass.cpp:25
+
+#include <iostream>
+
----------------
Here and below.


================
Comment at: libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.unary.pass.cpp:64
+int main(int, char**) {
+  types::for_each(types::forward_iterator_list<int*>{}, TestIterators{});
+
----------------
Lambdas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150736