-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[FEATURE] Include FlexiBLAS as available vendor. Find BLAS libs #21093
base: master
Are you sure you want to change the base?
Conversation
Hey @ccoulombe , Thanks for submitting the PR
CI supported jobs: [unix-cpu, unix-gpu, centos-gpu, miscellaneous, windows-cpu, edge, clang, windows-gpu, centos-cpu, sanity, website] Note: |
@@ -33,7 +34,14 @@ if(USE_BLAS MATCHES "MKL" OR USE_BLAS MATCHES "mkl" OR NOT DEFINED USE_BLAS) | |||
endif() | |||
endif() | |||
|
|||
if(BLAS STREQUAL "Atlas" OR BLAS STREQUAL "atlas") | |||
if(BLAS STREQUAL "FlexiBLAS" OR BLAS STREQUAL "flexiblas") | |||
find_package(BLAS REQUIRED) |
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.
Would it make sense to set BLA_VENDOR to FlexiBLAS here? https://cmake.org/cmake/help/latest/module/FindBLAS.html#blas-lapack-vendors
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.
Yes, as I do set it at CLI anyway.
It could be done for the others as well.
But I feel it is the same as setting the preferred BLAS with BLAS=FlexiBLAS
. Unless you have in mind to refactor a bit so that find_package
is by itself unless one set a preferred BLAS?
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.
For the other BLAS besides MKL this ChooseBlas.cmake does not rely on the upstream find_package(BLAS). My only concern is that with your PR, users can specify BLAS=FlexiBLAS, but your find_package(BLAS) will find any BLAS that may or may not be FlexiBLAS. That's why it will be helpful for you to force BLA_VENDOR=FlexiBLAS inside this if clause.
So, specifically I'm suggesting adding a set(BLA_VENDOR FlexiBLAS)
before the current line but still inside the if clause.
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.
Right! Just updated and bumped minimum cmake version to support BLA_VENDOR=Flexiblas.
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! How about keeping the CMakeLists.txt cmake_minimum_required as is and adding cmake_minimum_required(VERSION 3.19)
before set(BLA_VENDOR FlexiBLAS)
inside the if clause? That would avoid unnecessarily breaking users on Ubuntu 20.04 LTS
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 it is best to raise the minimum cmake version especially since there's a lot of nice to have and improvement in recent version. My opinion is : live in the present and look in front, not behind in terms of versions.
How would that break for users? (I mean except having to update their cmake version.)
Have two requirements in different places could be surprising.
Set BLA_VENDOR and bump minimum CMake version accordingly.
Include FlexiBLAS as available vendor.
Find BLAS libs to link on Flexiblas.