[libc++] Integrate the PSTL into libc++

[ldionne]

Summary

This commit allows specifying LIBCXX_ENABLE_PARALLEL_ALGORITHMS when configuring libc++ in CMake. When that option is enabled, libc++ will assume that the PSTL can be found somewhere on the CMake module path, and it will provide the C++17 parallel algorithms based on the PSTL (that is assumed to be available).

The commit also adds support for running the PSTL tests as part of the libc++ test suite.

[EricWF]

How does this include pattern work with modules? It's important the libc++ headers maintain their modularity.

libcxx/include/algorithm

5791 ↗

(On Diff #194378)

We normally use _ONE_UNDERSCORE_UPPER or __two_underscore_lower

[EricWF]

@ldionne

There's a circular dependency between libc++ and libc++abi, which causes normal CMake dependencies not to work for PSTL. Specifically, libc++abi will complain about not having access to pstl/internal/XXX when building.

Would this be fixed if libc++ and libc++abi configured as a single project instead of two?

[rodgert]

In D60480#1460300, @ldionne wrote:

The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.

Concrete changes I think we need to make in pstl:

• Rename all the files to double underscores.

This isn't a libstdc++ convention, but I'm not opposed.

• Move everything that's meant to be included directly by standard library implementations out of internal/, since that doesn't make sense. It's not internal, it's the API of PSTL (which is meant to be consumed by standard libraries).

I would like to maintain a clear separation between the bits that implement PSTL and the glue bits that tie into a given library. Currently everything in 'internal/' goes to libstdc++v3/include/pstl when I integrate with libstdc++v3. I don't want to be left guessing what those files are, I want them to move as a cohesive unit into the library's implementation detail. We can name whatever we want.

• We could provide the glue definitions and declarations as .ipp files that are meant to be included by standard libraries as-is. The Standard library could then define stuff like the namespace in which those declarations are injected, etc.

I would be interested in exploring this, subject to the caveat that whole of what's currently in 'internal' is an obvious package of related implementation detail that I can poke into libstdc++ as a unit.

I also suggest:

• Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

[rodgert]

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

[EricWF]

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

[EricWF]

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

But also thank you very much for your work :-)

[ldionne]

In D60480#1460728, @rodgert wrote:

In D60480#1460300, @ldionne wrote:

The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.

Concrete changes I think we need to make in pstl:

• Rename all the files to double underscores.

This isn't a libstdc++ convention, but I'm not opposed.

The problem with not doing that is that our headers could collide with user's header files.

• Move everything that's meant to be included directly by standard library implementations out of internal/, since that doesn't make sense. It's not internal, it's the API of PSTL (which is meant to be consumed by standard libraries).

I would like to maintain a clear separation between the bits that implement PSTL and the glue bits that tie into a given library. Currently everything in 'internal/' goes to libstdc++v3/include/pstl when I integrate with libstdc++v3. I don't want to be left guessing what those files are, I want them to move as a cohesive unit into the library's implementation detail. We can name whatever we want.

