[MXNET-679] Refactor handling BLAS libraries with cmake #11148

lebeg · 2018-06-05T09:50:12Z

Description

This is fixing a number of issues with handling BLAS libraries with cmake:

[MXNET-115] USE_LAPACK is forced on all platforms
#8702 [DISCUSSION] Should we deprecate Makefile and only use CMake?
#8532 mxnet-mkl (v0.12.0) crash when using (conda-installed) numpy with MKL // (general MKL handling in comments)
#10175 MXNet MKLDNN build dependency/flow discussion
#9868 MKL and CMake
#9105 libmxnet.so load path error
#8729 Build amalgamation using a docker // (LAPACK with cross compilation in the comments)
#7852 Trouble installing MXNet on Raspberry Pi 3

Checklist

Essentials

The PR title start starts with [MXNET-679]
Changes are complete
Code is well-documented
To the my best knowledge, examples are not affected by this change

Changes

Comments

USE_MKLML_MKL was renamed to USE_MKLML due to common sense

marcoabreu · 2018-06-05T21:13:45Z

cmake/ChooseBlas.cmake

+    endif()
+  endif()
+
+  return()


Do we have a default case?

marcoabreu · 2018-06-07T12:20:16Z

How is this related to #11094?

lebeg · 2018-06-07T12:27:19Z

#11094 does not fix the problem that there is no liblapack library for OpenBLAS as well as for no library except Atlas. The logic to determine whether lapack functionality is available is broken in disregards of the USE_LAPACK flag being used. This PR is an attempt to fix and improve the situation and automatically detect lapack for each blas library used in the right way.

marcoabreu · 2018-06-07T12:36:58Z

Okay, please resolve conflicts so I can continue to review

aaronmarkham · 2018-07-16T21:52:14Z

Does this perhaps fix #11769?

lebeg · 2018-07-16T21:58:39Z

Yes, I think this should help.

aaronmarkham

Really cool stuff. Seems like this will be a much smoother experience. I had some minor nits on capitalization and consistency in the libraries' names... and some questions/suggestions.

Will we be moving / copying some of info in the comments of the CMakeLists.txt file to web pages in/around the install pages?

aaronmarkham · 2018-07-16T21:54:24Z

CMakeLists.txt

+# * MKL (MKL, MKLML, MKLDNN)
+# * Apple Accelerate
+#
+# The default order of choice for the libraries if found follows the path from probably the most


probably? 😆

aaronmarkham · 2018-07-16T21:55:35Z

CMakeLists.txt

+# 1. MKLDNN (requires and checks for MKL or MKLML)
+# 2. MKL
+# 3. MKLML (downloaded)
+# 4. Accelerate (on mac)


triggered by apple correct? Maybe worth putting in each one's param value, so when reading this you're associating the correct values...

aaronmarkham · 2018-07-16T21:56:06Z

CMakeLists.txt

+# (recommended) to less performant backends. The order is as follows:
+#
+# 1. MKLDNN (requires and checks for MKL or MKLML)
+# 2. MKL


(must register, download, and install separately)
link to it?

aaronmarkham · 2018-07-16T21:59:21Z

CMakeLists.txt

+# Apple's mathematical framework, probably the best choice on a Mac if MKL/MKLML/MKLDNN
+# are not available.
+# https://developer.apple.com/documentation/accelerate
+mxnet_option(USE_ACCELERATE_IF_AVAILABLE "Use Apple Accelerate framework if found, \


I get it now, after digging in, but on first glance, I would confuse this with a generic acceleration option, and say, "YEAH! Turn that one on for sure!", and not realize it is for Apple. What if you called it USE_APPLE_ACCELERATE_IF_AVAILABLE?

aaronmarkham · 2018-07-16T22:05:23Z

cmake/ChooseBlas.cmake

-  if(NOT MKL_FOUND)
+function(switch_lapack enable)
+  if(${enable})
+    message(STATUS "Enabling lapack functionality")


aaronmarkham · 2018-07-16T22:13:06Z

cmake/Modules/FindAtlas.cmake

-  find_library(Atlas_LAPACK_LIBRARY NAMES alapack_r alapack lapack_atlas  PATHS ${Atlas_LIB_SEARCH_PATHS})
-  set(LOOKED_FOR ${LOOKED_FOR} Atlas_CLAPACK_INCLUDE_DIR Atlas_LAPACK_LIBRARY)
-endif(Atlas_NEED_LAPACK)
+  message(STATUS "Looking for lapack support...")


aaronmarkham · 2018-07-16T22:13:14Z

cmake/Modules/FindAtlas.cmake

+        Atlas_CLAPACK_INCLUDE_DIR
+        Atlas_LAPACK_LIBRARY)
+
+    message(STATUS "Lapack found")


