-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[llvm-openmp,openmp] add a meta-package for OpenMP #39389
base: master
Are you sure you want to change the base?
Conversation
* llvm-openmp: fix missing FindOpenMP.cmake, check _OPENMP define * llvm-openmp: improve exported compiler flags * llvm-openmp: print runtime library info in test_package * llvm-openmp: match the output of FindOpenMP.cmake * llvm-openmp: add MSVC support * llvm-openmp: mention potential cause of the armv8 Debug issues As suggested by @marxin at #19857 (comment) * llvm-openmp: add a comment * llvm-openmp: move omp to a separate component * llvm-openmp: add Conan v1 support * llvm-openmp: drop v10 and lower The wrong runtime library gets picked up in test_package for some reason. Don't feel like debugging it. * llvm-openmp: patches can be dropped * llvm-openmp: libomptarget is not available on macOS and Windows https://github.com/llvm/llvm-project/blob/llvmorg-17.0.6/openmp/CMakeLists.txt#L103-L104 * llvm-openmp: fix CMake 3.15 incompatibility * llvm-openmp: .get_safe("build_libomptarget") * llvm-openmp: don't use a target for linking in the module * llvm-openmp: restore the v11 macOS armv8 patch * llvm-openmp: add psapi system lib dep on Windows * llvm-openmp: OpenMP version and spec date version can be determined during packaging Avoids a fragile compilation check during find_package(). * llvm-openmp: re-wrap description * llvm-openmp: add v18.1.3 * llvm-openmp: print even more details in test_package * llvm-openmp: set OpenMP version to the minimum of compiler and runtime support * llvm-openmp: add all spec date versions From https://github.com/Kitware/CMake/blob/master/Modules/FindOpenMP.cmake * llvm-openmp: set shared=True by default on Windows * llvm-openmp: force shared=True on Windows * llvm-openmp: fix try_compile() on CXX-only projects * llvm-openmp: bump to v18.1.6 * llvm-openmp: bump to v18.1.8 * llvm-openmp: make the wrapper slightly more robust Based on microsoft/vcpkg#39389
set(_CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH}") | ||
# if(CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang)$") | ||
if(CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang)$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so how to use it also with MSVC, if I would like to?
There should be a "bypass" variable maybe? something like "...OR Z_VCPKG_USE_LLVM_OPENMP)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realistically, you should probably simply use the LLVM OpenMP implementation included with MSVC, available by setting /openmp:llvm
or OpenMP_RUNTIME_MSVC=llvm
in CMake.
I can add something like Z_VCPKG_OPENMP=llvm
for example, but unless it's exposed as a feature of the port, the user will have to take care to either set it globally or set it in the installed config, which feels a bit fragile. And using features to implement alternatives does not work well and is strongly discouraged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port installation tests pass with the following triplets:
- x64-osx (AppleClang 15)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New ports are not allowed to add wrapper files anymore, and as additional information, compiler condition is not allowed in portfile.cmake, it could be determined in the CMakeLists.txt of project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New ports are not allowed to add wrapper files anymore
Could you be more specific?
FindOpenMP.cmake
is an official CMake Find module. That's the case where wrappers might be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure how to modify it.
@data-queue Could you help to take a look? Thanks.
Hi @valgur @AugP @BillyONeal @data-queue @JavierMatosD @ras0219-msft and I discussed this PR and have some comments:
|
@vicroms First off, I just wanted to say thank you to you and the team for the time and consideration. It's definitely not one of the easier ones and can probably feel like a bit of a niche topic in the wider context of Vcpkg. 🙂
It is quite unusual in that sense, indeed. The
It gets rid of the need to ensure that libomp has been installed beforehand on macOS and Linux systems, obviously. This is both both ports that have a strict requirement on OpenMP and ones that enable it via a feature. Although in the latter case it could be argued that it's not unreasonable since the user already taking extra steps to enable it. The Homebrew is a bit of a special case, though, since AppleClang is not able to automatically find the relocated libomp it installs. This typically requires workarounds in the build systems, afaik (for example https://github.com/PointCloudLibrary/pcl/pull/6114/files). So an
That's a justified concern. There are three cases, broadly:
An example for the last case would be SuiteSparse, which is often used to do heavy lifting in the numerical core algorithms in other libraries, but which does not offer an alternative to OpenMP for parallelization (DrTimothyAldenDavis/SuiteSparse#313 (comment)). The opposite here would be OpenBLAS, which can use OpenMP but has a thread-based alternative backend available. I would definitely enable it for libraries that require it to be meaningfully usable, if possible. For the automatically-enabled case, explicitly enabling it should have zero impact on MSVC and GCC anyway, where OpenMP is always available, and Clang is typically well-tested as well by the projects. This does not apply to ports that have added an explicit feature to control and disable OpenMP, but they tend to be in the minority. There are currently about 30 ports on Vcpkg that expose an
The It does have the benefit of possibly offering more control over the exact OpenMP runtime implementation being used, but except for MSVC's Overall, I'm not adamant about adding The To address the |
There are many Vcpkg ports that can make use of OpenMP, but it's generally disabled by default due to the missing out-of-the-box support for Clang and AppleClang. This PR splits
llvm-openmp
fromllvm
into a separate port to fill that gap.Building the full
llvm
port just for OpenMP would be a massive overkill (easily more than hour to build and tens of GB of build artifacts). In contrast, the OpenMP component builds in just a few seconds.I also added an
openmp
meta-package that's intended to be similar toblas
andlapack
ones.llvm-openmp
is only used for Clang and AppleClang, but is installed as a dependency for all platforms, since thesupports
expression does not provide a way to distinguish between compilers.I placed all
llvm-openmp
libraries and headers under anunofficial-llvm-openmp
subdirectory ininclude/
,lib/
andbin/
to avoid conflicts with thellvm
port and with other OpenMP implementations that look for anomp.h
header as well.I added
openmp
as a dependency to thedmlc
port to test it as a direct and a transitive dependency (viarabit
, which depends ondmlc
).find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.