CMake: Pass OpenMP compiler and linker flags through CMake targets

[ywwry66]

Using OpenMP::OpenMP_LANG targets for CMake is less error-prone than passing the compiler and linker flags manually. Furthermore, it allows the user to customize those flags by setting OpenMP_LANG_FLAGS, OpenMP_LANG_LIB_NAMES, and OpenMP_omp_LIBRARY.

[ywwry66]

Ok, this is ready. If appropriate, I can also remove all OpenMP related code in cmake/fc.cmake to let CMake detect OpenMP Fortran flags automatically, as was done for C in 6aac065.

[dimpase]

see #5169 for the needed setup

[dimpase]

A successful test on Linux x86_64, where libomp is installed system-wide in /usr/. I ran the following:

cmake -B build -DUSE_OPENMP=1 -DBUILD_SHARED_LIBS=1 -DBUILD_STATIC_LIBS=0 -DOpenMP_C_FLAGS="-fopenmp" -DOpenMP_C_LIB_NAMES="omp" -DOpenMP_Fortran_FLAGS="-fopenmp" -DOpenMP_Fortran_LIB_NAMES="omp" -DOpenMP_omp_LIBRARY="omp" cd build/ && make -j8 && ctest 

and the linkage is correct:

$ ldd lib/libopenblas.so linux-vdso.so.1 (0x00007f0cc7826000) libm.so.6 => /lib64/libm.so.6 (0x00007f0cc6141000) libomp.so => /usr/lib64/libomp.so (0x00007f0cc5ff8000) libc.so.6 => /lib64/libc.so.6 (0x00007f0cc5dfd000) /lib64/ld-linux-x86-64.so.2 (0x00007f0cc7828000) 

[dimpase]

Also works on M1 up to date system, with

cmake -B build -DUSE_OPENMP=1 -DBUILD_SHARED_LIBS=1 -DBUILD_STATIC_LIBS=0 -DOpenMP_C_FLAGS="-Xpreprocessor -fopenmp -I$(brew --prefix libomp)/include" -DOpenMP_C_LIB_NAMES="omp" -DOpenMP_Fortran_FLAGS="-fopenmp" -DOpenMP_Fortran_LIB_NAMES="omp" -DOpenMP_omp_LIBRARY=$(brew --prefix libomp)/lib/libomp.dylib 

openblas.pc looks correct too.

[ywwry66]

Hi @martin-frbg, could you initiate those pending checks for the GitHub workflow? Thanks for the help.

[ywwry66]

The loongarch64 test does not use cmake, so the frozen test was not related to this PR.

[martin-frbg]

yes, that was probably some network error in the cloud

[dimpase]

Thank you!

[MehdiChinoune]

Is there a reason Why?
You know there is GNU Fortran and Flang (MinGW) for Windows that could build lapack+OpenMP on Windows.
@ywwry66

[ywwry66]

It is simply because the code was there before, added in 0beea3a, and I didn't investigate the feasibility of enabling OpenMP for OpenBLAS on Windows while working on this PR. But you are probably right, as your PR has passed the CI tests. @MehdiChinoune

[martin-frbg]

flang/OpenMP on Windows is still a bit dubious (it compiles but led to errors last time I checked, and I need to check again with a current version). but the restriction you complain about is only in the CMake configuration, and indeed mostly historical from when CMake usually implied someone using Visual Studio.

[derekchiang]

Has this fix been released to the openblas homebrew package? Currently users of ARM64 Macbooks still cannot install openblas due to the system clang not supporting fopenmp.

See this issue: ocaml/opam-repository#27483

[dimpase]

No, homebrew folks closed the corresponding issue as won't fix. With the rationale that it works with the "real" gcc, i.e. GNU gcc, not the clang renamed by Apple to gcc.

[ywwry66]

@derekchiang See Homebrew/homebrew-core#219339

[martin-frbg]

