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: match FindOpenMP.cmake output, add MSVC support #22353

Merged
merged 29 commits into from
Jul 8, 2024

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jan 15, 2024

With MSVC added, this PR adds compatibility with all compilers used on CCI, which allows this package to be used anywhere OpenMP is required without adding a bunch of OpenMP-specific build flags to package_info every time (e.g. https://github.com/conan-io/conan-center-index/blob/master/recipes/lightgbm/all/conanfile.py#L123-L133).

The compatibility with FindOpenMP.cmake ensures that find_package(OpenMP) with specified components and minimum version work as expected. Exports the same info about OpenMP version and flags as the official version does, but does so without compilation checks since the OpenMP spec version for llvm-openmp has been fixed and hardcoded since v9. The compilation checks were quite fragile when used with libs=False as well.

Also:

  • Drops old versions for which test_package failed to load the correct runtime for some reason.
  • Gets rid of all patches, since they don't really seem to be necessary for the build and test to pass. They were there mostly for the oldest versions anyway.

@valgur valgur changed the title llvm-openmp: fix missing FindOpenMP.cmake, check _OPENMP define llvm-openmp: rely on FindOpenMP.cmake from CMake Jan 15, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@valgur valgur changed the title llvm-openmp: rely on FindOpenMP.cmake from CMake llvm-openmp: match FindOpenMP.cmake output Jan 17, 2024
@conan-center-bot

This comment has been minimized.

@valgur valgur changed the title llvm-openmp: match FindOpenMP.cmake output llvm-openmp: match FindOpenMP.cmake output, add MSVC support Jan 17, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 10 (35fdf5ed036bf1d3cd633b1e3d249a23d42ec410):

  • llvm-openmp/18.1.8:
    All packages built successfully! (All logs)

  • llvm-openmp/15.0.7:
    All packages built successfully! (All logs)

  • llvm-openmp/13.0.1:
    All packages built successfully! (All logs)

  • llvm-openmp/11.1.0:
    All packages built successfully! (All logs)

  • llvm-openmp/17.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/14.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/16.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/12.0.1:
    All packages built successfully! (All logs)

  • llvm-openmp/17.0.4:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 11 (35fdf5ed036bf1d3cd633b1e3d249a23d42ec410):

  • llvm-openmp/18.1.8:
    All packages built successfully! (All logs)

  • llvm-openmp/17.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/17.0.4:
    All packages built successfully! (All logs)

  • llvm-openmp/16.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/11.1.0:
    All packages built successfully! (All logs)

  • llvm-openmp/14.0.6:
    All packages built successfully! (All logs)

  • llvm-openmp/13.0.1:
    All packages built successfully! (All logs)

  • llvm-openmp/12.0.1:
    All packages built successfully! (All logs)

  • llvm-openmp/15.0.7:
    All packages built successfully! (All logs)

@valgur valgur mentioned this pull request Jul 5, 2024
3 tasks
@conan-center-bot conan-center-bot merged commit a4e1f4f into conan-io:master Jul 8, 2024
44 checks passed
@valgur valgur mentioned this pull request Jul 8, 2024
3 tasks
jcar87 added a commit that referenced this pull request Jul 8, 2024
@jcar87
Copy link
Contributor

jcar87 commented Jul 8, 2024

Reverting this in #22353, as there are further questions before accepting this PR as it is.

The expectation when a recipe uses OpenMP is to use whatever is already supported by the compiler. So for gcc, this would be -fopenmp. Note that with gcc, this implies -lgomp, which is assumed to be part of the compiler toolchain.

For users that use either gcc or LLVM-clang, or any other compiler that already has OpenMP, I would expect that OpenMP can be used without much hassle, is this not the case?
If Conan Center’s docker images on Linux don’t currently have the built-in support for OpenMP for both GCC and clang, we can fix this and align our toolchain with what is already available in most distros.

I cannot find much evidence of success stories causing GCC to use LLVM’s libomp’s headers and libraries at build time. I understand what libomp.so may be ABI compatible with gcc’s libgomp.so, and this be runtime compatible in some scenarios - but even in that case there were issues in the past (see: https://lists.llvm.org/pipermail/llvm-dev/2015-May/085052.html ) Furthermore, other repository maintainers have explicitly avoided making the LLVM omp.h headers visible to gcc:

For gcc, the -fopenmp flag implies -lgomp , which will be passed to the linker regardless. So there is a very real risk of passing both -lomp and -lgomp. Depending on some circumstances (the build system used, shared/private, the link order, whether the linker has —as-needed or not), the following could happen:

  • Both omp (from this package) and gomp from the default installation are visible to the linker, in which case the linker may decide one or the other.
  • If for whatever reason both libomp and libgomp are to be loaded at runtime (for example, in those cases where -as-needed is NOT passed by the linker), this will cause issues : https://cpufun.substack.com/p/is-mixing-openmp-runtimes-safe . These could be: missing symbols (because gcc expects some internal symbols in libgomp), and in the worst case, the executable loads both libraries but does not work (does not actually parallelise anything)

For Visual Studio/msvc, using /openmp:llvm is still described as experimental
* https://learn.microsoft.com/en-us/cpp/build/reference/openmp-enable-openmp-2-0-support?view=msvc-170

With no indication that this is already available for production code. Additionally, it would seem to be the case that The libomp runtime is already shipped by visual studio, is it not usable? Why would one need to use a newer runtime, does it not work?

The most useful case would be for apple-clang, where OpenMP isn’t officially supported, and there is no system runtime, so making it available would solve this for users that have such a need - with the understanding that it is also experimental.

Additionally, I’m not sure completely overriding CMake’s built in FindOpenMP.cmake is a good idea at all. It already has logic to try and use the compiler built-in first. In most cases, the OpenMP::OpenMP_xxx target will ONLY be a wrapper around the relevant -fopenmp flag (or equivalent), and not contain any include or link directives - in that sense it is not much different than the Threads::Threads target.

There’s also loss of functionality in the file provided in this PR:

  • A user may expect to be able to influence which variant of the OpenMP flag is used by mcvc (with OpenMP_RUNTIME_MSVC which was added in CMake 3.30), but then find that conan is overriding this for them and making a different choice altogether.
  • FindOpenMP.cmake has support for compilers and languages that we do not cover in Conan Center - I see no reason why we should limit the functionality. This is mostly useful in the apple-clang case that requires providing a separate runtime.
  • There is logic that appears to have been "borrowed" from FindOpenMP.cmake in this PR - it needs to be fully identified, and we need to evaluate whether we need to propagate any license disclaimers.

So while providing a Conan package with the runtime (llvm-openmp) is very valid and can be useful in some scenarios (such as macOS with apple-clang), I fail to understand why the proposed PR is going way beyond that, and making it the default or all compilers we support in Conan Center, as well as completely supplanting the functionality provided by FindOpenMP.cmake. I’m assuming that for compilers that already support OpenMP, it should already work?

The motivation this PR description would be insufficient, in the sense that what is happening in this recipe, is not representative of the most common case: to have OpenMP directives in a public header, causing compiler flags to propagate. I doubt this is the most common case - what could happen however is that when libraries are built statically, one must propagate the relevant runtime as a system library (I see some recipes propagating -fopenmp but this would be unnecessary in most cases)

To summarise the concerns - we are outside of the comfort zone and it feels like are propagating this requirement (on this recipe) across conan center, is experimenting a tad too much:

gcc:

  • unknown risks of causing gcc to directly use the LLVM omp's headers and libraries at build time - this has been avoided by other package repository maintainers
  • risk of gcc users passing -lgomp and -lomp to the linker, which provide the same symbols - note that gcc will pass -lgomp as a consequence of using -fopenmp, regardless of where the runtime comes from

clang:

  • unknown what is the advantage of using this runtime vs the one that comes with clang -

msvc:

  • unjustified why an experimental flag should be made the default across all of conan center, removing control from the end users

all:

  • unknown what happens when a user is on a newer compiler that causes symbols to be needed that this runtime does not provide (vs the runtime that comes with the compiler)
  • the shared=False default - why it aligns with Conan Center, it's also unusual to use a static OpenMP runtime.

overriding FindOpenMP.cmake:

  • duplication of logic
  • "out of sync" by definition (there's already a switch in CMake 3.30 that is not supported here)

Otherwise across conan center:

  • If Conan Center's gcc and clang compilers on Linux don't support OpenMP, this is something we can fix. For msvc, we would support the default - it should be up to the user (not us) to decide which runtime to use, especially if the experimental llvm runtime can cause issues if user code assume the "default"/"legacy" runtime
  • We can consider depending on llvm-openmp when apple-clang is the compiler and OpenMP support is needed (as it's otherwise not available, nor would it conflict with anything else)
  • For recipes that depend on OpenMP, the static variant will require the openmp runtime to be listed in the system requirements

conan-center-bot pushed a commit that referenced this pull request Jul 9, 2024
… support"

* Revert "(#22353) llvm-openmp: match FindOpenMP.cmake output, add MSVC support"

This reverts commit a4e1f4f.

* Update conanfile.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants