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

[RFC] Build with MKL-DNN (or DNNL)#19944

Merged
szha merged 1 commit intoapache:masterfrom
bartekkuncer:buildwithonednn
Mar 15, 2021
Merged

[RFC] Build with MKL-DNN (or DNNL)#19944
szha merged 1 commit intoapache:masterfrom
bartekkuncer:buildwithonednn

Conversation

@bartekkuncer
Copy link
Contributor

@bartekkuncer bartekkuncer commented Feb 23, 2021 •
edited
Loading

From #19610:

Intel MKL-DNN was renamed with DNNL in its v1.1 release. Since then, the MXNet community has been working on the transition to DNNL to leverage the latest features and optimizations from the library. That includes using the string “DNNL” or “dnnl” for future development and communication. We propose to promote the flag “USE_DNNL” since MXNet 2.0 and start deprecating “USE_MKLDNN” at the same time.
DNNL source code resides in the 3rdparty/mkldnn folder of the MXNet repository and is released and distributed along with MXNet source code. If one wants to build MXNet with DNNL to accelerate the execution on Intel CPU, she/he needs to enable -DUSE_DNNL=ON in CMake. However, this flag has been set to ON by default for all platforms except edge devices. On the contrary, to disable the DNNL acceleration, one needs to set -DUSE_DNNL=OFF explicitly in the CMake command line or the CMake configuration file.
As both MXNet and DNNL are under quick development with different release cadence, we decide to link the DNNL library into MXNet statically to avoid mis-linking in the user's environment. Given this, we need to set DNNL_LIBRARY_TYPE to STATIC when building DNNL.
Some additional flags to build DNNL:

DNNl_CPU_RUNTIME: Need set it to SEQ explicitly when USE_OPENMP=OFF;
DNNL_ARCH_OPT_FLAGS: Need pass compiler options to this build flag in string. Eg. -march or -mtune for GCC.
MKLDNN_BUILD_TESTS and MKLDNN_BUILD_EXAMPLES: We set these two flags to OFF to speed up the compilation.
One thing that needs to be taken care of is that the header dnnl_config.h and dnnl_version.h will be generated dynamically during compilation and will be copied to the installation destination when calling make install. That means these two headers are not distributed with DNNL source code. For downstream projects which are including these headers need to find them in the installation path rather than the source code path.

I prepared three commits regarding this point of main RFC:

  1. changing USE_MKLDNN flag name to USE_ONEDNN to make it consistent with actual library name
    I believe that this commit is complete and if there is such a will can be merged into master.

  2. changing MXNET_USE_MKLDNN* flags names to MXNET_USE_ONEDNN* also for consistency reasons
    This commit regards changing inner MXNET flag so that it will be consistent with the actual lib name. To avoid creating even bigger mixture of mkldnn/dnnl/onednn acronyms I believe it should be accompanied with another commit changing acronyms used in mxnet function names and comments regarding this particular lib to oneDNN.

  3. changing the 3rdparty/mkldnn folder name to 3rdparty/onednn for consistency.

@mxnet-bot
Copy link

mxnet-bot commented Feb 23, 2021

Hey @bartekkuncer , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [edge, unix-cpu, windows-gpu, centos-cpu, sanity, centos-gpu, miscellaneous, windows-cpu, unix-gpu, clang, website]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Feb 23, 2021
@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 23, 2021

CC: @leezu, @TaoLv, @anko-intel, @szha, @akarbown

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 23, 2021

@mxnet-bot run ci [centos-gpu, windows-cpu]

@mxnet-bot
Copy link

mxnet-bot commented Feb 23, 2021

Jenkins CI successfully triggered : [centos-gpu, windows-cpu]

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Feb 23, 2021
@szha
Copy link
Member

szha commented Feb 23, 2021

@mxnet-bot run ci [windows-gpu]

@mxnet-bot
Copy link

mxnet-bot commented Feb 23, 2021

Jenkins CI successfully triggered : [windows-gpu]

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Feb 23, 2021
@samskalicky
Copy link
Contributor

samskalicky commented Feb 23, 2021

There is also another idea to change the 3rdparty/mkldnn folder name to 3rdparty/onednn for consistency. The problem is that it will create unnecessary difference in directories names between branches.

since this is the master branch, and we're moving from 1.x to 2.0 shouldnt this be the time to make this change? Having a directory named "mkldnn" but using "onednn" everywhere will only cause confusion in the future for those who do not have the historical background.

@lanking520 lanking520 added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Feb 23, 2021
@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 24, 2021 •
edited
Loading

There is also another idea to change the 3rdparty/mkldnn folder name to 3rdparty/onednn for consistency. The problem is that it will create unnecessary difference in directories names between branches.

since this is the master branch, and we're moving from 1.x to 2.0 shouldnt this be the time to make this change? Having a directory named "mkldnn" but using "onednn" everywhere will only cause confusion in the future for those who do not have the historical background.

