Page MenuHomePhabricator

[pstl] Implementation of Grand Central Dispatch backend
Needs ReviewPublic

Authored by nadiasvertex on Feb 19 2022, 6:12 AM.

Details

Reviewers
ldionne
jdoerfert
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

nadiasvertex created this revision.Feb 19 2022, 6:12 AM
nadiasvertex requested review of this revision.Feb 19 2022, 6:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Just some minor nits; the bulk of this is above my pay grade. :)

pstl/include/pstl/internal/gcd/parallel_invoke.h
39

Here and line 34, extraneous whitespace. (Weird spacing throughout, honestly, but that's clang-format for you.)

pstl/include/pstl/internal/gcd/parallel_merge.h
71

Here and throughout, FYI, the code is not ADL-proof, which means it'll have this problem:
https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/
I'm sure this is a pre-existing problem throughout PSTL, though. (We try to avoid it in libc++ proper but I assume nobody's ever made a big deal about it in PSTL.)

pstl/include/pstl/internal/gcd/parallel_stable_sort.h
110

(1) It kinda looks like __pattern_sort (pstl/include/pstl/internal/algorithm_impl.h line 2198) can call this with __xe == __xs. But maybe I'm wrong.
(2) _PSTL_ASSERT?

Add the __tag parameter, and address comments about ADL and a bad assertion.

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2022, 7:57 AM
nadiasvertex marked 2 inline comments as done.Mar 12 2022, 7:58 AM
nadiasvertex added inline comments.
pstl/include/pstl/internal/gcd/parallel_invoke.h
39

I have re-run clang-format on the code. Not sure what else to do about it.

pstl/include/pstl/internal/gcd/parallel_merge.h
71

Good point. I've tried to fix this everywhere, in the same way that the OpenMP backend fixes it.

pstl/include/pstl/internal/gcd/parallel_stable_sort.h
110

Good catch, thank you!

nadiasvertex marked 2 inline comments as done.May 11 2022, 7:46 AM
nadiasvertex added a subscriber: MikeDvorskiy.

@MikeDvorskiy could you look at this patch, please? Thank you!

philnik added a subscriber: philnik.Jun 1 2022, 6:51 AM
philnik added inline comments.
pstl/include/pstl/internal/gcd/parallel_invoke.h
29

Is there a reason you use ::std here instead of std?

pstl/include/pstl/internal/gcd/parallel_stable_partial_sort.h
29

This isn't ADL-proof.

pstl/include/pstl/internal/gcd/util.h
104

Also not ADL-proof

pstl/include/pstl/internal/parallel_backend_gcd.h
14–16

These comments seem weird to me.