-
Notifications
You must be signed in to change notification settings - Fork 6.7k
remove MKL_EXPERIMENTAL and update make files for MKL-DNN #9810
Changes from all commits
6f35f6e
40d0a6c
e6defbb
61c4a6e
4e28b68
8a634c8
e3aa01e
ddbd999
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,21 +82,6 @@ USE_OPENCV = 0 | |
| # use openmp for parallelization | ||
| USE_OPENMP = 1 | ||
|
|
||
| # MKL ML Library for Intel CPU/Xeon Phi | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing MKL flags from any arm cross compilation configs should be ok. Thanks for updating. |
||
| # Please refer to MKL_README.md for details | ||
|
|
||
| # MKL ML Library folder, need to be root for /usr/local | ||
| # Change to User Home directory for standard user | ||
| # For USE_BLAS!=mkl only | ||
| MKLML_ROOT=/usr/local | ||
|
|
||
| # whether use MKL2017 library | ||
| USE_MKL2017 = 0 | ||
|
|
||
| # whether use MKL2017 experimental feature for high performance | ||
| # Prerequisite USE_MKL2017=1 | ||
| USE_MKL2017_EXPERIMENTAL = 0 | ||
|
|
||
| # whether use NNPACK library | ||
| USE_NNPACK = 0 | ||
|
|
||
|
|
@@ -115,13 +100,10 @@ USE_LAPACK_PATH = | |
| USE_INTEL_PATH = NONE | ||
|
|
||
| # If use MKL only for BLAS, choose static link automatically to allow python wrapper | ||
| ifeq ($(USE_MKL2017), 0) | ||
| USE_STATIC_MKL = NONE | ||
| ifeq ($(USE_BLAS), mkl) | ||
| USE_STATIC_MKL = 1 | ||
| endif | ||
| else | ||
| USE_STATIC_MKL = NONE | ||
| endif | ||
|
|
||
| #---------------------------- | ||
| # distributed computing | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,21 +82,6 @@ USE_OPENCV = 0 | |
| # use openmp for parallelization | ||
| USE_OPENMP = 1 | ||
|
|
||
| # MKL ML Library for Intel CPU/Xeon Phi | ||
| # Please refer to MKL_README.md for details | ||
|
|
||
| # MKL ML Library folder, need to be root for /usr/local | ||
| # Change to User Home directory for standard user | ||
| # For USE_BLAS!=mkl only | ||
| MKLML_ROOT=/usr/local | ||
|
|
||
| # whether use MKL2017 library | ||
| USE_MKL2017 = 0 | ||
|
|
||
| # whether use MKL2017 experimental feature for high performance | ||
| # Prerequisite USE_MKL2017=1 | ||
| USE_MKL2017_EXPERIMENTAL = 0 | ||
|
|
||
| # whether use NNPACK library | ||
| USE_NNPACK = 0 | ||
|
|
||
|
|
@@ -115,13 +100,10 @@ USE_LAPACK_PATH = | |
| USE_INTEL_PATH = NONE | ||
|
|
||
| # If use MKL only for BLAS, choose static link automatically to allow python wrapper | ||
| ifeq ($(USE_MKL2017), 0) | ||
| USE_STATIC_MKL = NONE | ||
| ifeq ($(USE_BLAS), mkl) | ||
| USE_STATIC_MKL = 1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know why we use static link when use MKL as blas? Is there any mkl related function could not be supported by mklml library (I remembered we used dynamic link to mklml library instead of static link to mkl library before for MKL2017, any change so we need to use static link)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Full MKL supports static lib option, the above flag enables that option. mklml only support dynamic; i believe both support all blas API.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification, so do you mean USE_BLAS=mkl is used for using full MKL as blas library (and using static link)? If we want to use mklml as blas library (it is more light weighted if it support all blas API), how should we set the compile option? I also notice there is some potential conflict here, if set USE_MKLDNN=1 and USE_BLAS=mkl, looks the generated libmxnet.so will both static link to mkl library and dynamic link to mklml library: Dynamic link to mklml library: Is there any potential conflict here or indicate some issue need to fix? If both MKL and MKLML library could provide the blas API, which library it will use?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please respond to this que
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question*
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jinhuang415 @marcoabreu mshadow handles both of these cases already; This PR or MKL-DNN integration PR , do not change this behavior. It is the same as before. As @zheng-da mentioned, we can modify mshadow to directly use MKL-DNN build flag in another PR. It wasnt done before, as we did not have MKL-DNN integration (we do now). This PR scope is limited to removing MKL_EXPERIMENTAL and cleaning up minor make file left overs. |
||
| endif | ||
| else | ||
| USE_STATIC_MKL = NONE | ||
| endif | ||
|
|
||
| #---------------------------- | ||
| # distributed computing | ||
|
|
@@ -176,4 +158,4 @@ USE_CPP_PACKAGE = 0 | |
| # whether to use sframe integration. This requires build sframe | ||
| # git@github.com:dato-code/SFrame.git | ||
| # SFRAME_PATH = $(HOME)/SFrame | ||
| # MXNET_PLUGINS += plugin/sframe/plugin.mk | ||
| # MXNET_PLUGINS += plugin/sframe/plugin.mk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,10 +18,7 @@ Performance is mainly affected by the following 4 factors: | |
| ## Intel CPU | ||
|
|
||
| For using Intel Xeon CPUs for training and inference, we suggest enabling | ||
| both `USE_MKL2017 = 1` and `USE_MKL2017_EXPERIMENTAL = 1` in | ||
| `config.mk`. Check | ||
| [MKL_README.md](https://github.com/dmlc/mxnet/blob/master/MKL_README.md) for | ||
| details. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to measure the performance of MKLDNN.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree, probably a separate PR, as there are other changes too (MXNet changes/op-refactor etc), not just MKL-DNN. We also have new AWS instance type, it would be nice to add AVX512 instance numbers. |
||
| `USE_MKLDNN = 1` in`config.mk`. | ||
|
|
||
| We also find that setting the following two environment variables can help: | ||
| - `export KMP_AFFINITY=granularity=fine,compact,1,0` if there are two physical CPUs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,24 +16,24 @@ | |
| # under the License. | ||
|
|
||
| """ | ||
| MKLML related test cases | ||
| MKL-DNN related test cases | ||
| """ | ||
|
|
||
| import logging | ||
| import os | ||
| from sys import platform | ||
|
|
||
| def test_mklml_install(): | ||
| def test_mkldnn_install(): | ||
| """ | ||
| This test will verify that MXNet is built/installed correctly when | ||
| compiled with Intel MKLML library. The method will try to import | ||
| the mxnet module and see if the mklml library is mapped to this | ||
| compiled with Intel MKL-DNN library. The method will try to import | ||
| the mxnet module and see if the mkldnn library is mapped to this | ||
| process's address space. | ||
| """ | ||
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
| if not platform.startswith('linux'): | ||
| logging.info("Bypass mklml install test for non-Linux OS") | ||
| logging.info("Bypass mkldnn install test for non-Linux OS") | ||
| return | ||
|
|
||
| try: | ||
|
|
@@ -45,14 +45,14 @@ def test_mklml_install(): | |
|
|
||
| pid = os.getpid() | ||
| rc = os.system("cat /proc/" + str(pid) + \ | ||
| "/maps | grep libmklml_ > /dev/null") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check if we still linked to libmklml_* library even for mkldnn case? If so we also need to add the check for libmklml_* library?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically MKL-DNN can be used without mklml, though we recommend users use both together for best performance. |
||
| "/maps | grep libmkldnn > /dev/null") | ||
|
|
||
| if rc == 0: | ||
| logging.info("MXNet is built/installed correctly with MKLML") | ||
| logging.info("MXNet is built/installed correctly with MKL-DNN") | ||
| else: | ||
| assert 0, "MXNet is built/installed incorrectly with MKLML, please " \ | ||
| assert 0, "MXNet is built/installed incorrectly with MKL-DNN, please " \ | ||
| "double check your build/install steps or environment " \ | ||
| "variable settings" | ||
|
|
||
| if __name__ == '__main__': | ||
| test_mklml_install() | ||
| test_mkldnn_install() | ||
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.
Shouldn't there be new documentation about MKLDNN instead?
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.
with new MKL-DNN integration, the installation has been tightly integrated (eg: mkl-dnn is a sub-module), no separate instructions are needed; the user just has to pass
USE_MKLDNN=1or modify config.mk or cmake (just as other options).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.
@marcoabreu @ashokei I'd like to create a new page (documentation) for MKL-DNN to introduce the installation, feature, performance, coverage, etc.
But we can make this page as-is now and I will back a new PR :)
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.
Okay, thanks @pengzhao-intel