Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGee6ec2c5f1a5: [libc++][PSTL] Implement std::reduce and std::transform_reduce
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__algorithm/pstl_backends/cpu_backend.h | ||
---|---|---|
19–22 | 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. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/backend.h | ||
31 | inline constexpr? | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h | ||
165 | Do we not have a simd version for this one? | |
libcxx/include/__numeric/pstl_reduce.h | ||
37 | 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. | |
libcxx/include/__numeric/pstl_transform_reduce.h | ||
57 | 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. | |
libcxx/test/std/algorithms/numeric.ops/reduce/pstl.reduce.pass.cpp | ||
28 | Same! | |
50 | Probably a leftover from debugging? | |
78 | We can do with lambdas? | |
libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.binary.pass.cpp | ||
91 | Can be done with lambdas. | |
libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.unary.pass.cpp | ||
26 | Here and below. | |
65 | Lambdas. |
libcxx/include/__algorithm/pstl_backend.h | ||
---|---|---|
61 | _Backend parameter missing | |
98 | _Backend missing here and below. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/serial.h | ||
23 | You will need to also implement this for the std::thread backend. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h | ||
117 | 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. | |
135 | This should also fail with move-only types. If it doesn't, we are missing coverage. | |
libcxx/include/__numeric/pstl_reduce.h | ||
44 | 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. | |
61 | Same comment as above for capturing. | |
libcxx/test/std/algorithms/numeric.ops/transform.reduce/pstl.transform_reduce.binary.pass.cpp | ||
2 | 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. |
_Backend parameter missing