[libcxx-commits] [PATCH] D60480: [libc++] Integrate the PSTL into libc++

[Louis Dionne via Phabricator via libcxx-commits]

ldionne planned changes to this revision.
ldionne marked 2 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/algorithm:5790
+// otherwise just pull in forward declarations.
+#include <pstl/internal/pstl_config.h>
+#if defined(__PSTL_EXECUTION_POLICIES_DEFINED)
----------------
rsmith wrote:
> dexonsmith wrote:
> > ldionne wrote:
> > > EricWF wrote:
> > > > ldionne wrote:
> > > > > 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.
> > > > > 
> > > > 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?
> > > 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.
> > @bruno, will this pattern cause a problem for Clang modules (esp. with local submodule visibility)?
> 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?
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++.


================
Comment at: libcxxabi/src/CMakeLists.txt:149
 endif()
 
+find_package(ParallelSTL)
----------------
EricWF wrote:
> ldionne wrote:
> > EricWF wrote:
> > > Why does libc++abi depend on parallel algorithms? That seems backwards?
> > 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.
> This is a layering violation and creates a circular dependency. I don't think we can or should structure the project like this.
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60480/new/

https://reviews.llvm.org/D60480