[MXNET-343]fix Mkldnn with msvc #10629

yajiedesign · 2018-04-20T18:02:18Z

Description

fix Mkldnn with msvc

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:

https://issues.apache.org/jira/browse/MXNET-343

add linux

xinyu-intel · 2018-04-21T11:45:22Z

Jenkinsfile

@@ -358,7 +358,7 @@ try {
              mkdir pkg_%BUILD_NAME%\\build
              copy build_%BUILD_NAME%\\libmxnet.lib pkg_%BUILD_NAME%\\lib
              copy build_%BUILD_NAME%\\libmxnet.dll pkg_%BUILD_NAME%\\build
-              copy build_%BUILD_NAME%\\mkldnn.dll pkg_%BUILD_NAME%\\build
+              copy build_%BUILD_NAME%\\3rdparty\\mkldnn\\mkldnn.dll pkg_%BUILD_NAME%\\build
              copy build_%BUILD_NAME%\\libiomp5md.dll pkg_%BUILD_NAME%\\build


not sure if the omp and mklml are also in 3rdparty\mkldnn\

no omp mklml is in the root path.i am copy it in cmake,
the problem is that I can't figure out the location of mkldnn

maybe 3rdparty\mkldnn\src\mkldnn.dll

seems 3rdparty\mkldnn\src\mkldnn.dll is right but still some deps are missing.

yes,I don't know what is missing.Maybe msvcrt120.dll.

need some one login and look at it with depends.

marcoabreu · 2018-04-21T17:57:49Z

CMakeLists.txt

@@ -18,8 +18,8 @@ mxnet_option(USE_CUDNN            "Build with cudnn support"  ON) # one could se
 mxnet_option(USE_SSE              "Build with x86 SSE instruction support" ON)
 mxnet_option(USE_LAPACK           "Build with lapack support" ON IF NOT MSVC)
 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 UNIX AND (NOT APPLE))
-mxnet_option(USE_MKLDNN           "Use MKLDNN variant of MKL (if MKL found)" ON IF USE_MKL_IF_AVAILABLE AND UNIX AND (NOT APPLE))
+mxnet_option(USE_MKLML_MKL        "Use MKLDNN variant of MKL (if MKL found)" OFF IF (NOT APPLE))


Please keep the USE_MKL_IF_AVAILABLE argument - you're changing the default behaviour here

marcoabreu · 2018-04-21T17:59:05Z

Jenkinsfile

