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

[MXNET-115] USE_LAPACK is forced on all platforms with OpenBLAS and c… #11094

Merged
marcoabreu merged 1 commit into from
Jun 7, 2018
Merged

[MXNET-115] USE_LAPACK is forced on all platforms with OpenBLAS and c… #11094

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 8 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ mxnet_option(USE_OPENMP "Build with Openmp support" ON)
mxnet_option(USE_CUDNN "Build with cudnn support" ON) # one could set CUDNN_ROOT for search path
mxnet_option(USE_SSE "Build with x86 SSE instruction support" ON)
mxnet_option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
mxnet_option(USE_LAPACK "Build with lapack support" ON IF NOT MSVC)
mxnet_option(USE_LAPACK "Build with lapack support" ON)
mxnet_option(USE_MKL_IF_AVAILABLE "Use MKL if found" ON)
mxnet_option(USE_MKLML_MKL "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE))
mxnet_option(USE_MKLDNN "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND (NOT APPLE))
Expand All @@ -38,6 +38,7 @@ mxnet_option(BUILD_CPP_EXAMPLES "Build cpp examples" ON)
mxnet_option(INSTALL_EXAMPLES "Install the example source files." OFF)
mxnet_option(USE_SIGNAL_HANDLER "Print stack traces on segfaults." OFF)

message(STATUS "CMAKE_SYSTEM_NAME ${CMAKE_SYSTEM_NAME}")
if(USE_CUDA AND NOT USE_OLDCMAKECUDA)
message(STATUS "CMake version '${CMAKE_VERSION}' using generator '${CMAKE_GENERATOR}'")
if(
Expand Down Expand Up @@ -363,17 +364,17 @@ elseif(UNIX)
list(APPEND mxnet_LINKER_LIBS pthread)
endif()


# ---[ LAPack
if(USE_LAPACK AND NOT MSVC)
if(USE_LAPACK)
add_definitions(-DMXNET_USE_LAPACK=1)
list(APPEND mxnet_LINKER_LIBS lapack)
else(USE_LAPACK)
# Workaround for Windows until using new Jenkinsfile.
if(BLAS STREQUAL "Open" OR BLAS STREQUAL "open" OR USE_BLAS STREQUAL "Open" OR USE_BLAS STREQUAL "open")
add_definitions(-DMXNET_USE_LAPACK=1)
if (NOT MSVC)
list(APPEND mxnet_LINKER_LIBS lapack)
Copy link
Copy Markdown
Contributor

@lebeg lebeg May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There is no separate liblapack for openblas as far as I understood. Is there for other libraries other than OpenBLAS? As far as I can tell CI checks only for OpenBLAS.

Copy link
Copy Markdown
Contributor Author

@larroy larroy May 30, 2018

Choose a reason for hiding this comment

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

I don't get your comment, I tried to fix the logic as I understood the previous logic was working around adding these flags in the windows case and breaking other platforms. Can you elaborate?

Copy link
Copy Markdown
Contributor

@lebeg lebeg May 30, 2018

Choose a reason for hiding this comment

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

list(APPEND mxnet_LINKER_LIBS lapack) line adds liblapack to required linker libraries. But there is no such think as liblapack for OpenBLAS, at least not for our builds. The CI passed in this case because with this change -DUSE_LAPACK=OFF becomes effective.

Copy link
Copy Markdown
Contributor

@lebeg lebeg May 30, 2018

Choose a reason for hiding this comment

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

And there is a chance that liblapack exists for other blas libraries (atlas?) but we are not testing them in CI currently

Copy link
Copy Markdown
Contributor

@lebeg lebeg May 30, 2018

Choose a reason for hiding this comment

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

With your change you need to set -DUSE_LAPACK=ON in arm builds not to loose lapack functionality.

Copy link
Copy Markdown
Member

@szha szha May 30, 2018

Choose a reason for hiding this comment

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

Just create symlinks such as libcblas and liblapack to the compiled libopenblas. There’s no need to hack around.

Copy link
Copy Markdown
Contributor

@lebeg lebeg May 30, 2018

Choose a reason for hiding this comment

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

Ok, this might be an option

Copy link
Copy Markdown
Contributor Author

@larroy larroy May 31, 2018

Choose a reason for hiding this comment

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

Thanks for the clarification, I got now what was the concern. We need better ARM / ANDROID architecture detection when cross compiling in CMake. This might be something we can definitely refine later. For the moment I'm fine with either solution proposed by @lebeg or @szha I think we will need to refine and improve this logic on future PRS.

endif()
endif()

message("USE LAPACK ${USE_LAPACK}")

# ---[ jemalloc
if(USE_JEMALLOC)
find_package(JeMalloc)
Expand Down