Ahem, do I get this right that the entire motivation for this PR is that the homebrew package is set up to be built with OpenMP enabled, while the native AppleClang does not support OpenMP at all ?
Your changes are way to invasive if it is just that, and they actually recreated the "conflicting openmp runtimes" that you're trying to solve by stopping the build and forcing the user to supply additional variables. According to the CMake documentation, variables set by FindOpenMP.cmake can legally remain empty if the compiler does not require them. (Thus the added test breaks compilation with NVHPC - at least)

[dimpase]

No, the native Apple clang does support OpenMP, but it requires a non-standard option to do so. And, as all clangs, it cannot use GNU OpenMP, aka libgomp, it needs llvm's OpenMP, aka libomp.

So you can build and use OpenMP-enabled openblas with gfortran+clang, but with llvm's OpenMP; in addition, Apple's clang does not grok -fopenmp, it needs a custom option (which has to be written into .pc file, too).

[martin-frbg]

According to https://mac.r-project.org/openmp/ that would be -Xclang -fopenmp instead of a plain -fopenmp, but one would also need to provide the actual dynamic library to link against (which would seem to be more unusual work than trivially editing the .pc file, even if the R folks kindly provide a list of downloads for "matching" builds of LLVM's libomp).
It does not appear to me that OpenBLAS should cater to such idiosyncratic demands of just one platform where they could be resolved by simpler means (such as installing another compiler, instead of trying to beat the vendor one into submission). The issue of clang not dealing well with libgomp had already been resolved prior to this PR by trivially linking only to the OpenMP_C dependencies (as there should be no sane reason to combine gcc with flang in the foreseeable future)

[dimpase]

I don't know what you mean by "another compiler". gcc (the real thing, not a renamed Apple clang) cannot handle macOS headers, as they use blocks.

Do you mean to use several C compilers to build a project ? Building a project with several C compilers isn't standard, and would require custom scripts...

[dimpase]

provide the actual dynamic library to link against

Well, such a library is available in Homebrew. But the current OpenBLAS won't easily allow to create a homebrew formula to use it instead of incompatible with clang libgomp.

(R Project overcomplicates things here IMHO)

The issue of clang not dealing well with libgomp had already been resolved prior to this PR by trivially linking only to the OpenMP_C dependencies

I don't follow here. Do you mean to say that there is a way to use libgomp with clang?

[ywwry66]

Ahem, do I get this right that the entire motivation for this PR is that the homebrew package is set up to be built with OpenMP enabled, while the native AppleClang does not support OpenMP at all ?

Your changes are way to invasive if it is just that, and they actually recreated the "conflicting openmp runtimes" that you're trying to solve by stopping the build and forcing the user to supply additional variables. According to the CMake documentation, variables set by FindOpenMP.cmake can legally remain empty if the compiler does not require them. (Thus the added test breaks compilation with NVHPC - at least)

While one of the applications of the PR is to allow building OpenBLAS on macOS with GCC+Apple Clang with OpenMP support, the main motivation is to let CMake handle OpenMP related Fortran flags automatically, and at the same time offers better customizability. In this regard, the changes are very generic and an overall improvement to the old cmake code that handles flags manually, and I don't see how they are invasive.

Admittedly, the test I added that caused the NVHPC issue was a little sloppy. My suggestion here is to either demote the error to warning or remove it altogether instead of reverting the entire PR.

[ywwry66]

Furthermore, the scope of this PR is very similar to that of d3272e5 and 6aac065 (pretty much parallel treatments to Fortran vs C). I don't understand why those are fine whereas this is "invasive" and "cater to idiosyncratic demands of just one platform".

[dimpase]

Not allowing a seamless treatment of OpenBLAS on macOS would basically push users to Apple's Accelerate framework, which, after years of lagging behind OpenBLAS in following BLAS/LAPACK standards, has caught up.

E.g., numpy/scipy, one of the most popular BLAS/LAPACK-using Python libraries, switched to using Accelerate in their binary wheels.