Right, I agree. What I meant (but that's not what I wrote) is that the glue headers and everything that is expected to be used from standard libraries should be out of internal. The backend and the config stuff can (and should) stay in internal. Do we agree here?

• We could provide the glue definitions and declarations as .ipp files that are meant to be included by standard libraries as-is. The Standard library could then define stuff like the namespace in which those declarations are injected, etc.

I would be interested in exploring this, subject to the caveat that whole of what's currently in 'internal' is an obvious package of related implementation detail that I can poke into libstdc++ as a unit.

Well, so only the glue part would be .ipps. And I don't care if they're headers or .ipps, the idea is just that I want to reverse the model from "libc++ includes pstl headers that define the parallel algorithms" to "pstl provides declarations that a standard library can copy-paste into their own headers via #include".

I also suggest:

• Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in pstl/test that would define these headers, and that's what we would run the tests against.

Lastly, about the __PSTL_XXX vs _PSTL_XXX: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all __PSTL_XXX macros to be _PSTL_XXX to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

[rodgert]

In D60480#1461369, @ldionne wrote:

In D60480#1460728, @rodgert wrote:

In D60480#1460300, @ldionne wrote:

The purpose of this review is to expose how I plan to integrate the parallel STL into libc++, and to discuss some changes that are desirable in pstl for this to be possible. This is a work in progress, obviously, and it's meant as a concrete support for having discussions.

Concrete changes I think we need to make in pstl:

• Rename all the files to double underscores.

This isn't a libstdc++ convention, but I'm not opposed.

The problem with not doing that is that our headers could collide with user's header files.

I understand the complication. libstdc++ follows the convention of everything 'internal' being in bits/ and then the std headers #include <bits/foo.h>, and this is something that's
on my list to address for PSTL (as what is currently in libstdc++ could still clash with user headers). I don't have any real objection to the underscore naming convention. I can mechanically
translate them to #include <pstl/non_underscore_named_file.h>.

• Move everything that's meant to be included directly by standard library implementations out of internal/, since that doesn't make sense. It's not internal, it's the API of PSTL (which is meant to be consumed by standard libraries).

I would like to maintain a clear separation between the bits that implement PSTL and the glue bits that tie into a given library. Currently everything in 'internal/' goes to libstdc++v3/include/pstl when I integrate with libstdc++v3. I don't want to be left guessing what those files are, I want them to move as a cohesive unit into the library's implementation detail. We can name whatever we want.

Right, I agree. What I meant (but that's not what I wrote) is that the glue headers and everything that is expected to be used from standard libraries should be out of internal. The backend and the config stuff can (and should) stay in internal. Do we agree here?

Ok, yeah, I'm good with that.

• We could provide the glue definitions and declarations as .ipp files that are meant to be included by standard libraries as-is. The Standard library could then define stuff like the namespace in which those declarations are injected, etc.

I would be interested in exploring this, subject to the caveat that whole of what's currently in 'internal' is an obvious package of related implementation detail that I can poke into libstdc++ as a unit.

Well, so only the glue part would be .ipps. And I don't care if they're headers or .ipps, the idea is just that I want to reverse the model from "libc++ includes pstl headers that define the parallel algorithms" to "pstl provides declarations that a standard library can copy-paste into their own headers via #include".

I'm also good with this.

I also suggest:

• Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in pstl/test that would define these headers, and that's what we would run the tests against.

That would work for me...AND...we can probably remove the per-test #ifdef for the standalone checks, centralize it in the dummy standard headers in 'pstl/test' which would be much cleaner than what is currently there.

Lastly, about the __PSTL_XXX vs _PSTL_XXX: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all __PSTL_XXX macros to be _PSTL_XXX to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

JF and I discussed possibly some tooling for this sort of thing. I'm still interested in looking at that, I just don't see a lot of value in changing it by hand just to comply with conflicting conventions between standard library implementations.

[ldionne]

In D60480#1461416, @rodgert wrote:

In D60480#1461369, @ldionne wrote:

In D60480#1460728, @rodgert wrote:

In D60480#1460300, @ldionne wrote:

I also suggest:

• Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in pstl/test that would define these headers, and that's what we would run the tests against.

That would work for me...AND...we can probably remove the per-test #ifdef for the standalone checks, centralize it in the dummy standard headers in 'pstl/test' which would be much cleaner than what is currently there.

Ok, I'll submit a patch for that.

Lastly, about the __PSTL_XXX vs _PSTL_XXX: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all __PSTL_XXX macros to be _PSTL_XXX to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

JF and I discussed possibly some tooling for this sort of thing. I'm still interested in looking at that, I just don't see a lot of value in changing it by hand just to comply with conflicting conventions between standard library implementations.

I don't think the conventions used in PSTL should conform to the coding standard in any one standard library implementation. It's a library on its own. I think it should conform to its own coding standards, and in this case, I think those coding standard should be that of LLVM. Does this point of view make sense?

[jfb]

In D60480#1460733, @EricWF wrote:

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

[EricWF]

I take back my comments about libc++ style. I didn't understand what was going on here. my apologies.

My only remaining concern with this patch is that our headers stay modular and continue to work with clangs modules.

[rodgert]

In D60480#1461436, @ldionne wrote:

In D60480#1461416, @rodgert wrote:

In D60480#1461369, @ldionne wrote:

In D60480#1460728, @rodgert wrote:

In D60480#1460300, @ldionne wrote:

I also suggest:

• Removing the <algorithm> & friends headers from PSTL itself. Those don't belong there, they belong with the standard library.

WDYT @rodgert ?

If we remove them, then we sort of lose the ability to test pstl outside of standard library, no? I'm not going to be testing or packaging libc++ most likely, but I am going to be contributing changes here and running tests within the context of libstdc++. Having the ability to test changes to the library independent of any integration with another standard library is still useful for me.

I believe those headers should be provided by a dummy standard library that we use only to run the tests. Basically we'd have a dummy standard library in pstl/test that would define these headers, and that's what we would run the tests against.

That would work for me...AND...we can probably remove the per-test #ifdef for the standalone checks, centralize it in the dummy standard headers in 'pstl/test' which would be much cleaner than what is currently there.

Ok, I'll submit a patch for that.

Lastly, about the __PSTL_XXX vs _PSTL_XXX: This patch is using what's in PSTL today, that's all. As a separate change, I thought about changing all __PSTL_XXX macros to be _PSTL_XXX to follow the more common convention (underscore-capital or double-underscore-lowercase), but this is a change separate from the integration into libc++.

JF and I discussed possibly some tooling for this sort of thing. I'm still interested in looking at that, I just don't see a lot of value in changing it by hand just to comply with conflicting conventions between standard library implementations.

I don't think the conventions used in PSTL should conform to the coding standard in any one standard library implementation. It's a library on its own. I think it should conform to its own coding standards, and in this case, I think those coding standard should be that of LLVM. Does this point of view make sense?

At the end of the day, I'd rather invest in the tooling to make a uniform convention, than have to do this exercise again...ever. The more we can agree a consistent set of rules (ideally enforced by clang-format) for this library the better.

[rodgert]

In D60480#1461555, @jfb wrote:

In D60480#1460733, @EricWF wrote:

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

The process of applying this kind of change is very error prone. I still have a patch coming that fixes a few cases I missed. That's the bigger argument IMO for tooling than the specific format.

[ldionne]

In D60480#1461586, @rodgert wrote:

In D60480#1461555, @jfb wrote:

In D60480#1460733, @EricWF wrote:

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

The process of applying this kind of change is very error prone. I still have a patch coming that fixes a few cases I missed. That's the bigger argument IMO for tooling than the specific format.

I think I'm missing yours and JF's point. I don't understand why you need any tooling. All I'm saying is that PSTL being under the LLVM umbrella should conform to the usual LLVM code style, which used single underscore + capitals for macros unless I'm mistaken. libstdc++ and libc++ wouldn't apply any code transformation on the PSTL sources after that, it would just use it as a separate library and there wouldn't be very many PSTL macros used inside libstdc++/libc++. Only those macros that are part of the PSTL's interface, and yes, those would be using the PSTL's naming convention.

IOW, this is something that we simply missed in the initial check-in.

[rodgert]

In D60480#1461587, @ldionne wrote:

In D60480#1461586, @rodgert wrote:

In D60480#1461555, @jfb wrote:

In D60480#1460733, @EricWF wrote:

In D60480#1460729, @rodgert wrote:

Not sure what is up with phabricator, but it won't let me respond inline to EricWF, specifically -

__FIRST_MOVER_ADVANTAGE

libc++ and libstdc++ have different 'uglification' protocols. As I said to mclow early on in this process, If I'm the one that has to do the grunt work of *all* of the uglification (very much has been case, BTW), it's going to follow libstdc++ convention as much as is possible.

libc++ code should follow libc++ conventions.

Just to be clear: PSTL is neither libc++ nor libstdc++. Louis kinda said so above, and Tom is making that point too, but I want to reiterate it. It's overall a silly bikeshedding point, but it's critical to keep everyone happy. PSTL isn't more "ours" than "theirs" (or rather, it's "ours" in that we're all in the same C++ team). This point was critical in getting PSTL to be a shared resource in the first place, and I don't want to walk back this commitment.

As Tom mentions above, we've talked about having tooling to massage PSTL to libc++'s and libstdc++'s liking on integration. Without volunteers to write said tooling, we should just accept slight ache when integrating PSTL, as we see here when integrating it into libc++.

The process of applying this kind of change is very error prone. I still have a patch coming that fixes a few cases I missed. That's the bigger argument IMO for tooling than the specific format.

I think I'm missing yours and JF's point. I don't understand why you need any tooling. All I'm saying is that PSTL being under the LLVM umbrella should conform to the usual LLVM code style, which used single underscore + capitals for macros unless I'm mistaken. libstdc++ and libc++ wouldn't apply any code transformation on the PSTL sources after that, it would just use it as a separate library and there wouldn't be very many PSTL macros used inside libstdc++/libc++. Only those macros that are part of the PSTL's interface, and yes, those would be using the PSTL's naming convention.

IOW, this is something that we simply missed in the initial check-in.

So, I hear you volunteering to apply those changes :)

[ldionne]

In D60480#1461617, @rodgert wrote:

So, I hear you volunteering to apply those changes :)

