This is an archive of the discontinued LLVM Phabricator instance.

[pstl] Implementation of Grand Central Dispatch backend
Needs ReviewPublic

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

Details

Reviewers
jdoerfert

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
38

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
70

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
109

(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
38

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
70

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
109

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
30

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

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

This isn't ADL-proof.

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

Also not ADL-proof

pstl/include/pstl/internal/parallel_backend_gcd.h
15–17

These comments seem weird to me.

nadiasvertex marked 2 inline comments as done.Jul 10 2022, 9:16 AM
nadiasvertex added inline comments.
pstl/include/pstl/internal/gcd/parallel_invoke.h
30

Nope.

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

Okay. What do you suggest?

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

It's a C-function. What do you suggest?

pstl/include/pstl/internal/parallel_backend_gcd.h
15–17

Ok.

nadiasvertex marked 2 inline comments as done.

[pstl] Fix minor nit

philnik added inline comments.Jul 10 2022, 9:21 AM
pstl/include/pstl/internal/gcd/parallel_stable_partial_sort.h
30

Oh sorry, never mind. I didn't see that the function was an argument.

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

Just put :: in front of it.

[pstl] More ADL guards

nadiasvertex marked 5 inline comments as done.Jul 10 2022, 11:03 AM

Fixed some additional comments.

No. I suppose it's been so long that it won't apply cleanly. I would really like some help getting a builder to turn this on for macOS automatically so that it can run as part of the normal test suite. I can try to carve out some time to rebase it and re-test it. I'm not really sure what anyone is waiting for...

No. I suppose it's been so long that it won't apply cleanly. I would really like some help getting a builder to turn this on for macOS automatically so that it can run as part of the normal test suite. I can try to carve out some time to rebase it and re-test it. I'm not really sure what anyone is waiting for...

The problem is that the PSTL isn't tested currently, and because of that bit-rotted. We're trying to get it working properly, but it's not exactly a high priority.

ldionne removed a reviewer: Restricted Project.Sep 6 2023, 4:51 PM
ldionne removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2023, 4:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: jplehr. · View Herald Transcript
ldionne removed a reviewer: Restricted Project.Sep 6 2023, 4:52 PM
ldionne removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2023, 4:52 PM
ldionne resigned from this revision.Sep 6 2023, 4:53 PM

[Github PR transition cleanup]

Removed libc++ from this review since we no longer use the code in <monorepo-root>/pstl.

ldionne removed a reviewer: ldionne.Sep 6 2023, 4:53 PM
ldionne removed a subscriber: ldionne.