Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[FEATURE] Include FlexiBLAS as available vendor. Find BLAS libs #21093

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

[FEATURE] Include FlexiBLAS as available vendor. Find BLAS libs #21093

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
569ed57
Include FlexiBLAS as available vendor. Find BLAS libs
ccoulombe Jul 11, 2022
126fd30
Ensure Flexiblas is used when looking for it.
ccoulombe Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions cmake/ChooseBlas.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@
# under the License.

set(BLAS "Open" CACHE STRING "Selected BLAS library")
set_property(CACHE BLAS PROPERTY STRINGS "Atlas;Open;MKL")
set_property(CACHE BLAS PROPERTY STRINGS "Atlas;Open;MKL;FlexiBLAS")
# ---[ Root folders
set(INTEL_HOME_ROOT "$ENV{HOME}/intel" CACHE PATH "Folder contains user-installed intel libs")
set(INTEL_OPT_ROOT "/opt/intel" CACHE PATH "Folder contains root-installed intel libs")

if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
message(STATUS "BLAS is set to ${USE_BLAS}")
endif()
if(USE_BLAS MATCHES "MKL" OR USE_BLAS MATCHES "mkl" OR NOT DEFINED USE_BLAS)
find_path(MKL_INCLUDE_DIR mkl_version.h
Expand All @@ -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)
Copy link
Contributor

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

Copy link
Author

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?

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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

Copy link
Author

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.

include_directories(SYSTEM ${BLAS_INCLUDE_DIR})
list(APPEND mshadow_LINKER_LIBS ${BLAS_LIBRARIES})
add_definitions(-DMSHADOW_USE_CBLAS=1)
add_definitions(-DMSHADOW_USE_MKL=0)
add_definitions(-DMXNET_USE_LAPACK=1)
elseif(BLAS STREQUAL "Atlas" OR BLAS STREQUAL "atlas")
find_package(Atlas REQUIRED)
include_directories(SYSTEM ${Atlas_INCLUDE_DIR})
list(APPEND mshadow_LINKER_LIBS ${Atlas_LIBRARIES})
Expand Down