-
Notifications
You must be signed in to change notification settings - Fork 171
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
Provide BLAS, LAPACK backends and interfaces #772
Conversation
Any advice @perazz on how to attack this PR? I will have a detailed look on the code outside the |
Yep, I've limited this PR to BLAS/LAPACK source code hoping to make everything easier to review. The sources in Also: the most important thing to agree upon is the mechanism that allows to switch between internal/external implementation both in the CMake build and the fpm branch. Fpm only supports
stdlib/src/stdlib_linalg_blas.fypp Lines 20 to 37 in a356f14
|
@perazz I'm intrigued why you needed to branch the build process this way? From what I understood after working in stdlib is that either with CMake or the fpm-deployement script, build keywords are passed and used to have the 2-stage preprocessing. The main difference being that with fpm we are obliged to call this script because of fypp, then the fpm build. With CMake it does both in one go. I would have expected then that having exactly the same .f90(s) after fypp (script or CMake) and having cpp/fpp preprocessing activated, the build would follow the same rules. (I'm thinking outloud here so please correct me if I'm missing something) Ok, I think I'm getting the issue... with CMake, using find_package([...]) it can be possible to affect a variable like Would it make sense to maybe use a python script instead of a shell script for the fpm preprocessing and probably modify the toml file accordingly for compiling against and external lib or not? |
You've got the point @jalvesz - in fpm, interface-vs-implementation can only be resolved via I actually used a python script to generate that interface code, so if we find a better/more elegant alternative, I'm definitely up for putting the effort into it. However, I think that with these constraints standing, there's not many more options |
I think there is at least stdlib_system.F90 that uses a
If we find a way to conditionally affect the .toml file (or introduce conditionality within) we might get somewhere in this direction I guess... Maybe for Coming back to
This seems like the best approach |
Agree @jalvesz, here's an update that removes most of the boilerplate. now the files are pre-processed to |
src/stdlib_linalg_constants.fypp
Outdated
use stdlib_kinds, only: sp, dp, qp, int32, int64, lk | ||
use, intrinsic :: ieee_arithmetic, only: ieee_is_nan | ||
#if defined(_OPENMP) | ||
use omp_lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use omp_lib | |
!$ use omp_lib |
Using "!$" will make this line compiled conditionnaly on the support of OpenMP by the compiler (and the CPP dir. is therefore not needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jvdp1, interesting! These #if defined(_OPENMP)
statements were in the original LAPACK and I left them unchanged. Is it standard Fortran that they should be always properly interpreted if they start with !$
? If so, I could update them all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intriguing, what I had understood is that conditional directives require specific signatures such as !$omp
or !$dir
, etc. But this !$ something
is still just a comment... no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!$ something
is a conditional compilation sentinel and part of the OpenMP API (see section 3.3.2 of the OpenMP specs).
@perazz It is indeed part of OpenMP API specs (at least, I think, since OpenMP 2.5). I use it since many years with both Intel and gfortran compilers. I guess that it is also supported by other compilers. However, note that I am not against the cpp directive. I just find using !$ cleaner ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, didn't know that!! just tested a small code using this with nvfortran 24.1 and works as expected!
thank you @perazz! I think that your approach of using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I am very excited to see this get merged.
I have some minor comments, but other than that it looks great to me.
Comments
-DWITH_QP
would be good to have documentation in README.md-DSTDLIB_EXTERNAL_BLAS
would be good to have documentation in README.md-DSTDLIB_EXTERNAL_LAPACK
would be good to have documentation in README.md- As mentioned in the monthly call, and if easily doable, intend
!$
sentinels to be on the same level of nesting as the code they operate on. - Could we write down somewhere the version of BLAS/LAPACK is being used for the reference implementation? I know you mentioned that it is v3.10.1 in the comments, but it would be good if it was explicitly stated somewhere in the docs.
A slightly bigger picture question and related to my last comment. If I understood correctly the modernisation script for BLAS/LAPACK is fully automated. Would it make sense to be incorporated as part of stdlib's CD pipeline to keep stdlib's reference implementation updated?
To be clear I am not suggesting this to be done in this PR, or even as a next step. It's more like a final touch to the whole project.
Great work again.
Revert "indent OpenMP sentinels" This reverts commit 35b721b. indent OpenMP, do not strip EOL
35b721b
to
2be3340
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @perazz . LGTM.
It would be be nice if some specs could be added too, e.g., stdlib_linalg_blas.md
and stdlib_linalg_lapack.md
, mainly to document how these files were generated, from which version of BLAS/LAPACK, the phylosophy behind it (internal or external implementation,...).
IMO, for API of the different procedures, a link to any BLAS/LAPACK documentation is enough (no need to generate a full description of API of all procedures).
The other thing that I would suggest and might be helpful is to form a GitHub project (if one doesn't exist), to track Features/Issues specific to the BLAS/LAPACK work of stdlib. |
I agree with this. This PR will be quite a milestone, in terms of usage and of code size. |
Thanks @jalvesz for testing it out! The reason I made a special stdlib/src/stdlib_linalg_constants.fypp Lines 11 to 13 in 545b4c3
We could move it all back to
Thanks for catching these! Yes this is definitely a typo.
Absolutely. I'd suggest to do this as a separate issue though, as the current PR is already huge.
Thanks @gnikit, It's very much necessary, as otherwise, we won't be able to start deploying any high-level API. So if you have time, please take a look at that one too! (it's a far smaller PR). Thank you all @jvdp1 @jalvesz @gnikit for your time, and if you agree, let's try to get this merged soon. |
src/stdlib_linalg_lapack.fypp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file, there are 8 interfaces with the following signature:
procedure(stdlib_sel<>) :: select
need adding
import sp,dp,qp,ilp,lk, stdlib_sel<>
(stdlib_select_? or stdlib_selctg_?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may be right. Do procedure interfaces need to be imported? gfortran is not complaining for this, but it may be compiler-specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in c12b3c3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm double checking with gfortran, both ifort and ifx complained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I overlooked that because I'm testing with gfortran, but makes total sense, thanks for checking it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gfortran accepts building with and without. Since you added it in the latest push I think this is good to go :)
With two approvals, I think it makes sense to wait another few days, and if no more comments are made, it should be ready to merge. Thank you all |
I agree @jvdp1, I will merge this now. |
@perazz Thank you. I will prepare a PR for the release v0.5.0 . |
Yes, sure. no problem. I suggest that you already advertise it (even if not
being part of a release yet).
Le mar. 2 avr. 2024 à 10:56, Federico Perini ***@***.***> a
écrit :
… @jvdp1 <https://github.com/jvdp1> can I suggest to try review and merge
also PR #774 <#774> before
releasing v0.5.0? This would allow us to begin working on several
high-level APIs in parallel, so people would have time to look and comment
and we could iterate on them efficiently.
—
Reply to this email directly, view it on GitHub
<#772 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5RO7D7I2ZXT7SYC5CKJQLY3JXEZAVCNFSM6AAAAABEQJBQHKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGQ2DEMZRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
FYI, our build failed on CMAKE following the addition or #ifdef directives in the file stdlib_linalg_blas.f90. So we got some errors like
Anyway, we fix our build by adding in CMakeLists.txt
|
@cyrilgandon what OS/config are you using? those files are preprocessed by Lines 67 to 71 in f53ad6b
|
I import the .f90 files of stdlib directly from the stdlib-fpm branch of the project like this:
From my undestanding, the preprocessing of the fypp files are handling command like |
I also pick stdlib from the fpm branch that I preprocess locally with fypp before hand, having only @cyrilgandon indeed fypp is there as a pre-pre-processor to handle the meta-programming... would be so good if one could debug directly the |
We should definitely be using capitalised extensions if files contain cpp preprocessor directives. This is a convention that all modern compiler vendors adhere to and so should the Standard Library. |
Files are not capitalized in the Lines 12 to 13 in e958a29
I hadn't considered that people may just grab the fpm branch because it's not got any |
@perazz Excuse me, are there any immediate plans for CMake to call OpenBLAS and other BLAS backends? I am updating the Fortran-stdlib version for MSYS2, and it seems that I have prematurely linked it to OpenBLAS in MSYS2. (I'm about to revert to intrinsic Reference-BLAS of Fortran-stdlib) |
@zoziha if MSYS2 provides openblas, it makes absolute sense to link against it instead of using the internal implementation. It is probably faster |
@perazz, MSYS2 developers seem to suggest implementing CMake updates in the upstream Fortran-stdlib library, rather than adding a CMake patch to Fortran-stdlib in MSYS2. I am currently adding a CMake patch to MSYS2 that uses OpenBLAS, it seems too early to do so? |
Correct: although the mechanism to link against external BLAS is already in place (via preprocessor macros), we need to find a way to enable it both from CMake and fpm builds. This is not done yet |
with CMake, we could adapt the following CMake script for BLAS/LAPACK libraries. I believe @awvwgk had a similar one in one of his libraries (I actually adapted this one from Then we should just add in the main CMake file (with an adaptation for the CPP macro) the following:
I just found back the CMake script: FindCustomBlas.cmake. There is a similar one for LAPACK |
If we utilize the diff --git a/CMakeLists.txt b/CMakeLists.txt
index b10e1f7..167efb5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -42,6 +42,26 @@ if(NOT DEFINED CMAKE_MAXIMUM_RANK)
set(CMAKE_MAXIMUM_RANK 4 CACHE STRING "Maximum array rank for generated procedures")
endif()
+find_package(BLAS)
+if(BLAS_FOUND)
+ add_compile_definitions(STDLIB_EXTERNAL_BLAS)
+ include_directories(${BLAS_INCLUDE_DIRS})
+ link_directories(${BLAS_LIBRARY_DIRS})
+ message(STATUS "Using external BLAS")
+else()
+ message(STATUS "Using intrinsic reference BLAS")
+endif()
+
+find_package(LAPACK)
+if(LAPACK_FOUND)
+ add_compile_definitions(STDLIB_EXTERNAL_LAPACK)
+ include_directories(${LAPACK_INCLUDE_DIRS})
+ link_directories(${LAPACK_LIBRARY_DIRS})
+ message(STATUS "Using external LAPACK")
+else()
+ message(STATUS "Using intrinsic reference LAPACK")
+endif()
+
# --- find preprocessor
find_program(FYPP fypp)
if(NOT FYPP)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index a031ab8..91f4020 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -115,6 +115,14 @@ set(SRC
add_library(${PROJECT_NAME} ${SRC})
+if(BLAS_FOUND)
+ target_link_libraries(${PROJECT_NAME} ${BLAS_LIBRARIES})
+endif()
+
+if(LAPACK_FOUND)
+ target_link_libraries(${PROJECT_NAME} ${LAPACK_LIBRARIES})
+endif()
+
set_target_properties(
${PROJECT_NAME}
PROPERTIES If we need to deal with more complex external BLAS, the links mentioned by @jvdp1 are useful. |
This PR introduces BLAS and LAPACK backends and interfaces.
cpp
macros.WITH_QP
macroThe backend will necessarily support linear algebra development for users that don't have access to an external library, and, to enable quadruple precision support. How this was designed:
FPM_DEPLOYMENT
guards code that is needed to enablefpm
branch to be linked to an external library. This is done with C preprocessor macros, so, they don't have to be present in the CMake buildEXTERNAL_BLAS
andEXTERNAL_LAPACK
can be defined to makefypp
use interfaces instead of the module subroutines. The latter are always available via modulesstdlib_linalg_blas
andstdlib_linalg_lapack
cf. #710 #749 #450
cc: @jvdp1 @jalvesz @gnikit @everythingfunctional @fortran-lang/stdlib @henilp105