Fix build issues and test failures in MKLDNN, and refine cmake files by gujinghui · Pull Request #11853 · pytorch/pytorch

gujinghui · 2018-09-19T07:00:15Z

Fix undefined symbol issue in MKLDNN after caffe2 rebase
Fix build error for MKLDNN due to cmake global var corrupted
Remove avx2 support in MKLDNN
Seperate mkl, MKLDNN and mklml in cmake
Fix random failure in test case convfusion_test

gujinghui · 2018-09-19T07:04:34Z

Hi @yinghai
The MKLDNN upgrade in this PR is only to fix the missing symbol after rebasing caffe2.
There is no other changes any more in MKLDNN.
Pls help merge it ASAP.

soumith

@gujinghui if we want to build MKLDNN functionality, but without ideep, it's no longer possible after this PR. We need to make sure that MKLDNN-only build (i.e. without ideep) is not removed / regressed

gujinghui · 2018-09-20T02:36:04Z

hi @soumith

ideep and mkldnn are exactly same thing. ideep just is the integration bridge of mkldnn for caffe2.
In this PR, we just help to build mkldnn from source code for both caffe2 and aten, instead of finding the pre-installed mkldnn.
Is it OK to you? Or, could you tell your concern ?

Thanks,

tools/build_pytorch_libs.sh

soumith · 2018-09-20T03:53:16Z

thanks. this is helpful context. I didn't have the context before-hand, so was confused as to what it was doing.

I requested that you revert some tab / whitespace changes in build_pytorch_libs.sh / bat. After that this PR is good to go from my side.

And gracefully include omp.h Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Mapping between cmake var and c macro: USE_MKL --> CAFFE2_USE_MKL USE_IDEEP --> CAFFE2_USE_IDEEP USE_MKLML --> CAFFE2_USE_MKLML Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

jiecaoyu · 2018-09-21T01:18:23Z

I think this PR will solve my problem. I am spending days to force caffe2 to use mkldnn and ideep. I doubt whether current version is really using mkldnn at all since I cannot find where mkldnn primitives are called.

jgong5 · 2018-09-21T01:21:44Z

@jiecaoyu PyTorch repo is changing very rapidly these days. We are trying hard to get MKL-DNN build back live these days...

jiecaoyu · 2018-09-21T01:26:15Z

@jgong5 Thanks so much!

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

caffe2/contrib/ideep/CMakeLists.txt


-  add_library(Caffe2_ideep_operators OBJECT ${avx2_srcs})
-  add_dependencies(Caffe2_ideep_operators Caffe2_PROTO)
-  set_target_properties(Caffe2_ideep_operators PROPERTIES COMPILE_FLAGS "-mavx2")


caffe2/ideep/utils/ideep_context.h


-BaseStaticContext* GetIDEEPStaticContext();
+#ifdef _WIN32
+#define CAFFE2_IDEEP_EXPORT __declspec(dllexport)


Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

yinghai

Some of the build errors look weird. Have you trying rebasing to master?

yinghai

This diff still tries to do too many things and now it's painful to debug the build errors. Let's further localize the changes. Let's make a PR to do just one job:

Fix undefined symbol issue in iDEEP after caffe2
rebase

And see how things go. It's OK that initially we have build errors.

cmake/Modules/FindBLAS.cmake

  ELSE (BLAS_USE_CBLAS_DOT)
    SET(BLAS_USE_CBLAS_DOT FALSE)
  ENDIF (BLAS_USE_CBLAS_DOT)
+  SET(CMAKE_REQUIRED_LIBRARIES)


gujinghui · 2018-09-22T14:19:15Z

@yinghai

split to #11975

pjh5 · 2018-10-23T20:55:21Z

@gujinghui what's the status of this? Are there other PRs to follow up the other changes that were included here?

gujinghui · 2018-10-24T02:50:56Z

@pjh5
Changes in this PR has been split into several PRs. And some of them has been merged in master branch.
We are continously co-working with pytorch/caffe2 team to make this happen ASAP.
If done, this PR will be closed.

BTW, we are working on #12170 now. After this got merged, you can run below test cmd to build with MKLDNN.

NO_MKLDNN=NO python setup.py build_deps

yinghai · 2018-10-29T21:23:02Z

Closing this as #12170 is closed.

pytorchbot added the caffe2 label Sep 19, 2018

gujinghui force-pushed the fix_build branch from 0b39266 to a0a7b54 Compare September 19, 2018 07:01

gujinghui mentioned this pull request Sep 19, 2018

[caffe2] static build mkldnn into caffe2 #11794

Closed

gujinghui force-pushed the fix_build branch from 226c1a5 to 62f400d Compare September 19, 2018 10:32

gujinghui mentioned this pull request Sep 19, 2018

