remove MKL_EXPERIMENTAL and update make files for MKL-DNN by ashokei · Pull Request #9810 · apache/mxnet

ashokei · 2018-02-16T18:16:48Z

Description

remove MKL_EXPERIMENTAL and update make files for MKL-DNN

Checklist

Essentials

Passed code style checking (make lint)
Changes are complete (i.e. I finished coding on this PR)

Changes

remove MKL_EXPERIMENTAL and update make files for MKL-DNN

ashokei · 2018-02-16T22:06:41Z

@zheng-da @piiswrong can you please review ... these are minor changes to remove old references to MKL_EXPERIMENTAL and minor updates to other make/doc changes. thanks!

zheng-da · 2018-02-16T22:24:00Z

-
-  4. Navigate into the python directory
-
-  5. Run 'sudo python setup.py install'


Is it necessary to remove this part? We still need to link to MKL for BLAS, right?

good point, i added full MKL related instructions back.

zheng-da · 2018-02-16T22:25:14Z

-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.


We need to measure the performance of MKLDNN.

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.

marcoabreu · 2018-02-16T23:41:04Z


  5. Run 'sudo python setup.py install'

-# MKL2017 PLUGIN


Shouldn't there be new documentation about MKLDNN instead?

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=1 or modify config.mk or cmake (just as other options).

@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 :)

Okay, thanks @pengzhao-intel

marcoabreu · 2018-02-16T23:41:35Z

 # use openmp for parallelization
 USE_OPENMP = 1

-# MKL ML Library for Intel CPU/Xeon Phi


Removing MKL flags from any arm cross compilation configs should be ok. Thanks for updating.

pengzhao-intel · 2018-02-17T02:03:55Z

@ashokei there's a test for MKLML installation, tests/python/cpu/test_mklml.py.
Is it possible to update this for MKL-DNN?

KellenSunderland · 2018-02-18T16:59:55Z

 # use openmp for parallelization
 USE_OPENMP = 1

-# MKL ML Library for Intel CPU/Xeon Phi


Removing MKL flags from any arm cross compilation configs should be ok. Thanks for updating.

ashokei · 2018-02-20T18:44:16Z

@szha i addressed all the review comments, can you please take a look/approve if ok. thanks!

rahul003 · 2018-02-20T20:26:09Z

Would it be possible to fix the CMake build with MKLDNN too? It says mkldnn.hpp not found.

/Users/huilgolr/mxnet/include/mxnet/./ndarray.h:41:10: fatal error: 'mkldnn.hpp' file not found
#include <mkldnn.hpp>

jinhuang415 · 2018-02-17T16:33:54Z

-ifeq ($(USE_MKL2017), 0)
+USE_STATIC_MKL = NONE
 ifeq ($(USE_BLAS), mkl)
 USE_STATIC_MKL = 1


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)?

Full MKL supports static lib option, the above flag enables that option. mklml only support dynamic; i believe both support all blas API.

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:
Static link to mkl library:
mshadow.mk line 63
MSHADOW_LDFLAGS += -L${MKLROOT}/../compiler/lib/intel64 -Wl,--start-group ${MKLROOT}/lib/intel64/libmkl_intel_lp64.a ${MKLROOT}/lib/intel64/libmkl_core.a ${MKLROOT}/lib/intel64/libmkl_intel_thread.a -Wl,--end-group -liomp5 -ldl -lpthread -lm

Dynamic link to mklml library:
mshadow.mk line 78:
MSHADOW_LDFLAGS += -Wl,--as-needed -lmklml_intel -lmklml_gnu

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?

Please respond to this que

@jinhuang415 @marcoabreu
MSHADOW can use MKLML (which is automatically selected when users build with MKLDNN); USE_BLAS=mkl is specifically for full mkl (not related to MKLML or MKLDNN). So there is no conflict here; Either of those options support full BLAS compatible API;

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.

jinhuang415 · 2018-02-17T16:38:00Z

@@ -45,14 +45,14 @@ def test_mklml_install():

    pid = os.getpid()
    rc = os.system("cat /proc/" + str(pid) + \
-                       "/maps | grep libmklml_ > /dev/null")


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?

technically MKL-DNN can be used without mklml, though we recommend users use both together for best performance.

zheng-da · 2018-02-21T05:53:31Z

@rahul003 cmake should work with mkldnn. mkldnn is a submodule in 3rdparty. did you update update mkldnn with git submodule update?

marcoabreu

