| rodgert | |
| EricWF |
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.
| libcxx/include/algorithm | ||
|---|---|---|
| 5790 ↗ | (On Diff #194378) | The same pattern is repeated in other files. Basically, we check whether <execution> has been included, and if so, then we provide all the definitions. Otherwise, we just provide forward declarations to avoid the compile-time overhead if you just #include <algorithm> but don't care about the parallel algorithms. |
| libcxx/lib/CMakeLists.txt | ||
| 125 ↗ | (On Diff #194378) | The basic idea is that when ParallelSTL is installed on the system (or if it's in the tree), we just find it with find_package and then declare a dependency on it and everything should work. |
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 |
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?
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.
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.
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++.
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.
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?
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++.
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.
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.
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.
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
| libcxx/include/algorithm | ||
|---|---|---|
| 5790 ↗ | (On Diff #194378) | I really really really dislike this pattern. We shouldn't do it. 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. |
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? |
| 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. |
| 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:
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? |
| 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. |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 125 ↗ | (On Diff #194378) |
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". |
| 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. |
Use a local lit cfg to disable pstl tests and always include the definition of
algorithms in the headers.
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.
| 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? |
| libcxx/lib/CMakeLists.txt | ||
|---|---|---|
| 125 ↗ | (On Diff #194378) |
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.
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. |
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.
Ping! I think this is good to go and doesn't have the problems mentioned by reviewers.
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).
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. | |
| 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 |
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.
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). | |
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.
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.