build mkldnn as static lib on mac/linux #13197

azai91 · 2018-11-09T02:16:41Z

Description

currently MKLDNN is dynamically linked to mxnet. This can cause some issues if another version of mkldnn is present on the host machine and mxnet links to the host version. This change statically builds mkldnn into mxnet to handle version issues on linux/mac but keeps it dynamically linked on windows.

removed test_mkldnn_install.py from test as well since it tests if mkldnn is dynamically linked on linux (which it is no longe)

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:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

set property in cmake that causes mkldnn to statically link to mxnet

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

harshp8l · 2018-11-09T02:45:57Z

@mxnet-label-bot add [mkldnn, build]

kalyc · 2018-11-14T19:45:09Z

@mxnet-label-bot add [pr-work-in-progress]

azai91 · 2018-11-15T22:44:46Z

@marcoabreu do you have any idea why mxnet_unit_test still links to libmkldnn.so.0?

azai91 · 2018-11-20T22:53:22Z

@TaoLv can you take a look at this and help me find where we still link libmkldnn.so.0 in the CI?

TaoLv

Seems https://github.com/apache/incubator-mxnet/blob/master/tests/python/mkl/test_mkldnn_install.py#L27 also need be refactored.

TaoLv · 2018-11-22T13:10:08Z

CMakeLists.txt

@@ -225,6 +225,7 @@ endif()

 if(USE_MKLDNN)
  include(cmake/DownloadMKLML.cmake)
+  SET(MKLDNN_LIBRARY_TYPE STATIC)


Try set(MKLDNN_LIBRARY_TYPE "STATIC" CACHE INTERNAL "" FORCE)

azai91 · 2018-11-30T05:56:59Z

@TaoLv can you review this diff? prefer getting this merged before merging / fixing #13464 as there are quite a few changes to the CI that is needed.

Vikas-kum

Looks good!

apeforest · 2018-11-30T18:02:29Z

Makefile

+
+	ifneq ($(UNAME_S), Windows)
+	    LIB_DEP += $(MKLDNNROOT)/lib/libmkldnn.a
+    else


Indentation does not look right, could you please double check?

anirudh2290 · 2018-11-30T19:41:34Z

Makefile

+	else
+		CFLAGS += -I$(MKLDNNROOT)/include
+		LDFLAGS += -L$(MKLDNNROOT)/lib -lmkldnn -Wl,-rpath,'$${ORIGIN}'
+    endif


nit: indentation

it's for the ifeq on line 126.

Why isn't this aligned with L126 then?

apeforest · 2018-11-30T21:29:23Z

mkldnn.mk

 ifeq ($(UNAME_S), Darwin)
 	OMP_LIBFILE = $(MKLDNNROOT)/lib/libiomp5.dylib
 	MKLML_LIBFILE = $(MKLDNNROOT)/lib/libmklml.dylib
-	MKLDNN_LIBFILE = $(MKLDNNROOT)/lib/libmkldnn.0.dylib
+	MKLDNN_LIBFILE = $(MKLDNNROOT)/lib/libmkldnn.a
+else ifeq ($(UNAME_S), Windows)


Do we need a endif for this ifeq? Maybe better to break the line so it will be shown RED in web browser?

NVM, seems this is correct syntax in Makefile.

apeforest

LGTM

sandeep-krishnamurthy · 2018-12-01T06:58:40Z

Makefile

-	CFLAGS += -I$(MKLDNNROOT)/include
-	LDFLAGS += -L$(MKLDNNROOT)/lib -lmkldnn -Wl,-rpath,'$${ORIGIN}'
+
+	ifneq ($(UNAME_S), Windows)


Should we add a reason here, on why on Windows, it is dynamically linked for future maintenance?

+1 for adding comments. Also curious about how MKL BLAS is linked on Windows?

updated comment in #13503.

sandeep-krishnamurthy · 2018-12-01T07:01:07Z

Re-triggering failed CI.

TaoLv · 2018-12-01T15:31:00Z

Makefile

-	CFLAGS += -I$(MKLDNNROOT)/include
-	LDFLAGS += -L$(MKLDNNROOT)/lib -lmkldnn -Wl,-rpath,'$${ORIGIN}'
+
+	ifneq ($(UNAME_S), Windows)


+1 for adding comments. Also curious about how MKL BLAS is linked on Windows?

TaoLv · 2018-12-01T15:38:22Z

tests/python/mkl/test_mkldnn.py

-
-
-if __name__ == '__main__':
-    install.test_mkldnn_install()


Need make sure about the purpose of a test case before removing it. If this case is just for verifying whether MKL-DNN so is correctly linked, I'm OK with removing it. Otherwise, need replace it with something else.

The test looks if mkldnn appears on the memory table of the process running mxnet.I have verified manually (by cat /proc/{id}/maps | grep mkldnn) that mkldnn use to appear in the table when it is dynamically linked but no longer does (statically linked).

the second part of this just checks the response code (rc - probably to make sure the proc/{id}/maps file even exists).

