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

[v1.x][Feature] Add flag for disabling oneDNN BRGEMM implementation of FC #20450

Merged
merged 3 commits into from
Jul 16, 2021
Merged

[v1.x][Feature] Add flag for disabling oneDNN BRGEMM implementation of FC #20450

merged 3 commits into from
Jul 16, 2021

Conversation

bgawrych
Copy link
Contributor

Description

In new oneDNN version there are BRGEMM kernels for FullyConnected - it require special memory format of weights.
This PR let user to decide if BRGEMM implementation should be used by flag - it can significantly speedup FC execution for large tensors (got 42% speedup on BERT with 64 batch size ) - feature disabled by default as it's not so efficient on small tensors.

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)

@mxnet-bot
Copy link

Hey @bgawrych , 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, sanity, centos-cpu, centos-gpu, windows-gpu, clang, miscellaneous, unix-gpu, unix-cpu, website, windows-cpu]

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.

@mseth10 mseth10 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 Jul 15, 2021
@bgawrych
Copy link
Contributor Author

@mxnet-bot run ci [unix-cpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-cpu]

@mseth10 mseth10 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 Jul 15, 2021
anko-intel
anko-intel approved these changes Jul 15, 2021
View reviewed changes
src/operator/nn/mkldnn/mkldnn_base-inl.h Outdated
@@ -312,7 +312,8 @@ inline static mkldnn::memory::desc GetFCWeightDesc(const NDArray &arr, int dtype
for (size_t i = 0; i < dims.size(); i++) dims[i] = arr.shape()[i];
auto format = mkldnn::memory::format_tag::any;
// for batch 256 alexnet benchmark test
if (dims.size() == 2) {
const bool brgemm_disabled = dmlc::GetEnv("MXNET_DISABLE_ONEDNN_BRGEMM_FC", true);
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be good to add description to docs/static_site/src/pages/api/faq/env_var.md
Also I am not sure if for 1.x branch the name have to include ONEDNN ?
so maybe MXNET_MKLDNN_DISABLE_BRGEMM_FC

@mseth10 mseth10 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 Jul 15, 2021
@bgawrych
Copy link
Contributor Author

@szha Can you help with merge?

sfraczek
sfraczek approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@sfraczek sfraczek left a comment

Choose a reason for hiding this comment

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

LGTM

@mseth10 mseth10 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 Jul 16, 2021
szha
szha approved these changes Jul 16, 2021
View reviewed changes
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

Looks good to me. Ideally we don't want to introduce more environment variables, but instead make the decision automatically on which implementation to use based on input size.

@mseth10 mseth10 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 labels Jul 16, 2021
@anko-intel
Copy link
Contributor

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Unauthorized access detected.
Only following 3 categories can trigger CI :
PR Author, MXNet Committer, Jenkins Admin.

@bgawrych
Copy link
Contributor Author

@mxnet-bot run ci [unix-gpu]

@mxnet-bot
Copy link

Jenkins CI successfully triggered : [unix-gpu]

@mseth10 mseth10 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 Jul 16, 2021
@mseth10 mseth10 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 Jul 16, 2021
@szha szha merged commit a4c4fa0 into apache:v1.x Jul 16, 2021
TaoLv
TaoLv reviewed Aug 10, 2021
View reviewed changes
src/operator/nn/mkldnn/mkldnn_base-inl.h
@@ -312,7 +312,8 @@ inline static mkldnn::memory::desc GetFCWeightDesc(const NDArray &arr, int dtype
for (size_t i = 0; i < dims.size(); i++) dims[i] = arr.shape()[i];
auto format = mkldnn::memory::format_tag::any;
// for batch 256 alexnet benchmark test
if (dims.size() == 2) {
const bool brgemm_disabled = dmlc::GetEnv("MXNET_MKLDNN_DISABLE_BRGEMM_FC", true);
if (dims.size() == 2 && brgemm_disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please provide more benchmarking data with different m/n/k and formats?

BTW, actually brgemm_disabled looks misleading to me. According to the code change, i would rather call the flag force_plain_format or force_ab_format.

@bgawrych bgawrych mentioned this pull request Aug 17, 2021
Merged
bgawrych added a commit to bgawrych/incubator-mxnet that referenced this pull request Sep 3, 2021
@bgawrych
[v1.x][Feature] Add flag for disabling oneDNN BRGEMM implementation o…
17a2af0 
…f FC (apache#20450)

* Add flag for disabling oneDNN BRGEMM implementation of FC

* Review fixes

* Update env_var.md
@bgawrych bgawrych mentioned this pull request Sep 3, 2021
Merged
akarbown pushed a commit that referenced this pull request Sep 6, 2021
@bgawrych
[Backport] Enabling BRGEMM FullyConnected based on shapes (#20568)
1b98299 
* [v1.x][Feature] Add flag for disabling oneDNN BRGEMM implementation of FC (#20450)

* Add flag for disabling oneDNN BRGEMM implementation of FC

* Review fixes

* Update env_var.md

* [v1.x] Enabling BRGEMM FullyConnected based on shapes (#20533)

* Enable brgemm based on input info

* fix sanity

* Review fixes

* Change function name

* Fix typo

* Align variable assignments

* Fix review

* use const reference

* Update flag name
bgawrych added a commit to bgawrych/incubator-mxnet that referenced this pull request Sep 17, 2021
@bgawrych
[v1.x][Feature] Add flag for disabling oneDNN BRGEMM implementation o…
e2d5d09 
…f FC (apache#20450)

* Add flag for disabling oneDNN BRGEMM implementation of FC

* Review fixes

* Update env_var.md
@bgawrych bgawrych mentioned this pull request Sep 17, 2021
Closed
@bgawrych bgawrych mentioned this pull request Feb 22, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TaoLv TaoLv TaoLv left review comments

@szha szha szha approved these changes

@sfraczek sfraczek sfraczek approved these changes

@anko-intel anko-intel anko-intel approved these changes

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

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bgawrych @mxnet-bot @anko-intel @szha @sfraczek @TaoLv @mseth10