Page MenuHomePhabricator

[libc++] Add a CI configuration where we test the PSTL integration
Needs ReviewPublic

Authored by philnik on May 26 2021, 12:49 PM.

Details

Reviewers
ldionne
rarutyun
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Summary

This makes sure that the PSTL integration doesn't regress. Eventually,
when we're ready to ship PSTL, this configuration will be enabled by
default in all builds and we can drop this additional CI configuration.

Diff Detail

Event Timeline

ldionne created this revision.May 26 2021, 12:49 PM
ldionne requested review of this revision.May 26 2021, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 12:49 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
libcxx/include/functional
3032–3037 ↗(On Diff #348057)

This change is necessary for the PSTL tests to succeed.

I am fairly certain that we can reduce that failure sufficiently to show that we actually have a bug in std::not_fn. @zoecarver would you be able to take a look since you implemented __perfect_forward_impl?

cjdb added a subscriber: cjdb.May 26 2021, 1:06 PM
cjdb added inline comments.
libcxx/include/functional
3032–3037 ↗(On Diff #348057)

Do we need to add any tests to account for this change?

ldionne added inline comments.May 27 2021, 9:08 AM
libcxx/include/functional
3032–3037 ↗(On Diff #348057)

Yes, I was asking @zoecarver to look into the failure and add tests for std::not_fn and std::bind_front based on the findings!

@ldionne,

I am trying to update this review to continue enabling of PSTL CI with necessary modifications of the pipeline and fixes in PSTL source/tests, but I cannot do that. Seems like I don't have enough permissions to update the review I am not the author of. I am using web interface and when I am pressing "Update diff" I can only create the new revision and don't have an option to update the current one. Probably it's because I don't have direct Phabricator account (I have been using the GitHub one from the very beginning) or probably the reason is different. Together with @MikeDvorskiy we tried to do the same from his account (using web interface as well) and everything works just fine.

Could you please help me with the issue?

@Quuxplusone, @cjdb, @Mordante, if you guys know the answer on the question above, your help is appreciated.

P.S. That problem was observed even earlier so, it seems I've never had such permission

@rarutyun you first need to commandeer this review, which is in the "Add Action..." drop down.
(AFAIK that requires no special permissions.)

rarutyun commandeered this revision.Feb 28 2022, 10:33 AM
rarutyun added a reviewer: ldionne.

@rarutyun you first need to commandeer this review, which is in the "Add Action..." drop down.
(AFAIK that requires no special permissions.)

Thanks for the suggestion. It works. I am wondering why it works just like that (without commandeering the review) from @MikeDvorskiy account but anyway, thanks for you help.

rarutyun updated this revision to Diff 413433.Mar 7 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 4:46 AM
cjdb removed a subscriber: cjdb.Mar 7 2022, 1:47 PM
rarutyun updated this revision to Diff 419476.Mar 31 2022, 8:31 AM

@rarutyun If you're OK with it I'd like to continue to work on this. I've got a local copy where the CI runs properly. (and 9 tests fail)

@rarutyun If you're OK with it I'd like to continue to work on this. I've got a local copy where the CI runs properly. (and 9 tests fail)

Yes, please feel free to commandeer.

philnik commandeered this revision.Jul 22 2022, 6:42 PM
philnik added a reviewer: rarutyun.
philnik updated this revision to Diff 447021.Jul 22 2022, 6:43 PM

This should be a working PSTL bot

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

How realistic is it that this still makes it into LLVM 15? I'd really like to start testing the pstl in our ecosystem a bit, happy to file bug reports too if so, but I'd need a way to be able to build it...

philnik updated this revision to Diff 448179.Jul 27 2022, 3:04 PM
  • Fix CMake
ldionne requested changes to this revision.Jul 28 2022, 8:31 AM
ldionne added a subscriber: rodgert.

How realistic is it that this still makes it into LLVM 15? I'd really like to start testing the pstl in our ecosystem a bit, happy to file bug reports too if so, but I'd need a way to be able to build it...

Unfortunately, I don't think we'll cherry-pick this to LLVM 15. Proper integration requires additional work, and that would be too much for LLVM 15 since we've already branched. We focused on Ranges and Format for LLVM 15. This should make it for LLVM 16 though.

libcxx/include/algorithm
1673

I think the PSTL should be more tightly integrated with libc++, and we should probably be including libc++'s internal headers directly from it to break these kinds of circular dependencies.

This is going to be non-trivial though because there was an intent to share the PSTL between libc++ and libstdc++ -- I don't know whether that is still desired, and we may need to revisit this if we want to ease the integration between libc++ and PSTL.

@rodgert What is the current state of PSTL within libstdc++? How do you integrate it nowadays? Back in the days, I think you had a script that basically copied the sources to libstdc++ -- if that's still the case, do you have plans to keep it up to date, and can you link to that script so we can take a look at how the integration works?

libcxx/test/configs/llvm-libc++-with-pstl.cfg.in
11

We should be installing the PSTL headers to include/c++/v1 so that we find them alongside other libc++ headers out-of-the-box, and users don't need to add an additional include.

I suspect that means we'll have to start running the tests against a fake-installation of libc++/libc++abi/PSTL/whatever, which is what we should do anyways because it's closer to what users get. I've been meaning to do that for a while, I'll see if I can do this in the next few weeks.

pstl/include/pstl/internal/utils.h
27

This bit (and a few others) can land separately.

This revision now requires changes to proceed.Jul 28 2022, 8:31 AM
philnik updated this revision to Diff 453268.Aug 17 2022, 6:08 AM
philnik marked an inline comment as done.
  • Address comments
libcxx/include/algorithm
1673

If we want to do that it should probably be in a separate PR.

rodgert added inline comments.Aug 19 2022, 4:40 PM
libcxx/include/algorithm
1673

@rodgert What is the current state of PSTL within libstdc++?

I pulled a new version in 2020 and have not rebased since. I had been waiting on the tag dispatch rewrite before considering the process again. I also not seen any progress on that since January, so maybe I will rethink matters if that continues.

I think any changes to include libc++'s internal headers would be detrimental to the continued to sharing of this codebase. I deliberately have not done that so far with libstdc++ (or I maintain those changes downstream only) because that would almost certainly introduce massive and likely unresolvable breakage for libc++.