@pengzhao-intel is there another reason this test was added

azai91 · 2018-12-03T23:20:46Z

#13503 has been merged. closing this PR.

azai91 requested a review from szha as a code owner November 9, 2018 02:16

marcoabreu added Build MKLDNN labels Nov 9, 2018

azai91 added 7 commits November 12, 2018 13:59

build mkldnn as static lib

b8a0203

update makefile to statically build mkldnn

bc6c482

build static mkldnn

15a41fc

fix static name

42b3353

fix static name

5af258a

update static for mac

32ab9ce

rename mkldnn dep in ci

e2422d6

azai91 force-pushed the feature/mkldnn-static branch from 50ebcbb to e2422d6 Compare November 12, 2018 22:03

azai91 requested a review from marcoabreu as a code owner November 12, 2018 22:03

azai91 added 5 commits November 12, 2018 15:09

remove moving mkldnn dynamic lib

372f697

retrigger

67e4dff

remove commented code

150b324

merge master

890cf1d

retrigger

89b11c6

marcoabreu added the pr-work-in-progress PR is still work in progress label Nov 14, 2018

azai91 added 2 commits November 14, 2018 12:17

Merge branch 'master' into feature/mkldnn-static

f29c254

remove mkldnn dnaymic for unitest

40fd0ac

azai91 requested a review from anirudh2290 as a code owner November 14, 2018 20:25

TaoLv reviewed Nov 22, 2018

View reviewed changes

azai91 and others added 3 commits November 22, 2018 11:52

force static for mkldnn lib

0302290

Merge branch 'master' into feature/mkldnn-static

23fd7d9

remove dynamic mkldnn bind

d103ec8

azai91 force-pushed the feature/mkldnn-static branch from 43cc63e to d103ec8 Compare November 28, 2018 02:38

remove test mkldnn install

45e8cd8

azai91 changed the title ~~build mkldnn as static lib~~ build mkldnn as static lib on mac/linux Nov 30, 2018

Vikas-kum approved these changes Nov 30, 2018

View reviewed changes

apeforest suggested changes Nov 30, 2018

View reviewed changes

fix spacing

ed31e12

anirudh2290 approved these changes Nov 30, 2018

View reviewed changes

fix index

6a02949

apeforest reviewed Nov 30, 2018

View reviewed changes

apeforest approved these changes Nov 30, 2018

View reviewed changes

azai91 force-pushed the feature/mkldnn-static branch from e1d1f60 to 232f6d4 Compare December 1, 2018 04:31

azai91 requested review from gigasquid, nswamy, sergeykolychev and yzhliu as code owners December 1, 2018 04:31

azai91 force-pushed the feature/mkldnn-static branch 5 times, most recently from 09de96b to e1d1f60 Compare December 1, 2018 05:02

add artifacts

13304ba

azai91 force-pushed the feature/mkldnn-static branch from e1d1f60 to 13304ba Compare December 1, 2018 05:05

retrigger

530a82c

sandeep-krishnamurthy reviewed Dec 1, 2018

View reviewed changes

azai91 mentioned this pull request Dec 1, 2018

Feature/mkldnn static 2 #13503

Merged

7 tasks

retrigger

1172bc6

TaoLv reviewed Dec 1, 2018

View reviewed changes

azai91 closed this Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build mkldnn as static lib on mac/linux #13197

build mkldnn as static lib on mac/linux #13197

azai91 commented Nov 9, 2018 •

edited

Loading

harshp8l commented Nov 9, 2018

kalyc commented Nov 14, 2018

azai91 commented Nov 15, 2018

azai91 commented Nov 20, 2018

TaoLv left a comment

TaoLv Nov 22, 2018

azai91 commented Nov 30, 2018

Vikas-kum left a comment

apeforest Nov 30, 2018

anirudh2290 Nov 30, 2018

azai91 Nov 30, 2018

apeforest Nov 30, 2018

apeforest Nov 30, 2018

apeforest Nov 30, 2018

apeforest left a comment

sandeep-krishnamurthy Dec 1, 2018

TaoLv Dec 1, 2018

azai91 Dec 1, 2018

sandeep-krishnamurthy commented Dec 1, 2018

TaoLv Dec 1, 2018

TaoLv Dec 1, 2018

azai91 Dec 1, 2018

azai91 commented Dec 3, 2018

build mkldnn as static lib on mac/linux #13197

build mkldnn as static lib on mac/linux #13197

Conversation

azai91 commented Nov 9, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

harshp8l commented Nov 9, 2018

kalyc commented Nov 14, 2018

azai91 commented Nov 15, 2018

azai91 commented Nov 20, 2018

TaoLv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azai91 commented Nov 30, 2018

Vikas-kum left a 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

Choose a reason for hiding this comment

apeforest left a 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

sandeep-krishnamurthy commented Dec 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azai91 commented Dec 3, 2018

azai91 commented Nov 9, 2018 •

edited

Loading