Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
Add findBLAS support to CMakeLists.txt #844
Changes from 10 commits
a203645
6e36e6b
6862209
03590ea
5567ab1
095ee16
d475595
434b849
0534325
3254fd0
90fd5b9
a690390
40765ad
b867c2b
9de0589
58074c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Correct me if I got this wrong, but I think the test should be calling
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.
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 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:
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!
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.
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
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 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
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/config/fypp_deployment.py
Line 23 in 4859081
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 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:
So in light of the additional complexity, maybe let's just remove the tests and go for option 1?
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 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.