Skip to content
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

Draft
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jun 19, 2024

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 from llvm 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 to blas and lapack ones. llvm-openmp is only used for Clang and AppleClang, but is installed as a dependency for all platforms, since the supports expression does not provide a way to distinguish between compilers.

I placed all llvm-openmp libraries and headers under an unofficial-llvm-openmp subdirectory in include/, lib/ and bin/ to avoid conflicts with the llvm port and with other OpenMP implementations that look for an omp.h header as well.

I added openmp as a dependency to the dmlc port to test it as a direct and a transitive dependency (via rabit, which depends on dmlc).


  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@WangWeiLin-MV WangWeiLin-MV added category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jun 20, 2024
conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Jul 8, 2024
* 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
@valgur valgur marked this pull request as ready for review July 8, 2024 13:34
@valgur valgur requested a review from cenit July 8, 2024 13:35
ports/llvm-openmp/portfile.cmake Outdated Show resolved Hide resolved
ports/llvm-openmp/vcpkg.json Outdated Show resolved Hide resolved
ports/openmp/vcpkg.json Outdated Show resolved Hide resolved
set(_CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH}")
# if(CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang)$")
if(CMAKE_CXX_COMPILER_ID MATCHES "^(Clang|AppleClang)$")
Copy link
Contributor

@cenit cenit Jul 9, 2024

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)"

Copy link
Contributor Author

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.

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a 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)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Aug 12, 2024
@data-queue data-queue added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 15, 2024
@vicroms
Copy link
Member

vicroms commented Sep 12, 2024

Hi @valgur

@AugP @BillyONeal @data-queue @JavierMatosD @ras0219-msft and I discussed this PR and have some comments:

  • The usual method to enable OpenMP is via a compiler flag (-fopenmp), which in turn does more work than just linking the library. It kind of blurs the line between something that is expected to be provided by the toolset and a development library. How does using the llvm-openmp port figure into this process? How does this improve on just doingbrew install libomp?
  • We are concerned that this PR is part of a larger effort to enable openmp by default in a number of ports and we are not convinced that doing so is the right option for all ports that support it.
  • We are not opposed to the existence of the meta-port since it would also offer some control on system dependencies. But would like to better understand the motivations behind creating one.

@WangWeiLin-MV WangWeiLin-MV removed the info:reviewed Pull Request changes follow basic guidelines label Sep 12, 2024
@JavierMatosD JavierMatosD marked this pull request as draft October 3, 2024 21:12
@valgur
Copy link
Contributor Author

valgur commented Nov 3, 2024

@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. 🙂

The usual method to enable OpenMP is via a compiler flag (-fopenmp), which in turn does more work than just linking the library. It kind of blurs the line between something that is expected to be provided by the toolset and a development library. How does using the llvm-openmp port figure into this process?

It is quite unusual in that sense, indeed.

The llvm-openmp port still relies on -fopenmp or similar being used, but requires the flags for include and libdir paths for the relocated libomp to be passed on top of it. This works automatically when find_package(OpenMP) and OpenMP::OpenMP_C/OpenMP::OpenMP_CXX is used. However, this cannot be made to work seamlessly with OpenMP_C_FLAGS/OpenMP_CXX_FLAGS, since they don't normally get passed on to the linker. The same goes for older build systems that rely on directly setting the build flags. This is usually easy to fix via patching for the ports on Vcpkg, if necessary, but does have the risk of affecting downstream projects that rely on OpenMP_C_FLAGS when they unexpectedly load the FindOpenMP.cmake from llvm-openmp.

How does this improve on just doing brew install libomp?

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 llvm-openmp port might be especially useful in that regard.

We are concerned that this PR is part of a larger effort to enable openmp by default in a number of ports and we are not convinced that doing so is the right option for all ports that support it.

That's a justified concern. There are three cases, broadly:

  1. Libraries with an optional support for OpenMP.
  2. Libraries that automatically enable OpenMP when available.
  3. Libraries that automatically enable it by default, tend to be used in performance-critical contexts, and don't offer any alternatives for parallelization support.

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 openmp feature, and while I have done the same analysis for Vcpkg ports, #pragma omp or #include <omp.h> was being used in 122/1900 recipes on ConanCenter. So explicitly enabling OpenMP for them might not affect the status quo for such ports all that much anyway. I personally would not mind leaving it disabled by default, though.

We are not opposed to the existence of the meta-port since it would also offer some control on system dependencies. But would like to better understand the motivations behind creating one.

The openmp meta-package was mainly intended to encapsulate the complexity of adding the llvm-openmp dependency for only Clang and AppleClang toolchains in ports. Not much more to it.

It does have the benefit of possibly offering more control over the exact OpenMP runtime implementation being used, but except for MSVC's -openmp:llvm and -openmp:experimental alternative OpenMP variants, I have not seen any cases where there might be benefit or interest in using a non-native implementation of OpenMP.


Overall, I'm not adamant about adding llvm-openmp or openmp as recipes, but I think it would be a net benefit.

The llvm-openmp limitation I mentioned above could be a genuine concern in some cases for downstream users, but at the same time should make OpenMP much easier to use on AppleClang compared to the Homebrew alternative. Improving the AppleClang support would also equalize the default capabilities of the many libraries that silently make use of OpenMP across all platforms.

To address the llvm-openmp limitation, we could consider adding an openmp[llvm] feature that is default-enabled on macOS and could be optionally enabled for Clang on other platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants