[MKLDNN] Enable convolution fusion. #12308

ZhennanQin · 2018-08-23T08:37:51Z

Description

Implement mkldnn convlution fusion(eg. conv+relu, conv+bn, conv+sum) based on subgraph.
@pengzhao-intel @TaoLv @zheng-da @reminisce

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

marcoabreu · 2018-08-23T08:50:59Z

src/operator/nn/mkldnn/mkldnn_convolution-inl.h

+  }
+  const int GetHash() const {
+    int hash = 0;
+    hash = hash * 2 + this->with_bn ? 1 : 0;


Possible hash collision: with_bn=0 and with_relu=1 equals BN=1 and relu0. Consider using bitflags

why not std::hash?

lebeg · 2018-08-24T13:47:38Z

src/operator/nn/convolution.cc

+
+static inline std::string PrintArguments(const ConvolutionParam& param_) {
+  auto args = ListArguments(param_);
+  std::string str = "[";


It's better to use std::stringstream to compose such string, otherwise it's a lot of redundant copying internally.

lebeg · 2018-08-24T13:49:11Z

src/operator/nn/mkldnn/mkldnn_convolution-inl.h

+    DMLC_DECLARE_FIELD(with_postsum_relu).set_default(false)
+    .describe("Add post relu after sum");
+  }
+  const int GetHash() const {


Can std::string be used here instead?

lebeg · 2018-08-24T14:00:17Z

src/operator/subgraph/mkldnn/mkldnn_conv.cc

+  LOG(INFO) << "Conv req size: " << req.size();
+  for (size_t k = 0; k < inputs.size(); ++k) {
+    auto input = inputs[k];
+    printf("input %ld :", k);


Sometimes it's LOG(INFO), sometimes it's printf. I think it's better to use LOG always.

lebeg · 2018-08-24T14:02:36Z

src/operator/subgraph/mkldnn/mkldnn_conv.cc

+  if (it != attrs.dict.end() && it->second == "true") {
+    it = attrs.dict.find("in_sum_at_begin");
+    if (it != attrs.dict.end() && it->second == "true") {
+      return std::vector<std::pair<int, int>>{std::pair<int, int>{0, 0}};


You can use std::make_pair and ommit the template argument types

lebeg · 2018-08-24T14:04:29Z

src/operator/subgraph/mkldnn/mkldnn_conv.cc

+                            DefaultSubgraphOpResourceRequest)
+.set_attr<std::string>("key_var_num_args", "num_args")
+.set_attr<nnvm::FInplaceOption>("FInplaceOption", [](const nnvm::NodeAttrs
+                                                          &attrs) {


If you specify the return type, like -> std::vector<std::pair<int, int>> then you can do things like return {}; further in the lambda or just return std::make_pair(0, 0).

lebeg · 2018-08-24T14:06:08Z

src/operator/subgraph/mkldnn/mkldnn_conv.h

+class SgMKLDNNConvSelector : public SubgraphSelector {
+ public:
+  /*! \brief pattern match status */
+  enum SelectStatus {


It's better to use typed enums since they have better scoping and you don't need any prefixes or all caps. Usage will be like: SelectStatus.Fail

lebeg · 2018-08-24T14:07:24Z

src/operator/subgraph/mkldnn/mkldnn_conv.h

+          if (new_node.inputs[1].node.get() == &n) {
+            sum_entry = new_node.inputs[0];
+          }
+          #if 0


Is this code needed?

lebeg · 2018-08-24T14:08:58Z

src/operator/subgraph/mkldnn/mkldnn_conv.h

+    auto last_node = sym.outputs[0].node;
+    nnvm::Symbol new_sym;
+    new_sym.outputs.emplace_back(nnvm::NodeEntry{last_node, 0, 0});
+    std::string node_name = "";


std::stringstream

lebeg · 2018-08-24T14:10:44Z

src/operator/subgraph/mkldnn/mkldnn_conv.cc

+  ConvFusionFallBackCompute();
+}
+
+class SgMKLDNNConvOperator {


Why not have the definition in the header file?

lebeg · 2018-08-24T14:11:33Z

src/operator/subgraph/mkldnn/mkldnn_conv.h

+  nnvm::NodeEntry conv_data;
+  std::vector<const nnvm::Node *> matched_list;
+
+  bool HandleMatchStatus() {


Why not have the implementations for all methods in the .cc file?

ZhennanQin · 2018-08-28T07:01:09Z

@marcoabreu @lebeg @larroy. Thanks for your reviewing comments, I will address them in next version.

stu1130 · 2018-09-18T00:02:38Z

@ZhennanQin Thanks for your contribution. Could you address the feedback? Let us know if you need help!

vandanavk · 2018-09-24T23:44:14Z

@mxnet-label-bot [pr-awaiting-response]

vrakesh · 2018-10-09T07:09:21Z

@ZhennanQin Requesting an update on the PR, have the changes been addressed?

ZhennanQin · 2018-10-09T07:24:09Z

@vrakesh @stu1130 Sorry for responding late. This PR is combined into #12530, leaving as a reference for the fusion part change. Shall I close this PR?

[MKLDNN] Enable convolution fusion.

3047d9d

ZhennanQin requested a review from anirudh2290 as a code owner August 23, 2018 08:37

marcoabreu reviewed Aug 23, 2018

View reviewed changes

lebeg reviewed Aug 24, 2018

View reviewed changes

marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Sep 24, 2018

ZhennanQin closed this Oct 10, 2018

ZhennanQin deleted the conv_fusion branch October 10, 2018 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MKLDNN] Enable convolution fusion. #12308

[MKLDNN] Enable convolution fusion. #12308

ZhennanQin commented Aug 23, 2018 •

edited

Loading

marcoabreu Aug 23, 2018

larroy Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

lebeg Aug 24, 2018

ZhennanQin commented Aug 28, 2018

stu1130 commented Sep 18, 2018 •

edited

Loading

vandanavk commented Sep 24, 2018

vrakesh commented Oct 9, 2018

ZhennanQin commented Oct 9, 2018

[MKLDNN] Enable convolution fusion. #12308

[MKLDNN] Enable convolution fusion. #12308

Conversation

ZhennanQin commented Aug 23, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

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

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

ZhennanQin commented Aug 28, 2018

stu1130 commented Sep 18, 2018 • edited Loading

vandanavk commented Sep 24, 2018

vrakesh commented Oct 9, 2018

ZhennanQin commented Oct 9, 2018

ZhennanQin commented Aug 23, 2018 •

edited

Loading

stu1130 commented Sep 18, 2018 •

edited

Loading