Yes, it's never been my intention to push that work onto someone else. I just wanted to make sure we agreed before going ahead and doing it. https://reviews.llvm.org/D60521

[ldionne]

Non-draft version of the patch.

[EricWF]

libcxx/include/algorithm

5790 ↗

(On Diff #194378)

I really really really dislike this pattern. We shouldn't do it.
I don't think this is how you should structure a library.

Are there other alternatives?

libcxxabi/src/CMakeLists.txt

149 ↗

(On Diff #205866)

Why does libc++abi depend on parallel algorithms? That seems backwards?

pstl/test/pstl/header_inclusion_order_algorithm_0.pass.cpp

11 ↗

(On Diff #205866)

Does every test under pstl/test/pstl require "parallel-algorithms"? Because if so there are easier ways to do this.

[ldionne]

Please note that this review is really about integrating pstl into libc++, not pstl itself. I know there are changed needed before pstl is ready for prime time (and LIBCXX_ENABLE_PARALLEL_ALGORITHMS becomes ON by default).

libcxx/include/algorithm

5790 ↗

(On Diff #194378)

I dislike it too. The only alternative I can think of is to unconditionally include the definitions, however this may impose a compile-time hit on everyone that's not using the parallel algorithms.

5791 ↗

(On Diff #194378)

For file names? Please disregard pstl file names for the time being. The goal of this review is to check-in the integration between libc++ and pstl, but it's understood that pstl's structure is going to change in the future. I'm just trying to integrate early to better see how changes in pstl will end up impacting libc++.

libcxxabi/src/CMakeLists.txt

149 ↗

(On Diff #205866)

Yes, it is. It's because libc++abi includes headers from libc++ that pull in the parallel algorithms when they're enabled. So this is needed to support building libc++abi against a libc++ that has the parallel algorithms enabled.

pstl/test/pstl/header_inclusion_order_algorithm_0.pass.cpp

11 ↗

(On Diff #205866)

Yes, they all do. I could also use a lit.local.cfg if that's what you're thinking about?

[dexonsmith]

libcxx/include/algorithm

5790 ↗

(On Diff #194378)

@bruno, will this pattern cause a problem for Clang modules (esp. with local submodule visibility)?

[EricWF]

libcxxabi/src/CMakeLists.txt

149 ↗

(On Diff #205866)

This is a layering violation and creates a circular dependency. I don't think we can or should structure the project like this.

[rsmith]

libcxx/include/algorithm

5790 ↗

(On Diff #194378)

This won't work with modules (neither Clang modules with local submodule visibility, nor C++20 modules). We will build <algorithm> in a context where <execution> has not been included, and vice versa, and neither of them will contain definitions of the parallel algorithms. More fundamentally, we should not be trying to create a situation where we have two headers and including both of them together provides more content than the union of the content provided by including the two separately -- that's bad header design and inevitably will lead to problems.

One possible alternative design approach:

• <algorithm> provides definitions of the algorithms that forward to ExecutionPolicy::__<algorithm>(<args>) if they exist, or otherwise forward to the non-execution-policy overload if ExecutionPolicy::<some tag member> exists (indicating that the template argument was a real execution policy that just doesn't happen to specialize that algorithm), and otherwise produce an error that the argument is not an execution policy.
• <execution> defines the specialized algorithms for each execution policy as static member functions on the execution policy.

This adds minimal cost to code that only includes <algorithm> and not <execution> while keeping both headers modular. It also supports the easy addition of new implementation-specific execution policies, defining one policy as a refinement of another, and gives an easy way for an implementation to be conforming in the absence of a pstl implementation (or to provide an easy non-parallel implementation for single-threaded builds): just define the execution policy objects with no members other than the "I'm a real execution policy" tag member.

Would that work?

[ldionne]

libcxx/include/algorithm

5790 ↗

(On Diff #194378)

I'll experiment with that. For now, however, we could always include <glue_algorithm_impl.h>, which will pull in the algorithm definition. That'll take care of the libc++/pstl integration. I can then work on pstl in isolation to make it include the definitions of algorithms only when <execution> is included.

Once we're satisfied with what we have we can turn on the parallel algorithms by default in libc++.

libcxxabi/src/CMakeLists.txt

149 ↗

(On Diff #205866)

To be clear, we already have this problem today. libc++abi requires libc++ in order to build. Now we just need to transitively pull in the pstl.

[MikeDvorskiy]

libcxx/lib/CMakeLists.txt

125 ↗

(On Diff #194378)

ParallelSTL is installed on the system

What does it mean? The fact there is no ParallelSTL as "product/component". ParallelSTL - just a set of headers.

So, the cmake file should find "ParallelSTL package".

[ldionne]

libcxx/lib/CMakeLists.txt

125 ↗

(On Diff #194378)

Either it's in the tree and we can find it (like we do here), or it's somewhere on the system and has a ParallelSTLConfig.cmake file installed alongside it (which our current pstl build does install). Realistically, I only expect the former to be used.

[ldionne]

Use a local lit cfg to disable pstl tests and always include the definition of
algorithms in the headers.

[ldionne]

Install <execution> alongside other headers

[ldionne]

So, as it turns out, we actually have forward declarations for all the algorithms in the pstl, so the integration as-is will not pull any of those in. They're only pulled in when <execution> is included.

[MikeDvorskiy]

libcxx/lib/CMakeLists.txt

125 ↗

(On Diff #194378)

Parallel STL is not a separate product/package, so it doesn't make sense to have formal versioning for it. Also it doesn't make sense installation part from CMakeLists.txt.

We won't be able to use find_package(ParallelSTL) (and find_package(ParallelSTL <version>) as well) in order to include Parallel STL into user project. But it is possible to use add_subdirectory(/path/to/parallelstl).

WDYT?

[ldionne]

libcxx/lib/CMakeLists.txt

125 ↗

(On Diff #194378)

Parallel STL is not a separate product/package, so it doesn't make sense to have formal versioning for it. Also it doesn't make sense installation part from CMakeLists.txt.

It definitely is a separate package, no? It's a self-standing library that libc++ happens to be using as an implementation detail. I can't see another way to look at this other than saying it's a bunch of sources that we copy-paste into libc++. But that way of looking at things isn't good because it means it doesn't make sense to build/test the PSTL on its own, which we want to do.

We won't be able to use find_package(ParallelSTL)

It does work if the PSTL is installed (which installs a ParallelSTLConfig.cmake file) before building libc++. Otherwise, it also works if the ParallelSTL is enabled as part of the monorepo, because even though the call to find_package(ParallelSTL) will fail, the pstl::ParallelSTL CMake target still exists.

Also, we're commenting on an old diff.

[ldionne]

Make sure we don't warn when we don't find the ParallelSTL package through CMake. In most if not all cases, this lookup will fail and we'll be using the local PSTL inside the monorepo, so we shouldn't be warning in the common case.

[ldionne]

Ping! I think this is good to go and doesn't have the problems mentioned by reviewers.

[ldionne]

I'll ship this on Friday if no additional objections are noted.

[ldionne]

I reverted this (in r366603) because it uncovered existing issues that somehow didn't show up locally. I'll work through those issues and re-commit once they are fixed (probably next week).

[EricWF]

Re-opening.

I have some concerns I would like to see addressed before this lands again.

Additionally, the PSTL integration is complicated enough that it warrants a design doc discussing it.
Please add additional documentation under libcxx/docs/DesignDocs.

Finally, I have some concerns about the include structure this patch introduces. For example, this could allow the PSTL, which includes STL headers and is included by them, to introduce cyclical dependencies in our headers.
How do we plan to manage this very weird yet intimate interaction between us and the PSTL?

libcxx/trunk/CMakeLists.txt

749

This causes *everyone* by default to generate a __config_site header.

The philosophy behind __config_site is that it should only be generated for exceptional cases, and that __config should be enough to correctly configure libc++ by default.

So this change is contrary to that design.

I think we should either invert this condition (so PSTL builds are special), or find a wholly different approach to this.

libcxx/trunk/src/CMakeLists.txt

200

Does find_package find the in-tree PSTL sources first? And then only later look for system installations?

231

Does the PSTL even produce a library? What libraries does this cause to be linked in?

And if the PSTL does require additional libraries to be linked in, are they all C libraries? If there are C++ libraries, have they been built from source against this specific configuration of libc++?

Consider cases where we're building a sanitized version of libc++. We need all the PSTL libs to be build with the sanitizers enabled as well.

libcxx/trunk/test/std/pstl

2

This is an interesting way to pull the test suite in. I'm not sure it's portable. Lots of version control systems or platforms do'nt like symlinks. This also assumes the monorepo layout.

Did you consider alternative solutions?

libcxxabi/trunk/src/CMakeLists.txt

150

Libc++abi shouldn't have to know about the parallel algorithms library *at all*.

Having the C++ runtime take a dependency on the PSTL is a layering violation.
This patch should not proceed until that dependency is removed.

[ldionne]

• Invert the logic to avoid defining a __config_site all the time
• Remove calls to find_package to make it obvious we're version-locked with the PSTL

libcxx/trunk/CMakeLists.txt

749

I'll revert the logic to avoid generating a __config_site header most of the time.

libcxx/trunk/src/CMakeLists.txt

200

This find_package call would find PSTL installed in something like /usr/lib, IF it was installed from CMake and ParallelSTLConfig.cmake was installed (typically in /usr/lib/cmake/ParallelSTL/ParallelSTLConfig.cmake).

In practice, this find_package call always fails and we only use the pstl::ParallelSTL target from the pstl project. I did it that way because find_package is the blessed way of finding dependencies with CMake. That being said, we just spoke and I'll remove the call to find_package to make it clear that pstl and libc++ are version-locked.

231

Does the PSTL even produce a library? What libraries does this cause to be linked in?

No. It just adds the -I<path-to-pstl-includes> flag to the compiler. In the future, if pstl were producing a library, this would also add whatever linker flags are necessary to link against it.

And if the PSTL does require additional libraries to be linked in, are they all C libraries? If there are C++ libraries, have they been built from source against this specific configuration of libc++?

There's no enforcing that we link against a compatibly-configured pstl, just like we can link incompatibly-configured libc++abi.dylib and libc++.dylib today. This is up to whoever is doing the build to make sure they're compatible. Normally these folks are vendors, so they understand the implications of messing up.

libcxx/trunk/test/std/pstl

2

I did consider adding a lit.local.cfg file that would somehow create an "external test suite" in the subdirectory it is present in. I couldn't find anything like that by myself, so I asked on the LLVM IRC and someone suggested using symlinks, which I did.

libcxxabi/trunk/src/CMakeLists.txt

150

To be clear, this dependency is only in order to get the right include paths. It's just a more CMake-friendly way of finding the path to the headers and explicitly adding -I<path-to-headers>, like we do for libc++ (through LIBCXXABI_LIBCXX_SRC_DIRS which sets up LIBCXXABI_LIBCXX_INCLUDE_DIRS).

[EricWF]

LGTM.

I still have my concerns about how we find, package, and install the PSTL. But after offline discussions with Louis, it's clear this is on his radar as well, and improvements will be made going forward.
This is a good first step.

[EricWF]

Oh, I forget to mention:

This. Needs. Docs.

They don't need to be a part of this patch, but the PSTL integration is novel, and it should have a design doc explaining how it's set up and why.

[intractabilis]

How to build clang with LIBCXX_ENABLE_PARALLEL_ALGORITHMS=ON? How do I install this mysterious PSTL? Is it Intel's PSTL? LLVM's PSTL? I tried to build and install pstl from llvm-project, but I still get

[6541/6545] Performing configure step for 'runtimes'
...
CMake Error at /home/user/libs-dev/llvm-project/libcxx/src/CMakeLists.txt:194 (message):
  Could not find ParallelSTL


-- Configuring incomplete, errors occurred!

when I build clang.