Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

Update mshadow.cmake to set BLAS #374

Merged
merged 2 commits into from
May 14, 2019
Merged

Conversation

yinghu5
Copy link
Contributor

@yinghu5 yinghu5 commented Apr 29, 2019

Working with @yajiedesign, @TaoLv @pengzhao-intel @larroy @NeoZhangJiangyu to restructure the mxnet cmake build systems to make MKLDNN, MKL logic correct.
In the past, the logic to set BLAS in mshadow.cmake is incorrect. Now, USE_BLAS have high priority than USE_MKL_IF_AVAILABLE.

Working with @yajiedesign, @TaoLv @pengzhao-intel @larroy @NeoZhangJiangyu to restructure the mxnet cmake build systems to make MKLDNN, MKL logic correct. 
In the past,  the logic to set  BLAS in mshadow.cmake is incorrect.  Now, USE_BLAS have high priority than USE_MKL_IF_AVAILABLE.
@pengzhao-intel
Copy link

Thanks of the improvement :) @larroy @szha

Could you paste the diagram of cmake (you showed internally) so people can understand the change easily?

@pengzhao-intel
Copy link

BTW, how do you verify the current change? Could you paste the test results?

@yinghu5
Copy link
Contributor Author

yinghu5 commented Apr 29, 2019

We planned to update the 3 cmakefiles : ChooseBLAS.cmake, FindMKL.cmake and mshadow.cmake. the logic diagram is as the pdf. Red parts are original logic and green parts are new. cmake_mxnet_fixed.pdf

The mshadow.cmake part is as below.
MKL_mxnet

@yinghu5
Copy link
Contributor Author

yinghu5 commented Apr 29, 2019

we check the change by add debug message in cmakefile.

message(STATUS "USE_MKL_IF_AVAILABLE ${USE_MKL_IF_AVAILABLE}")
message(STATUS "USE_MKLML_MKL ${USE_MKLML_MKL}")
message(STATUS "USE_MKLDNN ${USE_MKLDNN}")
message(STATUS "USE_BLAS ${USE_BLAS}")
message(STATUS "MKL_FOUND ${MKL_FOUND}")
message(STATUS "BLAS ${BLAS}")

and test were passed. Record into the doc files.
cmake_test.docx

Thanks

format the code
@yinghu5
Copy link
Contributor Author

yinghu5 commented May 5, 2019

@pengzhao-intel, please help to review the format change. thanks

Copy link

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@szha @lebeg @larroy to review and merge.

@TaoLv
Copy link
Member

TaoLv commented May 5, 2019

@yinghu5 please use this upstream branch in your MXNet PR then we can have an end to end validation.

@TaoLv
Copy link
Member

TaoLv commented May 14, 2019

@yajiedesign @szha @eric-haibin-lin is it possible to have this merged?

@szha szha merged commit 1d79ecf into dmlc:master May 14, 2019
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants