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

Performance improving for MKL-DNN Quantized FullyConnected #14528

Merged
merged 3 commits into from
Mar 27, 2019
Merged

Performance improving for MKL-DNN Quantized FullyConnected #14528

merged 3 commits into from
Mar 27, 2019

Conversation

ciyongch
Copy link
Contributor

Description

The patch is mainly for improving the performance of MKL-DNN quantized FullyConnected.

@pengzhao-intel @TaoLv

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

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

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

pengzhao-intel
pengzhao-intel approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

It's good to use an enum instead of the hardcode number in the code.

LGTM.

src/operator/nn/fully_connected-inl.h
@@ -48,6 +48,12 @@ enum FullyConnectedOpResource {kTempSpace};
enum FullyConnectedOpOutputs {kOut};
} // fullc

namespace quantized_fullc {
Copy link
Contributor

Choose a reason for hiding this comment

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

quantized_fc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to align with fullc :)

src/operator/subgraph/mkldnn/mkldnn_fc.cc
@@ -195,6 +195,13 @@ void SgMKLDNNFCOp::Forward(const OpContext &ctx,
}

MKLDNNFCForwardFullFeature(full_param_, ctx, fwd_.get(), new_inputs, new_req, out_data);

if (mkldnn_param.quantized && !mkldnn_param.enable_float_output) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comments on why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's straightforward here, the OutMin and OutMax are only valid when the op is quantized and not generating fp32 output.

@abhinavs95
Copy link
Contributor

abhinavs95 commented Mar 26, 2019 •
edited
Loading

@mxnet-label-bot update [MKLDNN, Performance, pr-awaiting-testing]

@marcoabreu marcoabreu added MKLDNN Performance pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 26, 2019
@pengzhao-intel
Copy link
Contributor

@anirudh2290 @ZhennanQin @xinyu-intel to review

ZhennanQin
ZhennanQin reviewed Mar 27, 2019
View reviewed changes
src/operator/quantization/mkldnn/mkldnn_quantized_fully_connected.cc
@@ -52,15 +47,15 @@ void MKLDNNQuantizedFullyConnectedForward(const nnvm::NodeAttrs &attrs,
NDArray weight = in_data[fullc::kWeight];

const float min_data =
in_data[num_inputs + quantized_fc_enum::kDataMin].data().dptr<float>()[0];
in_data[num_inputs + quantized_fullc::kDataMin].data().dptr<float>()[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite strange usage. Why not define a whole input sets with original inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original inputs might not include bias, which results in different index for all these min/max. Just to simplify the ordering for quantized op only.

ZhennanQin
ZhennanQin approved these changes Mar 27, 2019
View reviewed changes
Copy link
Contributor

@ZhennanQin ZhennanQin left a comment

Choose a reason for hiding this comment

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

LGTM.

@pengzhao-intel
Copy link
Contributor

Thanks for your contribution. Merging now.

@pengzhao-intel pengzhao-intel merged commit 5d2a451 into apache:master Mar 27, 2019
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@ciyongch @vdantu
Performance improving for MKL-DNN Quantized FullyConnected (apache#14528
dbd6c3f 
)

* Cached bias to Quantized FullyCOnnected based on Subgraph to improve performance

* retrigger CI

* retrigger CI
ZhennanQin pushed a commit to ZhennanQin/incubator-mxnet that referenced this pull request Apr 3, 2019
@ciyongch @ZhennanQin
Performance improving for MKL-DNN Quantized FullyConnected (apache#14528
2f66223 
)

* Cached bias to Quantized FullyCOnnected based on Subgraph to improve performance

* retrigger CI

* retrigger CI
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@ciyongch @nswamy
Performance improving for MKL-DNN Quantized FullyConnected (#14528)
c4b8d30 
* Cached bias to Quantized FullyCOnnected based on Subgraph to improve performance

* retrigger CI

* retrigger CI
@ciyongch ciyongch deleted the qfc_perf branch May 22, 2019 02:35
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@ciyongch
Performance improving for MKL-DNN Quantized FullyConnected (apache#14528
7f0bae4 
)

* Cached bias to Quantized FullyCOnnected based on Subgraph to improve performance

* retrigger CI

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

@ZhennanQin ZhennanQin ZhennanQin approved these changes

@pengzhao-intel pengzhao-intel pengzhao-intel approved these changes

@TaoLv TaoLv TaoLv approved these changes

@xinyu-intel xinyu-intel xinyu-intel approved these changes

Assignees
No one assigned
Labels
MKLDNN Performance pr-awaiting-testing PR is reviewed and waiting CI build and test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ciyongch @abhinavs95 @pengzhao-intel @xinyu-intel @TaoLv @ZhennanQin @marcoabreu