[caffe2] Enable int8 inference for MKLDNN #11860

Closed

gujinghui force-pushed the fix_build branch from 62f400d to f0bb08d Compare September 19, 2018 17:06

gujinghui requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners September 19, 2018 17:06

soumith requested changes Sep 19, 2018

View reviewed changes

gujinghui force-pushed the fix_build branch from f0bb08d to 647bcd0 Compare September 20, 2018 03:05

soumith reviewed Sep 20, 2018

View reviewed changes

tools/build_pytorch_libs.sh Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

gujinghui force-pushed the fix_build branch from 1783ba0 to baf5067 Compare September 20, 2018 07:15

gujinghui added 8 commits September 20, 2018 16:00

Export iDEEP singlton explictly to fix test failures,

5476511

And gracefully include omp.h Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Fix build error for ideep

9f03e37

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Fix iDEEP convfusion test case

13c6d13

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Remove avx2 support and seperate mkl, ideep and mklml for mkldnn-bdg

66960e1

Mapping between cmake var and c macro: USE_MKL --> CAFFE2_USE_MKL USE_IDEEP --> CAFFE2_USE_IDEEP USE_MKLML --> CAFFE2_USE_MKLML Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

build mkldnn bridge for pytorch

47dea8e

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Avoid cmake error of not exported target

8f83cf7

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Disable mkldnn tests build

ce42820

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Fix build error in mklml interface

ce3d8a6

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

gujinghui force-pushed the fix_build branch from baf5067 to ce3d8a6 Compare September 20, 2018 08:35

Export GetIDEEPStaticContext for detectron_ops

2aaeb86

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

gujinghui added 3 commits September 21, 2018 10:21

Disable mkldnn test build on win32

2120724

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Correct lib path for MINGW/MSVC and export symbols for mkldnn and mklml

d9292cd

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Remove USE_IDEEP option, use USE_MKLDNN option, instead

f9160e2

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

gujinghui force-pushed the fix_build branch from 25be4d3 to f9160e2 Compare September 21, 2018 05:29

yinghai reviewed Sep 21, 2018

View reviewed changes

gujinghui added 4 commits September 21, 2018 16:57

Use CAFFE2_EXPORT to export symbols

6bbe047

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Add MKLDNN to caffe2 public dependencies

0ea7a9b

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

ln mkldnn in test_aten to fix not finding lib issue

13813db

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

Disable MKL-DNN if using MSVC due to omp version limitation

2c056b6

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>

gujinghui force-pushed the fix_build branch 2 times, most recently from 47403eb to 2c056b6 Compare September 21, 2018 14:58

pjh5 mentioned this pull request Sep 21, 2018

[Caffe2] undefined reference to mkldnn_* #9580

Closed

yinghai reviewed Sep 22, 2018

View reviewed changes

yinghai suggested changes Sep 22, 2018

View reviewed changes

yinghai reviewed Sep 22, 2018

View reviewed changes

cmake/Modules/FindBLAS.cmake

ELSE (BLAS_USE_CBLAS_DOT)

SET(BLAS_USE_CBLAS_DOT FALSE)

ENDIF (BLAS_USE_CBLAS_DOT)

SET(CMAKE_REQUIRED_LIBRARIES)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

gujinghui mentioned this pull request Sep 23, 2018

[caffe2]Export symbols for pybind and other libs after caffe2 rebase #11975

Closed

jiecaoyu mentioned this pull request Sep 27, 2018

[Caffe2/Bug] Cannot enable MKL-DNN #11850

Open

gujinghui changed the title ~~Fix build issues and test failures in iDEEP, and refine cmake files~~ Fix build issues and test failures in MKLDNN bridge, and refine cmake files Sep 28, 2018

gujinghui changed the title ~~Fix build issues and test failures in MKLDNN bridge, and refine cmake files~~ Fix build issues and test failures in MKLDNN, and refine cmake files Sep 28, 2018

yinghai closed this Oct 29, 2018

ezyang added the open source label Jun 24, 2019

Conversation

gujinghui commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gujinghui commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soumith left a comment

Choose a reason for hiding this comment

Uh oh!

gujinghui commented Sep 20, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

soumith commented Sep 20, 2018

Uh oh!

jiecaoyu commented Sep 21, 2018

Uh oh!

jgong5 commented Sep 21, 2018

Uh oh!

jiecaoyu commented Sep 21, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

yinghai left a comment

Choose a reason for hiding this comment

Uh oh!

yinghai left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

gujinghui commented Sep 22, 2018

Uh oh!

pjh5 commented Oct 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

yinghai commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gujinghui commented Sep 19, 2018 •

edited

Loading

gujinghui commented Sep 19, 2018 •

edited

Loading

gujinghui commented Oct 24, 2018 •

edited

Loading