aaronmarkham · 2018-07-16T22:13:23Z

cmake/Modules/FindAtlas.cmake

+    message(STATUS "Lapack found")
+  else()
+    set(Atlas_LAPACK_FOUND False)
+    message(WARNING "Lapack with Atlas could not be found, lapack functionality will not be available")


aaronmarkham · 2018-07-16T22:14:24Z

cmake/Modules/FindOpenBLAS.cmake

+    )
+
+if(OpenBLAS_NEED_LAPACK)
+  message(STATUS "Looking for lapack support...")


aaronmarkham · 2018-07-16T22:14:37Z

cmake/Modules/FindOpenBLAS.cmake

+    message(STATUS "Lapack found")
+  else()
+    set(OpenBLAS_LAPACK_FOUND False)
+    message(WARNING "OpenBlas has not been compiled with Lapack support, lapack functionality will not be available")


lebeg · 2018-07-16T22:38:37Z

Thanks! Yes, probably building from source section would be the right place, what do you think?

szha

@lebeg thanks for the overhaul on cmake. Please don't carry out #8702 just yet as the deprecation may break builds.

lebeg · 2018-07-17T05:54:49Z

@szha Yes, of course. Actually the cake scripts are far from being ready for the switch. This change is just a step for resolving #8702

lupesko · 2018-08-06T19:38:32Z

@lebeg - are you planning to fix the issues that fail the tests?

lebeg · 2018-08-08T14:14:51Z

@lupesko sure, just came back from vacation, working on the fixes

larroy

Great PR! Are the changes to random in cpp code intended?

larroy · 2018-08-09T13:46:25Z

CMakeLists.txt

+
+# workaround to store CMAKE_CROSSCOMPILING because is getting reset by the project command
+if(CMAKE_CROSSCOMPILING)
+  set(__CMAKE_CROSSCOMPILING ${CMAKE_CROSSCOMPILING})


Good catch! And good comment

larroy · 2018-08-09T13:47:24Z

CMakeLists.txt

-message(STATUS "CMAKE_SYSTEM_NAME ${CMAKE_SYSTEM_NAME}")
+# Choose BLAS (Basic Linear Algebra Subprograms) computation libraries
+
+# MXNet supports multiple mathematical backends for computations on the CPU:


Awesome documentation!

larroy · 2018-08-09T13:49:05Z

CMakeLists.txt

+if(MSVC)
+  set(SYSTEM_ARCHITECTURE x86_64)
+else()
+  execute_process(COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE SYSTEM_ARCHITECTURE)


What if we are cross compiling?

That supposed to be the host architecture. It's used to not try to download MKLML in case we are on an arm platform.

But we cross compile for ARM in the host...

For cross compilation we use the CMAKE_CROSSCOMPILING flag to prevent a download

lebeg · 2018-08-09T14:46:36Z

Yes, the <random> code was necessary due to android armv7 clang build not having the rand_r function at all. Actually using rand_r in the way it was used is dangerous in a multithreaded environment. This change fixes this.

lebeg · 2018-08-09T14:51:33Z

@larroy @szha @aaronmarkham @marcoabreu @anirudh2290 @piiswrong @lupesko @nswamy could you have another look? Thanks!

KellenSunderland · 2018-08-10T13:14:51Z

cmake/ChooseBLAS.cmake

+  endif()
+
+  if(NOT MKL_FOUND)
+    message(WARNING "MKLDNN requires either MKL or MKLML, MKLDNN will not be available")


My understanding was that MKLDNN could be installed stand-alone, without any other MKL flavours.

I had actually the opposite opinion, but after taking a closer look I've realised that this is true. I will make the needed changes.

lebeg · 2018-11-22T10:45:51Z

This PR needs to be separated and, as I have stated in on the @dev list, refactored to used as much built-in cmake functionality as possible.

* update mkml * refine DownloadMKLML.cmake * merge DownloadMKLML.cmake from apache#11148 * fix mkldnn release version * fix windows compilation

stu1130 · 2019-03-15T20:11:09Z

@lebeg what would the plan for next step on this PR?

lebeg · 2019-03-18T16:49:04Z

@stu1130 I'm out of capacity for it right now, I would be glad if anyone has interest in taking it over. I would be happy to make reviews and provide support as much as I can.

lebeg requested a review from szha as a code owner June 5, 2018 09:50

marcoabreu reviewed Jun 5, 2018

View reviewed changes

cmake/ChooseBlas.cmake Outdated

endif()

endif()

return()

Copy link

Contributor

marcoabreu Jun 5, 2018

Choose a reason for hiding this comment

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

Do we have a default case?