Can you please add a Jenkins compile job (doesn't have to be run by unittests) that uses CMake with MKLDNN? Just take https://github.com/apache/incubator-mxnet/blob/master/Jenkinsfile#L263, copy it and enable the MKL flags.

ashokei · 2018-02-23T04:43:56Z

@marcoabreu can you please review requested Jenkins file changes; This PR is primarily targeting at removing MKL_EXPERIMENTAL and minor changes to regular makefiles; we may do a separate PR for cmake mkldnn build as required. thanks!

marcoabreu · 2018-02-23T09:18:30Z

 mx_lib = 'lib/libmxnet.so, lib/libmxnet.a, dmlc-core/libdmlc.a, nnvm/lib/libnnvm.a'
 // mxnet cmake libraries, in cmake builds we do not produce a libnvvm static library by default.
 mx_cmake_lib = 'build/libmxnet.so, build/libmxnet.a, build/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so'
+mx_cmake_mkldnn_lib = 'build/libmxnet.so, build/libmxnet.a, build/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, /usr/local/lib/libiomp5.so, /usr/local/lib/libmklml_gnu.so, build/3rdparty/mkldnn/install/lib/libmkldnn.so, build/3rdparty/mkldnn/install/lib/libmkldnn.so.0, /usr/local/lib/libmklml_intel.so'


I don't understand why we are generating files outside the source directory. This could cause issues because I would not assume that a compile process writes files to /usr/local or any other place outside of the source directory. Since I don't have much experience with cmake, I'd like to ask @larroy and @cjolivier01 for assistance here

@cjolivier01 /usr/local/lib/libiomp5.so /usr/local/lib/libmklml_gnu.so /usr/local/lib/libmklml_intel.so

beats me — ask the Intel guys

Will do. But is this an industry standard or would you expect all artefacts to be written into the source dir?

@pengzhao-intel

cjolivier01 · 2018-02-23T13:44:00Z

which files exactly?

…

On Fri, Feb 23, 2018 at 1:19 AM Marco de Abreu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Jenkinsfile <#9810 (comment)> : > @@ -24,6 +24,7 @@ mx_lib = 'lib/libmxnet.so, lib/libmxnet.a, dmlc-core/libdmlc.a, nnvm/lib/libnnvm.a' // mxnet cmake libraries, in cmake builds we do not produce a libnvvm static library by default. mx_cmake_lib = 'build/libmxnet.so, build/libmxnet.a, build/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, build/3rdparty/openmp/runtime/src/libomp.so' +mx_cmake_mkldnn_lib = 'build/libmxnet.so, build/libmxnet.a, build/dmlc-core/libdmlc.a, build/tests/mxnet_unit_tests, /usr/local/lib/libiomp5.so, /usr/local/lib/libmklml_gnu.so, build/3rdparty/mkldnn/install/lib/libmkldnn.so, build/3rdparty/mkldnn/install/lib/libmkldnn.so.0, /usr/local/lib/libmklml_intel.so' I don't understand why we are generating files outside the source directory. This could cause issues because I would not assume that a compile process writes files to /usr/local or any other place outside of the source directory. Since I don't have much experience with cmake, I'd like to ask @larroy <https://github.com/larroy> and @cjolivier01 <https://github.com/cjolivier01> for assistance here — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9810 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKts_V4K6R7JHtxWYDj-8E-RY7BJpf5eks5tXoKLgaJpZM4SIufG> .

ashokei · 2018-02-23T17:35:26Z

@marcoabreu jenkins issue is fixed, it passes the build.

ashokei · 2018-02-24T16:44:33Z

@marcoabreu i made suggested changes...can you please review/merge if ok.

marcoabreu

Lgtm. Please address the open questions

* replace MKL2017 references with MKL-DNN * remove MKLML_ROOT * MKL_README.md for Full MKL * update test_mkldnn * update Jenkinsfile * update jenkins * trigger Jenkins with new changes * trigger Jenkins with new changes

ashokei added 2 commits February 15, 2018 15:49

replace MKL2017 references with MKL-DNN

6f35f6e

remove MKLML_ROOT

40d0a6c

ashokei requested a review from szha as a code owner February 16, 2018 18:16

zheng-da reviewed Feb 16, 2018

View reviewed changes

MKL_README.md for Full MKL

e6defbb

marcoabreu reviewed Feb 16, 2018

View reviewed changes

update test_mkldnn

61c4a6e

KellenSunderland approved these changes Feb 18, 2018

View reviewed changes

rahul003 mentioned this pull request Feb 20, 2018

add USE_MKLDNN to config.mk #9831

Closed

6 tasks

szha approved these changes Feb 20, 2018

View reviewed changes

jinhuang415 reviewed Feb 21, 2018

View reviewed changes

marcoabreu suggested changes Feb 21, 2018

View reviewed changes

update Jenkinsfile

4e28b68

update jenkins

8a634c8

marcoabreu reviewed Feb 23, 2018

View reviewed changes

ashokei added 2 commits February 23, 2018 08:41

trigger Jenkins with new changes

e3aa01e

trigger Jenkins with new changes

ddbd999

marcoabreu approved these changes Feb 24, 2018

View reviewed changes

marcoabreu merged commit 5c5a904 into apache:master Feb 25, 2018

chengdazhi mentioned this pull request Mar 25, 2018

[Python2] Occasional and random Illegal Memory Access Error | Cached Op Input Context Error #10240

Closed


		4. Navigate into the python directory

		5. Run 'sudo python setup.py install'

Conversation

ashokei commented Feb 16, 2018

Description

Checklist

Essentials

Changes

Uh oh!

ashokei commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pengzhao-intel commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashokei commented Feb 20, 2018

Uh oh!

rahul003 commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinhuang415 Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zheng-da commented Feb 21, 2018

Uh oh!

marcoabreu left a comment

Choose a reason for hiding this comment

Uh oh!

ashokei commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjolivier01 commented Feb 23, 2018 via email

Uh oh!

ashokei commented Feb 23, 2018

Uh oh!

ashokei commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahul003 commented Feb 20, 2018 •

edited

Loading

jinhuang415 Feb 21, 2018 •

edited

Loading

ashokei commented Feb 24, 2018 •

edited

Loading