[PATCH] D60480: [libc++] Integrate the PSTL into libc++

[Eric Fiselier via Phabricator via llvm-commits]

EricWF reopened this revision.
EricWF added a comment.

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?



================
Comment at: libcxx/trunk/CMakeLists.txt:749
 config_define_if(LIBCXX_NO_VCRUNTIME _LIBCPP_NO_VCRUNTIME)
+config_define_if_not(LIBCXX_ENABLE_PARALLEL_ALGORITHMS _LIBCPP_HAS_NO_PARALLEL_ALGORITHMS)
 
----------------
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.


================
Comment at: libcxx/trunk/src/CMakeLists.txt:200
+find_package(ParallelSTL QUIET)
+if (LIBCXX_ENABLE_PARALLEL_ALGORITHMS AND NOT TARGET pstl::ParallelSTL)
+  message(FATAL_ERROR "Could not find ParallelSTL")
----------------
Does `find_package` find the in-tree PSTL sources first? And then only later look for system installations?


================
Comment at: libcxx/trunk/src/CMakeLists.txt:232
+  if (LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
+    target_link_libraries(${name} PUBLIC pstl::ParallelSTL)
+  endif()
----------------
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.



================
Comment at: libcxx/trunk/test/std/pstl:1
+link ../../../pstl/test/std
----------------
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?




================
Comment at: libcxxabi/trunk/src/CMakeLists.txt:150
 
+find_package(ParallelSTL QUIET)
+if (NOT TARGET pstl::ParallelSTL)
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60480