lebeg force-pushed the lapack branch from d1caba2 to c8ded06 Compare July 9, 2018 16:46

lebeg changed the title ~~[WIP] [MXNET-115] USE_LAPACK is forced on all platforms~~ [WIP] Refactored handling BLAS libraries with cmake Jul 16, 2018

lebeg force-pushed the lapack branch from fc67655 to 34f7c85 Compare July 16, 2018 19:14

lebeg changed the title ~~[WIP] Refactored handling BLAS libraries with cmake~~ [MXNET-679] Refactor handling BLAS libraries with cmake Jul 16, 2018

aaronmarkham reviewed Jul 16, 2018

View reviewed changes

szha reviewed Jul 17, 2018

View reviewed changes

lebeg force-pushed the lapack branch 2 times, most recently from 0bd2103 to a909551 Compare July 20, 2018 11:41

pengzhao-intel mentioned this pull request Jul 26, 2018

Add linux and macos MKLDNN Building Instruction #11049

Merged

5 tasks

ThomasDelteil mentioned this pull request Jul 30, 2018

Build amalgamation using a docker #8729

Closed

lebeg force-pushed the lapack branch from 394baac to 2594c06 Compare August 8, 2018 14:20

lebeg requested a review from anirudh2290 as a code owner August 9, 2018 10:58

lebeg force-pushed the lapack branch from 3826be8 to 4ae3439 Compare August 9, 2018 11:43

larroy reviewed Aug 9, 2018

View reviewed changes

szha requested a review from yajiedesign August 9, 2018 18:59

lebeg force-pushed the lapack branch from f639bf0 to 55ec400 Compare August 10, 2018 10:57

KellenSunderland reviewed Aug 10, 2018

View reviewed changes

lebeg added 20 commits November 20, 2018 11:00

Various MKL/MKLML improvements

a32cd58

Fixed logging indentation

f1c5a05

Applied review comments fixes, disabled OpenMP for AppleClang

217294b

Small improvement to MKL libraries search

14ec7c5

Fixed MKL force after it was found

f9266ad

Added workaround for another mshadow inclusion

78bbb43

Added debug statements

ff406dd

Improved workaround for mshadow

c9b5541

Fixed intel/gnu omp recognition

7866006

Added Intel OpenMP usage check

47d878b

Reverted Intel OpenMP removal in the download script

0b8040c

Upgraded MKLML release to v0.16

6cd96b8

Improved log wording about OpenMP

6c25ea3

Updated md5 hases for MKLML files

ce50b8e

Reverted MKLML upgrade

66367a8

Improved documentation based on Aaron's feedback

ff327c1

Small fixes

e1f5563

Bumped MKLDNN to v0.17-rc version

0620b00

Fixd multiple problems

97dbf9b

Updated submodules

07bcddb

lebeg force-pushed the lapack branch from 5fae00a to 07bcddb Compare November 20, 2018 10:02

Reverted MKLDNN_RELEASE name change

bb92ac5

lebeg closed this Nov 22, 2018

azai91 pushed a commit to azai91/incubator-mxnet that referenced this pull request Dec 1, 2018

Update MKLML dependency (apache#13181)

c70146c

* update mkml * refine DownloadMKLML.cmake * merge DownloadMKLML.cmake from apache#11148 * fix mkldnn release version * fix windows compilation

This was referenced Dec 7, 2018

Replaced rand_r with std:: random generation #13574

Closed

[MXNET-679] Improved CMakeLists.txt #13576

Closed

lebeg mentioned this pull request Feb 12, 2019

Default cmake build uses openblas instead of MKL #14085

Closed

[MXNET-679] Refactor handling BLAS libraries with cmake #11148

[MXNET-679] Refactor handling BLAS libraries with cmake #11148

Conversation

lebeg commented Jun 5, 2018 • edited

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

marcoabreu commented Jun 7, 2018

lebeg commented Jun 7, 2018

marcoabreu commented Jun 7, 2018

aaronmarkham commented Jul 16, 2018

lebeg commented Jul 16, 2018

aaronmarkham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeg commented Jul 16, 2018

szha left a comment

Choose a reason for hiding this comment

lebeg commented Jul 17, 2018

lupesko commented Aug 6, 2018

lebeg commented Aug 8, 2018

larroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy Aug 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeg commented Aug 9, 2018

lebeg commented Aug 9, 2018

Choose a reason for hiding this comment

lebeg Aug 13, 2018 • edited

Choose a reason for hiding this comment

lebeg commented Nov 22, 2018

stu1130 commented Mar 15, 2019

lebeg commented Mar 18, 2019

lebeg commented Jun 5, 2018 •

edited

larroy Aug 13, 2018 •

edited

lebeg Aug 13, 2018 •

edited