@samskalicky Ok, will add it soon.

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 24, 2021

Added commit regarding changing name of the submodule directory.

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 24, 2021

I think that the commits regarding updating onednns old acronyms used in mxnet in different contexts can be added to separate PR and if there is no objection this one can be already merged into master to avoid creating code with deprecated versions of the flags.

@samskalicky
Copy link
Contributor

samskalicky commented Feb 24, 2021

I think that the commits regarding updating onednns old acronyms used in mxnet in different contexts can be added to separate PR and if there is no objection this one can be already merged into master to avoid creating code with deprecated versions of the flags.

Can you give an example, which acronyms are you referring to?

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Feb 25, 2021

I think that the commits regarding updating onednns old acronyms used in mxnet in different contexts can be added to separate PR and if there is no objection this one can be already merged into master to avoid creating code with deprecated versions of the flags.

Can you give an example, which acronyms are you referring to?

Before v1.1 of oneDNN it was called MKLDNN, then dnnl up until some time ago. Due to that we have a mixture of different acronyms used in the code and unifying them using the most recent one can help avoid some unnecessary confusion.

@lanking520 lanking520 added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 8, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Mar 8, 2021
@bartekkuncer bartekkuncer force-pushed the buildwithonednn branch 2 times, most recently from a5ad223 to 4f43d96 Compare March 8, 2021 15:45
@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 8, 2021

@mxnet-bot run ci [website,windows-gpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 8, 2021

Jenkins CI successfully triggered : [website, windows-gpu]

@lanking520 lanking520 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress labels Mar 8, 2021
@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 9, 2021

@mxnet-bot run ci [centos-cpu,unix-cpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 9, 2021

Jenkins CI successfully triggered : [unix-cpu, centos-cpu]

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 9, 2021

@szha @samskalicky @leezu Hi, I am struggling to find the reason why website and windows-gpu checks are failing. I see in the logs that there was a problem during git merge but locally, here on github and in other checks I do not see such problems. It seems like all the failing checks are run on the same machine Running on utility_0se3punbeq in /home/jenkins_slave/workspace/xnet-validation_website_PR-19944. Do you know what is/might be the problem here or have an advice on how should I proceed to find the issue in the fastest way? Maybe there are some artifacts which interrupt the merge? Thanks for your attention. I’m looking forward to your reply.

@szha
Copy link
Member

szha commented Mar 10, 2021

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

mxnet-bot commented Mar 10, 2021

Jenkins CI successfully triggered : [sanity, centos-gpu, edge, windows-cpu, clang, miscellaneous, website, centos-cpu, windows-gpu, unix-gpu, unix-cpu]

@szha
Copy link
Member

szha commented Mar 10, 2021

Looks like the merge issue still exists and I'm still not sure what's causing it after searching jenkins jira (e.g. this one. Maybe you could try squashing the 6 commits to a single one and push again?

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 11, 2021

@mxnet-bot run ci [unix-cpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 11, 2021

Jenkins CI successfully triggered : [unix-cpu]

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 12, 2021

@mxnet-bot run ci [unix-cpu, centos-cpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 12, 2021

Jenkins CI successfully triggered : [unix-cpu, centos-cpu]

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 12, 2021

@mxnet-bot run ci [unix-cpu, centos-cpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 12, 2021

Jenkins CI successfully triggered : [unix-cpu, centos-cpu]

@bartekkuncer
Change inner mxnet flags nomenclature for oneDNN library
60cda22 
This change includes:
* changing MXNET_USE_MKLDNN flag name to MXNET_USE_ONEDNN
* changing USE_MKLDNN flag name to USE_ONEDNN
* changing 3rdparty/mkldnn folder name to 3rdparty/onednn
* changing include/mkldnn folder name to include/onednn
* changing MKLDNN occurences in build and documentation files to ONEDNN
* adding Bartosz Kuncer to contributors list
@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 15, 2021

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 15, 2021

Jenkins CI successfully triggered : [unix-gpu]

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 15, 2021

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

mxnet-bot commented Mar 15, 2021

Jenkins CI successfully triggered : [unix-gpu]

@bartekkuncer
Copy link
Contributor Author

bartekkuncer commented Mar 15, 2021

@szha @leezu @samskalicky Please help with the merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aaronmarkham aaronmarkham Awaiting requested review from aaronmarkham aaronmarkham is a code owner

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

@marcoabreu marcoabreu Awaiting requested review from marcoabreu marcoabreu is a code owner

@szha szha Awaiting requested review from szha szha is a code owner

3 more reviewers

@anko-intel anko-intel anko-intel left review comments

@leezu leezu leezu approved these changes

@samskalicky samskalicky samskalicky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bartekkuncer @mxnet-bot @szha @samskalicky @TaoLv @leezu @anko-intel @lanking520