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

Add findBLAS support to CMakeLists.txt #844

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented Jul 3, 2024

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.

@zoziha zoziha requested review from perazz and jvdp1 July 3, 2024 04:00
@perazz
Copy link
Contributor

perazz commented Jul 3, 2024

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:

6862209#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.

@jvdp1
Copy link
Member

jvdp1 commented Jul 3, 2024

thank you @zoziha for this PR.

Note that C/CXX must be enabled for MKL (see here). Did you test it with MKL?

@jalvesz
Copy link
Contributor

jalvesz commented Jul 3, 2024

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.

@perazz
Copy link
Contributor

perazz commented Jul 3, 2024

@jalvesz couple thoughts:

  • Testing all possible in the CI is hard (experience from fpm metapackages), 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:
    ! Integer size support for ILP64 builds should be done here
    integer, parameter :: ilp = int32
    private :: int32, int64

Should become

#ifdef STDLIB_BLAS_ILP64
integer, parameter :: ilp = int64
#else
integer, parameter :: ilp = int32
#endif

@jalvesz
Copy link
Contributor

jalvesz commented Jul 3, 2024

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

@perazz
Copy link
Contributor

perazz commented Jul 3, 2024

Absolutely! 💯

!> Error handling
type(error_type), allocatable, intent(out) :: error

#ifdef STDLIB_EXTERNAL_BLAS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I got this wrong, but I think the test should be calling

use stdlib_linalg_blas, only: axpy
...
call axpy( ... )

use stdlib_linalg_lapack, only: getrf
...
call getrf( ... )

Which already interface the respective routines if STDLIB_EXTERNAL_BLAS \ STDLIB_EXTERNAL_LAPACK are set or not.

In this case, the test_blas_lapack.fypp file does not need to be moved to the C preprocessed category. Which is why the fpm build is failing as it was not included in the fypp_deployment.py file, but I'm guessing that it is not needed.

Copy link
Contributor

@perazz perazz Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great comment Jose, thank you for looking into it. I've tried that. The meaning of the test is to ensure that the external function is being picked. However, calling the interface does not guarantee that we're using the external library function. One thing that I had tried to check that was to use a procedure pointer:

abstract interface
    subroutine blas_saxpy(blabla)
end interface

procedure(blas_saxpy), pointer :: axpy_ 
axpy_ => axpy
! Check that we're not using the internal implementation
call check(error, .not.associated(axpy_ , stdlib_saxpy))

But unfortunately, Fortran does not allow pointing to an interface, so this is not possible. If you have a better idea how could we check this, I'm very much in for removing preprocessing from the test module!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: the way that CMake uses to check if a fortran function exists is just to declare it external and try to compile a program:

https://github.com/Kitware/CMake/blob/a63bee680ce050e41f549ad9e3a811700347a3f6/Modules/CheckFortranFunctionExists.cmake#L62-L67

Copy link
Contributor

@jalvesz jalvesz Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so if I understand, the objective of this test is to check if STDLIB_EXTERNAL_BLAS \ STDLIB_EXTERNAL_LAPACK are activated, that the hypothesis of an optimized library being called is fulfilled, right?

If say one does force these macros to be TRUE but the external library is not visible in the path, then a compile-time error would occur such as "'undefined reference to `dgetrf_'" which would already alert of a link issue. If the library is visible, then

use stdlib_linalg_lapack, only: getrf
...
call getrf( ... )

would have the same effect as the local test interface. So I'm just wondering if there could be a different kind of test using directly the higher-level interfaces from the stdlib modules?

Now, if the test is kept as is, this file should be added here

"stdlib_linalg_lapack_w"
to enable the fpm job to treat it accordingly :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of the stdlib CI, yes because there are testing routines that check that the base functions work anyways.

But in general, if you don't build the tests there is no guarantee that these functions are available.
Consider this example at the Compiler Explorer: if you don't call axpy, the compiler doesn't care what's in the interface, so you're never sure.

So long story short, I think we have these two options:

  • remove these two additional tests, but run a full stdlib CI with an external BLAS/LAPACK in at least one of the system configurations;
  • keep these two tests, and then add preprocessing to the testing routines in the fpm deployment.

So in light of the additional complexity, maybe let's just remove the tests and go for option 1?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of option 1, so given this PR started by @zoziha is meant to help using the bindings in MSYS2, what about changing one of the windows CI jobs, I would also suggest one of the linux-intel jobs to link againts MKL. That should be more than good enough I think.

@zoziha
Copy link
Contributor Author

zoziha commented Jul 13, 2024

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.

README.md Outdated Show resolved Hide resolved
@zoziha
Copy link
Contributor Author

zoziha commented Jul 13, 2024

@perazz, @jalvesz, thank you for your review comments. The following contents have been updated currently:

  1. Keep the two tests submitted by @perazz ;
  2. 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.
  3. Add the FIND_BLAS option in CMakeLists.txt to 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.

@jvdp1
Copy link
Member

jvdp1 commented Jul 14, 2024

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

@perazz
Copy link
Contributor

perazz commented Jul 15, 2024

but fails on linalg_svd

Thanks @jvdp1, your fix looks great - the singular vectors can be flipped, which corresponds to opposite sign eigenvalues.

@zoziha
Copy link
Contributor Author

zoziha commented Jul 17, 2024

@jvdp1, thanks so much, this helps a lot.

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.

4 participants