This is an archive of the discontinued LLVM Phabricator instance.

[libc++][PSTL] Implement std::reduce and std::transform_reduce
ClosedPublic

Authored by philnik on May 16 2023, 3:54 PM.

Details

Diff Detail

Event Timeline

philnik created this revision.May 16 2023, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 3:54 PM
Herald added a subscriber: miyuki. · View Herald Transcript
philnik requested review of this revision.May 16 2023, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 3:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 523201.May 17 2023, 3:50 PM

Generate files

philnik updated this revision to Diff 523382.May 18 2023, 7:48 AM

Try to fix CI

ldionne requested changes to this revision.May 18 2023, 9:50 AM
ldionne added inline comments.
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.

This revision now requires changes to proceed.May 18 2023, 9:50 AM
philnik updated this revision to Diff 524883.May 23 2023, 2:21 PM
philnik marked 11 inline comments as done.

Address comments

philnik updated this revision to Diff 525267.May 24 2023, 10:53 AM

Try to fix CI

ldionne added inline comments.May 25 2023, 8:51 AM
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.

philnik updated this revision to Diff 525806.May 25 2023, 2:24 PM
philnik marked 7 inline comments as done.

Address comments

philnik marked an inline comment as done.May 25 2023, 2:35 PM
philnik updated this revision to Diff 526069.May 26 2023, 8:08 AM

Try to fix CI

ldionne accepted this revision.May 26 2023, 11:21 AM
This revision is now accepted and ready to land.May 26 2023, 11:21 AM
philnik updated this revision to Diff 526675.May 30 2023, 9:24 AM

Try to fix CI

philnik updated this revision to Diff 526794.May 30 2023, 2:20 PM

Try to fix CI

philnik updated this revision to Diff 527050.May 31 2023, 8:18 AM

Try to fix CI

philnik updated this revision to Diff 527082.May 31 2023, 9:24 AM

Try to fix CI

philnik updated this revision to Diff 527254.May 31 2023, 6:22 PM

Try to fix CI