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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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)
 
----------------
EricWF wrote:
> 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.
I'll revert the logic to avoid generating a `__config_site` header most of the time.


================
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")
----------------
EricWF wrote:
> Does `find_package` find the in-tree PSTL sources first? And then only later look for system installations?
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.


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



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60480