Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-74]Update mkldnn to the newest & Add clang build test with mkldnn. #9918

Merged
merged 17 commits into from
Mar 14, 2018
Merged

[MXNET-74]Update mkldnn to the newest & Add clang build test with mkldnn. #9918

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
de64891
add clang test with mkldnn
xinyu-intel Feb 28, 2018
cbc2699
update mkldnn
xinyu-intel Feb 28, 2018
8feeb90
Update mkldnn to the newest & Add clang build test with mkldnn. #9918
xinyu-intel Mar 1, 2018
8126acb
update all dockerfiles in order to use the correct mkl version
xinyu-intel Mar 1, 2018
f87cdb6
solve conflict on Dockerfile
xinyu-intel Mar 1, 2018
58f3844
fix bugs in mkldnn_base
xinyu-intel Mar 1, 2018
a297465
update mklml link and modify dockerfiles
xinyu-intel Mar 1, 2018
c9e45ac
delete blank line
xinyu-intel Mar 1, 2018
a258d08
no changes, just retrigger.
xinyu-intel Mar 2, 2018
cb7c8fb
no changes, just retriggering
xinyu-intel Mar 4, 2018
781ec00
put mklml in a single run file
xinyu-intel Mar 5, 2018
2c7647b
debug mklml run file
xinyu-intel Mar 5, 2018
b7be591
debug mklml script
Mar 5, 2018
b246a05
resolve conflicts
xinyu-intel Mar 12, 2018
98484d9
add mkldnn clang test
xinyu-intel Mar 12, 2018
17747f8
give each build task its own unique workspace
xinyu-intel Mar 13, 2018
bc5d44c
change lib name
xinyu-intel Mar 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion 3rdparty/mkldnn
Open in desktop
Submodule mkldnn updated from 283c4a to 464c26
18 changes: 18 additions & 0 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,24 @@ try {
}
}
},
'CPU: Clang 3.9 MKLDNN': {
node('mxnetlinux-cpu') {
ws('workspace/build-cpu-clang39') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're trying to give each build task its own unique workspace. I'd recommend we change this to:

ws('workspace/build-cpu-mkldnn-clang39')

init_git()
sh "ci/build.py --build --platform ubuntu_cpu /work/runtime_functions.sh build_ubuntu_cpu_clang39_mkldnn"
pack_lib('mkldnn_cpu', mx_mkldnn_lib)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate name

}
}
},
'CPU: Clang 5 MKLDNN': {
node('mxnetlinux-cpu') {
ws('workspace/build-cpu-clang50') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and

ws('workspace/build-cpu-mkldnn-clang50')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I am going to fix it:)

init_git()
sh "ci/build.py --build --platform ubuntu_cpu /work/runtime_functions.sh build_ubuntu_cpu_clang50_mkldnn"
pack_lib('mkldnn_cpu', mx_mkldnn_lib)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate name

}
}
},
'CPU: MKLDNN': {
node('mxnetlinux-cpu') {
ws('workspace/build-mkldnn-cpu') {
Expand Down
2 changes: 1 addition & 1 deletion ci/docker/install/ubuntu_mklml.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@
# the whole docker cache for the image

set -ex
wget --no-check-certificate -O /tmp/mklml.tgz https://github.com/01org/mkl-dnn/releases/download/v0.12/mklml_lnx_2018.0.1.20171227.tgz
wget --no-check-certificate -O /tmp/mklml.tgz https://github.com/intel/mkl-dnn/releases/download/v0.12/mklml_lnx_2018.0.1.20171227.tgz
tar -zxvf /tmp/mklml.tgz && cp -rf mklml_*/* /usr/local/ && rm -rf mklml_*
26 changes: 26 additions & 0 deletions ci/docker/runtime_functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,32 @@ build_ubuntu_cpu_clang50() {
-j$(nproc)
}

build_ubuntu_cpu_clang39_mkldnn() {
set -ex
make \
USE_PROFILER=1 \
USE_CPP_PACKAGE=1 \
USE_BLAS=openblas \
USE_MKLDNN=1 \
USE_OPENMP=0 \
CXX=clang++-3.9 \
CC=clang-3.9 \
-j$(nproc)
}

build_ubuntu_cpu_clang50_mkldnn() {
set -ex
make \
USE_PROFILER=1 \
USE_CPP_PACKAGE=1 \
USE_BLAS=openblas \
USE_MKLDNN=1 \
USE_OPENMP=1 \
CXX=clang++-5.0 \
CC=clang-5.0 \
-j$(nproc)
}

build_ubuntu_cpu_mkldnn() {
set -ex
make \
Expand Down
3 changes: 2 additions & 1 deletion src/operator/nn/mkldnn/mkldnn_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,13 +237,14 @@ mkldnn_memory_format_t GetDefaultFormat(mkldnn::memory::desc desc) {
case mkldnn_gOIhw16o16i:
case mkldnn_gIOhw16o16i:
case mkldnn_gOihw8o:
case mkldnn_Goihw8g:
case mkldnn_gOihw16o:
case mkldnn_gOhwi8o:
case mkldnn_gOhwi16o:
case mkldnn_gOhIw16o4i:
return mkldnn_goihw;
default:
LOG(FATAL) << "Unknown MKLDNN format for 4 dimensions: " << desc.data.format;
LOG(FATAL) << "Unknown MKLDNN format for 5 dimensions: " << desc.data.format;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We notice that there is a test case failed here and it was caused by a missing data format "mkldnn_Goihw8g". Then we found the error message is misleading.
Please find the error message here: http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-9918/4/pipeline/513

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah great. But the error message is indeed pretty misleading. Would it be possible to make a better fitting test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean a negative test case which can run into the default statement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because all the 5D data formats that mkldnn supported currently are covered by the swich-case statement here. So I don't think we can create a mkldnn::memory::desc with another 5D data format to touch the default statement.
One feasible method is to set a test case to check whether there is a new format added into mkldnn data format list. If the test case is failed, maybe we need check the code here and add the new format into the switch-case statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a good idea! Especially considering that updating the MKLDNN submodule requires a PR, problems like this would be caught during an early stage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoabreu Sorry for late response. It seems a little difficult to monitor the change of a enum type in mkldnn package. I have asked mkldnn team for support and am waiting for their reply. Since the mkldnn version updated in this pr works well with this part of code, I would like this pr be merged firstly and it will address the compilation issue on OSX. I will add unit test for it later if I get support from mkldnn team. If needed, I can help create a JIRA ticket to track this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you create the jira issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created. MXNET-98

return mkldnn_format_undef;
}
} else {
Expand Down