@@ -311,7 +311,7 @@ try {
            bat """mkdir build_vc14_gpu
              call "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\x86_amd64\\vcvarsx86_amd64.bat"
              cd build_vc14_gpu
-              cmake -G \"NMake Makefiles JOM\" -DUSE_CUDA=1 -DUSE_CUDNN=1 -DUSE_NVRTC=1 -DUSE_OPENCV=1 -DUSE_OPENMP=1 -DUSE_PROFILER=1 -DUSE_BLAS=open -DUSE_LAPACK=1 -DUSE_DIST_KVSTORE=0 -DCUDA_ARCH_NAME=All -DCMAKE_CXX_FLAGS_RELEASE="/FS /MD /O2 /Ob2 /DNDEBUG" -DCMAKE_BUILD_TYPE=Release ${env.WORKSPACE}"""
+              cmake -G \"NMake Makefiles JOM\" -DUSE_CUDA=1 -DUSE_CUDNN=1 -DUSE_NVRTC=1 -DUSE_OPENCV=1 -DUSE_OPENMP=1 -DUSE_PROFILER=1 -DUSE_BLAS=open -DUSE_LAPACK=1 -DUSE_DIST_KVSTORE=0 -DCUDA_ARCH_NAME=Maxwell -DCMAKE_CXX_FLAGS_RELEASE="/FS /MD /O2 /Ob2 /DNDEBUG" -DCMAKE_BUILD_TYPE=Release ${env.WORKSPACE}"""


Please elaborate this change? We're trying to cover all supported archs

This only speeds up compilation speed. Theoretically, it will not affect the correctness of programs.
In fact, even if you use "ALL", it is not a test of "ALL" at the time of testing.the NV driver will only choose a suitable one.

marcoabreu · 2018-04-21T18:00:33Z

Jenkinsfile

+                call "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\x86_amd64\\vcvarsx86_amd64.bat"
+                cd build_%BUILD_NAME%
+                set /a cores=36 * 2
+                jom -j 72


Please use the proper env variables and don't hardcode these values

marcoabreu · 2018-04-21T18:01:37Z

cmake/FirstClassLangCuda.cmake

@@ -125,8 +125,8 @@ endif ()
 #   mshadow_select_nvcc_arch_flags(out_variable)
 function(mshadow_select_nvcc_arch_flags out_variable)

-  set(CUDA_ARCH_LIST "Auto" CACHE STRING "Select target NVIDIA GPU achitecture.")
-  set_property( CACHE CUDA_ARCH_LIST PROPERTY STRINGS "" "All" "Common" ${CUDA_KNOWN_GPU_ARCHITECTURES} )
+  set(CUDA_ARCH_LIST "" CACHE STRING "Select target NVIDIA GPU achitecture.")


default behaviour change?

marcoabreu · 2018-04-21T18:03:00Z

python/mxnet/libinfo.py

@@ -37,6 +38,8 @@ def find_lib_path():
                logging.warning("MXNET_LIBRARY_PATH should be an absolute path, instead of: %s",
                                lib_from_env)
            else:
+                if os.name == 'nt':
+                    os.environ['PATH'] = os.path.dirname(lib_from_env) + ';' + os.environ['PATH']


Please append to the end of path instead of the beginning to prevent path masking

marcoabreu · 2018-04-21T18:03:09Z

python/mxnet/libinfo.py

@@ -69,6 +72,8 @@ def find_lib_path():
    if len(lib_path) == 0:
        raise RuntimeError('Cannot find the MXNet library.\n' +
                           'List of candidates:\n' + str('\n'.join(dll_path)))
+    if os.name == 'nt':
+        os.environ['PATH'] = os.path.dirname(lib_path[0]) + ';' + os.environ['PATH']


marcoabreu

.

xinyu-intel · 2018-04-22T03:11:59Z

@yajiedesign @marcoabreu It seems all check passed, I'll modify readme in #10613 when this pr merged. Thank you:)

zheng-da · 2018-04-22T08:25:12Z

Jenkinsfile

+        }
+      }
+    },
+    'Python 3: MKLDNN-GPU Win':{


i'm always wondering if it's necessary to test both Python 2 and 3 for all different cases? @marcoabreu

I think only keep one python 2 test is ok.

Yes, please only keep one test. Since you're testing the CPP backend, which is independent of Python, there's no need to test both versions.

marcoabreu · 2018-04-22T12:44:40Z

@yajiedesign in future, please start using meaningful commit messages or amend your commit. The current style does not add any value to your commits and makes it hard to keep track of the changes.

marcoabreu · 2018-04-22T12:46:22Z

Jenkinsfile

@@ -311,7 +311,7 @@ try {
            bat """mkdir build_vc14_gpu
              call "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\x86_amd64\\vcvarsx86_amd64.bat"
              cd build_vc14_gpu
-              cmake -G \"NMake Makefiles JOM\" -DUSE_CUDA=1 -DUSE_CUDNN=1 -DUSE_NVRTC=1 -DUSE_OPENCV=1 -DUSE_OPENMP=1 -DUSE_PROFILER=1 -DUSE_BLAS=open -DUSE_LAPACK=1 -DUSE_DIST_KVSTORE=0 -DCUDA_ARCH_NAME=All -DCMAKE_CXX_FLAGS_RELEASE="/FS /MD /O2 /Ob2 /DNDEBUG" -DCMAKE_BUILD_TYPE=Release ${env.WORKSPACE}"""
+              cmake -G \"NMake Makefiles JOM\" -DUSE_CUDA=1 -DUSE_CUDNN=1 -DUSE_NVRTC=1 -DUSE_OPENCV=1 -DUSE_OPENMP=1 -DUSE_PROFILER=1 -DUSE_BLAS=open -DUSE_LAPACK=1 -DUSE_DIST_KVSTORE=0 -DCUDA_ARCH_NAME=Maxwell -DCMAKE_CXX_FLAGS_RELEASE="/FS /MD /O2 /Ob2 /DNDEBUG" -DCMAKE_BUILD_TYPE=Release -DUSE_MKL_IF_AVAILABLE=0 ${env.WORKSPACE}"""


Please leave the cuda-archs unchanged

marcoabreu · 2018-04-22T12:47:52Z

cmake/MklDnn.cmake

+if(MSVC)
+  set(MKL_NAME "mklml_win_2018.0.2.20180127")
+  file(DOWNLOAD "https://github.com/intel/mkl-dnn/releases/download/v0.13/${MKL_NAME}.zip" "${CMAKE_CURRENT_BINARY_DIR}/mklml/${MKL_NAME}.zip" EXPECTED_MD5 "D7B4BD9E85BB7B85F5C956FE119A8722" SHOW_PROGRESS)
+  file(DOWNLOAD "https://github.com/yajiedesign/mxnet/releases/download/weekly_binary_build_v2/7z.exe" "${CMAKE_CURRENT_BINARY_DIR}/mklml/7z2.exe" EXPECTED_MD5 "E1CF766CF358F368EC97662D06EA5A4C" SHOW_PROGRESS)


Please refrain from using private repositories

Also please refrain from downloading unstable weekly builds. We want our users to only use stable and released versions.

need a new tag like "tools" or "utils" place this tools.

marcoabreu

See comments

use arc to all remove redundant test

yajiedesign · 2018-04-24T21:46:00Z

can we merge?

zheng-da · 2018-04-30T02:57:10Z

CMakeLists.txt

+  add_definitions(-DUSE_MKL=1)
+  add_definitions(-DCUB_MKL=1)
+  add_definitions(-DMXNET_USE_MKLDNN=1)
+  list(APPEND mxnet_LINKER_LIBS mkldnn)


I see what you are doing now. Some of the lines here are indeed not necessary.

yajiedesign · 2018-04-30T15:10:23Z

Is there any problem? Can we merge? @marcoabreu @szha

szha

Approved for code changes. My cmake knowledge is limited so a second pair of eyes is appreciated.

szha · 2018-05-03T05:07:35Z

Ping

xinyu-intel · 2018-05-03T05:26:19Z

@yajiedesign seems some conflicts occured.

…to mkldnn

xinyu-intel · 2018-05-04T09:06:48Z

@yajiedesign update your submodule using git submodule update --init --recursive, and then commit the new mkldnn to your repo.

…to mkldnn

TaoLv · 2018-05-05T06:06:07Z

Can you remove the mkldnn submudule change from this pr, then there is not need to fix the cpp test error.

yajiedesign · 2018-05-05T07:22:49Z

@TaoLv no,it not need with cpp test.but it need to other.the master mkldnn(b4137dfc88e3bf5c6b62e833121802eb8c6696da) is some error with msvc.look this https://github.com/intel/mkl-dnn/blob/b4137dfc88e3bf5c6b62e833121802eb8c6696da/src/common/utils.cpp#L18 the WIN32 must is _WIN32, the new version mkldnn is fixed it.

yajiedesign · 2018-05-05T08:21:29Z

ok.update done.wait merge.

TaoLv · 2018-05-05T08:25:59Z

OK, I see. There is another PR (#10819 ) is also trying to update the version of mkldnn submodule (maybe another version). Let's wait to see which one will be merged firstly.

yajiedesign · 2018-05-05T08:29:31Z

@TaoLv is same version 0.14 tag 0e7ca738866d22cc700aa33b8de120b938f910d0

ashokei · 2018-05-05T19:52:02Z

Since this issue is related to msvc, can we have this PR only have msvc change and its CI tests.

mkl-dnn version update and its CI tests are covered under MXNET-367 PR #10819 .

* fix mkldnn with msvc add linux * fix sum name * jk ip * add test * fixed missing depend * fix tools url use arc to all remove redundant test * updata mkldnn to 0.14 * change mkldnn to v0.14 * close "-mtune=generic" with msvc, turn off test example * up mkldnn * change mkldnn to good v0.14 tag * fix mklml download * fix MKLDNN_UTIL_FUNC.MemFormat

yajiedesign requested a review from szha as a code owner April 20, 2018 18:02

yajiedesign added 2 commits April 21, 2018 09:01

fix mkldnn with msvc

cfc6d88

add linux

fix sum name

40649b3

yajiedesign force-pushed the mkldnn branch from c5053fc to 9c5eeb2 Compare April 21, 2018 01:40

yajiedesign requested a review from marcoabreu as a code owner April 21, 2018 01:40

yajiedesign mentioned this pull request Apr 21, 2018

Add Windows MKLDNN Building Instruction #10613

Merged

5 tasks

xinyu-intel reviewed Apr 21, 2018

View reviewed changes

yajiedesign added 2 commits April 21, 2018 22:54

jk ip

ed95c85

add test

5e89c00

yajiedesign force-pushed the mkldnn branch from 137e036 to dea374f Compare April 21, 2018 14:57

yajiedesign added Windows CI labels Apr 21, 2018

yajiedesign force-pushed the mkldnn branch from 87e9d1b to 0697d33 Compare April 21, 2018 16:24

marcoabreu reviewed Apr 21, 2018

View reviewed changes

marcoabreu suggested changes Apr 21, 2018

View reviewed changes

zheng-da reviewed Apr 22, 2018

View reviewed changes

marcoabreu reviewed Apr 22, 2018

View reviewed changes

marcoabreu suggested changes Apr 22, 2018

View reviewed changes

yajiedesign added 2 commits April 23, 2018 09:36

fixed missing depend

0d27de6

fix tools url

7d1fc74

use arc to all remove redundant test

yajiedesign force-pushed the mkldnn branch from 6c344e6 to 7d1fc74 Compare April 23, 2018 01:37

zheng-da approved these changes Apr 30, 2018

View reviewed changes

szha approved these changes May 1, 2018

View reviewed changes

yajiedesign added 2 commits May 4, 2018 15:40

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

f9639d3

…to mkldnn

updata mkldnn to 0.14

3ecaa3c

yajiedesign added 9 commits May 4, 2018 19:24

change mkldnn to v0.14

01134e1

close "-mtune=generic" with msvc, turn off test example

4188015

up mkldnn

41daa7e

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

e31338b

…to mkldnn

change mkldnn to good v0.14 tag

d26540d

fix mklml download

b9cd9a3

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

edba55d

…to mkldnn

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

6fdb67c

…to mkldnn

fix MKLDNN_UTIL_FUNC.MemFormat

f39b1d0

TaoLv mentioned this pull request May 5, 2018

[MXNET-367] update mkldnn to v0.14 and disable building test examples #10819

Merged

7 tasks

piiswrong merged commit 31016c4 into apache:master May 7, 2018

rahul003 mentioned this pull request May 10, 2018

update mshadow #10853

Merged

4 tasks

yajiedesign deleted the mkldnn branch October 31, 2018 21:18

[MXNET-343]fix Mkldnn with msvc #10629

[MXNET-343]fix Mkldnn with msvc #10629

Conversation

yajiedesign commented Apr 20, 2018 • edited Loading

Description

Checklist

Essentials

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinyu-intel Apr 21, 2018 • edited Loading

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

marcoabreu Apr 21, 2018 • edited Loading

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

marcoabreu left a comment

Choose a reason for hiding this comment

xinyu-intel commented Apr 22, 2018

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

marcoabreu commented Apr 22, 2018

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

yajiedesign Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

marcoabreu left a comment

Choose a reason for hiding this comment

yajiedesign commented Apr 24, 2018

Choose a reason for hiding this comment

yajiedesign commented Apr 30, 2018 • edited Loading

szha left a comment

Choose a reason for hiding this comment

szha commented May 3, 2018

xinyu-intel commented May 3, 2018

xinyu-intel commented May 4, 2018

TaoLv commented May 5, 2018

yajiedesign commented May 5, 2018 • edited Loading

yajiedesign commented May 5, 2018 • edited Loading

TaoLv commented May 5, 2018

yajiedesign commented May 5, 2018

ashokei commented May 5, 2018

yajiedesign commented Apr 20, 2018 •

edited

Loading

xinyu-intel Apr 21, 2018 •

edited

Loading

marcoabreu Apr 21, 2018 •

edited

Loading

yajiedesign Apr 23, 2018 •

edited

Loading

yajiedesign commented Apr 30, 2018 •

edited

Loading

yajiedesign commented May 5, 2018 •

edited

Loading

yajiedesign commented May 5, 2018 •

edited

Loading