stdlib
stdlib copied to clipboard
Add findBLAS support to CMakeLists.txt
Description
(Related to stdlib/pull/772#issuecomment and MINGW-packages/pull/20874#issuecomment.)
The stdlib has recently seen significant developments in linear algebra support. In the downstream fortran-stdlib MSYS2 package, there are optimized BLAS links such as OpenBLAS. This PR is prepared to submit a CMakeLists.txt patch that supports findBLAS: If CMake finds other BLAS or LAPACK backends, it will preferentially link them; if CMake does not find other BLAS or LAPACK backends, the built-in BLAS and LAPACK versions of stdlib will be used.
Suggestions are welcome, thanks in advance. I'm not sure if CMake options like -DFIND_BLAS need to be added for this.
Thank you for this PR @zoziha. I have tested it on my laptop and it works well!
I believe that in the future, we may want to extend findBLAS and findLAPACK to check for 64-bit integer types to avoid potential issues with some versions of MKL. But for now it should be OK (CMake will search for a 32-bit version first if no options are specified).
I think it would be nice to add a couple of tests. I've tried to fork your repository to add them directly but it says "No available destinations to fork this repository.". So I've created a new branch on my repository and you can check it out here:
https://github.com/fortran-lang/stdlib/commit/68622098db3320a560ff9eb0414f9e8579a31a33#diff-33394812ba204689144fd2f80832db83853ba1cb32403edb4e15fe4893e675fd
@jalvesz has also worked on the build system and may have advice for us.
EDIT: I found how to add the tests into your PR branch, thanks.
thank you @zoziha for this PR.
Note that C/CXX must be enabled for MKL (see here). Did you test it with MKL?
I think the tests and examples CMakeLists.txt should include if(BLAS_FOUND) if(LAPACK_FOUND)
There is also the BLA_VENDOR flag to activate as shown in the link shared by @jvdp1, there is some extra info in the latest cmake doc version : https://cmake.org/cmake/help/latest/module/FindBLAS.html (see at the end)
to check for 64-bit integer types to avoid potential issues with some versions of MKL
@perazz in the documentation they propose precisely:
set(BLA_VENDOR Intel10_64lp)
To make it point to a 64-bit version.
It could be a good idea to enable testing this within the CI:
- gnu + openblas
- intel compilers + MKL
- Apple + intel classic + accelerate ? (mkl can be mixed with gnu compilers but maybe it would be an unnecessary complexity for the CI maintenance)
for linux then having one of the gnu entries verify installation of blas/lapack
sudo apt-get install libblas-dev liblapack-dev
I'm not sure if CMake options like -DFIND_BLAS need to be added for this.
I think this is a good idea, the question is: what should the default be? I would be tempted to say that by default it should be TRUE.
Once this is running with CMake we can try to extend it with fpm as well.
@jalvesz couple thoughts:
- Testing all possible in the CI is hard (experience from
fpmmetapackages), but I do acknowledge that testing at least one external BLAS per platform is useful. However, we don't want to abandon testing against our own implementation, because that is the one we ship stdlib with, so it's good that we also run all tests with the internal implementation. - other packages allow some user inputs when selecting external BLAS/LAPACK. But, are we trying to run before we can walk? For now, I think it'd be great to just let CMake do the heavy lifting, and just ask the user whether they want an external library or not (i.e. via
-DFIND_BLAS) - For the 64-bit version, we are one cpp macro far from it because it's already engineered such that all integers are typed. It could be part of this PR as follows. CMake should just set one additional cpp macro to affect this line: https://github.com/fortran-lang/stdlib/blob/6d9d7fde67c1735f10427e4d358fcb5c84b6936b/src/stdlib_linalg_constants.fypp#L10-L12
Should become
#ifdef STDLIB_BLAS_ILP64
integer, parameter :: ilp = int64
#else
integer, parameter :: ilp = int32
#endif
@perazz totally agree, I just wanted to propose to have 1 or 2 jobs in the CI to test it, not to test all possible combinations as it would be too much and out of scope.
Absolutely! 💯
Note that C/CXX must be enabled for MKL (see here). Did you test it with MKL?
@jvdp1, the BLAS solution I commonly use personally is OpenBLAS, and I lack practical experience of using MKL on Linux. Therefore, I got stuck on MKL in this patch submission. I followed the guidance on the CMake help webpage. When enabling FIND_BLAS to search for MKL, I specified -DBLA_VENDOR=Intel10_64lp and sourced the script /opt/intel/oneapi/mkl/latest/env/vars.sh. The FindBLAS of CMake still not be able to find the location of the MKL link libraries.
(I hope someone experienced can provide assistance.)
In the latest patch, I explicitly specified the location of the MKL link library libmkl_rt.so, such as -DBLAS_LIBRARIES="/opt/intel/oneapi/mkl/latest/lib/intel64/libmkl_rt.so" -DLAPACK_LIBRARIES="/opt/intel/oneapi/mkl/latest/lib/intel64/libmkl_rt.so", which made the link successful.
test_linalg_svd reported an error due to linking to MKL and it will be fixed in the subsequent patch later.
@perazz, @jalvesz, thank you for your review comments. The following contents have been updated currently:
- Keep the two tests submitted by @perazz ;
- Add two CI tasks:
- Under the Windows-MSYS2-MinGW64 environment, GFortran compiles Stdlib and links OpenBLAS;
- Under the Ubuntu environment, the Intel LLVM Fortran compiler compiles Stdlib and links MKL.
- Add the
FIND_BLASoption inCMakeLists.txtto facilitate explicitly stating whether to search for external BLAS/LAPACK, which also helps the CI test.
I am not familiar with linking MKL with CMake under Linux. Any suggestions or patches are welcome. Thank you in advance.
I am not familiar with linking MKL with CMake under Linux. Any suggestions or patches are welcome. Thank you in advance.
I will have a look.
@zoziha I opened a PR in your repo. It finds MKL, but fails on linalg_svd
but fails on
linalg_svd
Thanks @jvdp1, your fix looks great - the singular vectors can be flipped, which corresponds to opposite sign eigenvalues.
@jvdp1, thanks so much, this helps a lot.
A new version of fortran_stdlib is about to be released. I'm wondering if this PR can be merged before the new version is released.
This PR implements the finding and linking of external BLAS and LAPACK in CMakelists.txt, and adds CI tests for Ubuntu + Ifx + MKL and MSYS2.UCRT + GFortran + OpenBLAS.
Thanks @zoziha for this PR! I think it is almost ready, just one last detail:
- the tests that need c-preprocessing should be added also in
config/fypp_deployment.pywithin the C_PREPROCESSED list. That way the suffix F90 will also be added forfpmdeployment.
- the tests that need c-preprocessing should be added also in
config/fypp_deployment.pywithin the C_PREPROCESSED list. That way the suffix F90 will also be added forfpmdeployment.
@jalvesz Okay, thanks for the reminder. I re-checked config/fypp_deployment.py and CMakeLists.txt and found that the members of the C_PREPROCESSED list are all related to BLAS + LAPACK. I also noticed some reference errors in CMakeLists.txt, where cppFiles and fppFiles were confused. Now they have been corrected.
Since they are all related to BLAS + LAPACK, it seems appropriate to fix them in this PR (Add findBLAS support to CMakeLists.txt).
The content of this PR is mostly finished. If possible, it can be merged later. 🍻
@perazz, @jalvesz, @jvdp1, thank you for your review and help.
Thank you for this PR @zoziha, with no further comments I'll merge this one.