You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/07/09 13:25:02 UTC

[GitHub] [incubator-mxnet] sfraczek opened a new pull request #20430: [FEATURE] Asymmetric fc fc

sfraczek opened a new pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430


   ## Description ##
   Added asymmetric quantization to fc fc pattern and refactoring by Dominika. It is recommended to look at commits separately because refactoring is in separate commit to make review easier.
   
   ## Checklist ##
   ### Essentials ###
   - [x ] PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
   - [ x] Changes are complete (i.e. I finished coding on this PR)
   - [ x] All changes have test coverage
   - [ ] Code is well-documented
   
   ### Changes ###
   Added fc-fc to asymmetric quantization
   refactoring
   
   This is second of asymmetric quantization optimizations applied during mlperf optimization. There will be small change when mkldnn is updated.
   
   Single image calibration result
   ```
   MXNET_DISABLE_SHIFTED_QUANTIZATION_OPTIMIZATIONS=1 {"exact_match": 81.90160832544939, "f1": 89.5201243007145}
   MXNET_DISABLE_SHIFTED_QUANTIZATION_OPTIMIZATIONS=0 {"exact_match": 82.09082308420057, "f1": 89.79067425156269}
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-877185604


   Hey @sfraczek , 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**: [clang, miscellaneous, centos-cpu, centos-gpu, edge, windows-cpu, sanity, windows-gpu, unix-gpu, unix-cpu, 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. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670185458



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;
+  auto min_data = std::stof(quantize->attrs.dict.at("min_calib_range"));
+  auto max_data = std::stof(quantize->attrs.dict.at("max_calib_range"));
+
+  FCInputIndex id(nnvm::get<MKLDNNFCFullParam>(fc->attrs.parsed));

Review comment:
       But code in lines 109&110 will have 101 columns and formatter does hideous things with it so I would appreciate a suggestion how to break it if I am supposed to make it "idx".




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-888092720


   Jenkins CI successfully triggered : [windows-gpu]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r672972992



##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file asymmetric_quantize_graph_pass.cc
+ * \brief
+ */
+#if MXNET_USE_MKLDNN == 1
+#include "quantize_graph_pass.h"
+
+namespace mxnet {
+namespace op {
+namespace asym_quant {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1

Review comment:
       ok

##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,67 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+#if MXNET_USE_MKLDNN == 1

Review comment:
       ok




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-887283221


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-893520454


   I like it the way Andrzej wrote it. I'm open to suggestions but I don't see any better obvious solution.
   Bert uses those optimizations. 
   It is disabled by default. It should be enabled when we track down accuracy problem on other machine. Preferably after one more optimization which will be pushed when OneDNN is upgraded to newer version (zero point reorder is on public master available but not on current release).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek edited a comment on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek edited a comment on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-894166459


   I mean FullyConnected->FullyConnected. When there are two FullyConnected layers we shift output from first FC and compensate in bias of second FC. This makes second FC operator change input dtype from s8 to u8, which runs faster on OneDNN.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670185458



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;
+  auto min_data = std::stof(quantize->attrs.dict.at("min_calib_range"));
+  auto max_data = std::stof(quantize->attrs.dict.at("max_calib_range"));
+
+  FCInputIndex id(nnvm::get<MKLDNNFCFullParam>(fc->attrs.parsed));

Review comment:
       But code in lines 109&110 will have 101 columns and my formatter does hideous things with it so I would appreciate a suggestion how to break it if I am supposed to make it "idx".




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-888092659


   @mxnet-bot run ci [windows-gpu]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] anko-intel commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
anko-intel commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r674022121



##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -36,8 +36,7 @@ using nnvm::NodeEntry;
 using nnvm::Graph;
 
 template <bool require_bias>
-static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
-#if MXNET_USE_MKLDNN == 1
+bool IsOneDNNFullyConnected(const ObjectPtr& n) {

Review comment:
       Please do not remove "static". Otherwise not required function by other modules will be visible outside this module. IT potentially could increase linking time




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670390568



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;

Review comment:
       ok




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] akarbown merged pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
akarbown merged pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mozga-intel commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mozga-intel commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-893503256


   We need to figure out how to remove some duplications i.e: `QuantizeFcShiftedQuantization` `FcFcShiftedQuantization` [(here](https://github.com/apache/incubator-mxnet/pull/20430/files#diff-93eeb14143daedfc06e4bf9be25dd477cd13ad536c124e45bed92e1787101854R139)). Calculate a position of particular input in the input vector could be more error-proofing. It could refer to the implementation of a more specific structure that gives an ability to set up those attributes. [here](https://github.com/apache/incubator-mxnet/pull/20430/files#diff-fe163642508b98c6d809888be8dad197d653e7b602a639846751a7ac1bcc64c8R111). In this way we can lack errors and unexpected mistakes.  
   
   But otherwise, this looks good! Thanks! 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r672972992



##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file asymmetric_quantize_graph_pass.cc
+ * \brief
+ */
+#if MXNET_USE_MKLDNN == 1
+#include "quantize_graph_pass.h"
+
+namespace mxnet {
+namespace op {
+namespace asym_quant {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1

Review comment:
       ok

##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,67 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+#if MXNET_USE_MKLDNN == 1

Review comment:
       ok




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670198764



##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,39 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+// Forward declaration
+class FCInputIndex {

Review comment:
       Ok.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-894166459


   I mean FullyConnected->FullyConnected. When there are two FullyConnected layers we shift output from first FC and compensate in bias of second FC. This makes second FC operator change input dtype from s8 to u8 input, which runs faster on OneDNN.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] bgawrych commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
bgawrych commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r668865247



##########
File path: tests/python/quantization/test_quantization.py
##########
@@ -1348,6 +1348,64 @@ def check(number, qdtype):
             check(i, qdtype)
 
 
+@with_seed()
+def test_onednn_shifted_fc_fc():
+    batch_size = 2
+    if not is_test_for_mkldnn():
+        print("Test only for mkldnn")
+        return
+
+    def get_fc_fc_layers():
+        net = mx.gluon.nn.HybridSequential()
+        with net.name_scope():
+            net.add(mx.gluon.nn.Dense(2, use_bias=True, flatten=True,
+                                      weight_initializer=mx.initializer.Normal(),
+                                      bias_initializer=mx.initializer.Normal()))
+            net.add(mx.gluon.nn.Dense(2, use_bias=True, flatten=True,
+                                      weight_initializer=mx.initializer.Normal(),
+                                      bias_initializer=mx.initializer.Normal()))
+        net.initialize()
+        return net
+
+    def quantize_net(net, qdtype, random_data):
+        calib_data = NDArrayIter(data=random_data, batch_size=batch_size)
+        calib_data = DummyIter(calib_data)
+        net = mx.contrib.quant.quantize_net(net, quantized_dtype=qdtype,
+                                            exclude_layers=None,
+                                            exclude_layers_match=[],
+                                            calib_data=calib_data,
+                                            calib_mode='naive',
+                                            num_calib_examples=1,
+                                            ctx=mx.current_context())
+        net.hybridize(static_alloc=True, static_shape=True)
+        print("calibrated, now run to get symbol")
+        out = net(random_data)
+        out.wait_to_read()
+
+        _, sym = net._cached_graph
+        fc_attrs = sym.attr_dict()['quantized_sg_mkldnn_fully_connected_0']
+        return fc_attrs, out
+
+    def check(qdtype, random_data):
+        net_ref = get_fc_fc_layers()
+        out_ref = net_ref(random_data)
+        out_ref.wait_to_read()
+
+        fc_attrs, out_q = quantize_net(net_ref, qdtype, random_data)
+
+        assert_almost_equal(out_ref, out_q)
+
+        if qdtype == 'auto':
+            assert fc_attrs['shifted_output'] == 'True'
+        else:
+            assert 'shifted' not in fc_attrs
+
+    with environment({'MXNET_DISABLE_SHIFTED_QUANTIZATION_OPTIMIZATIONS': '0',
+        'MXNET_DISABLE_SHIFTED_QUANTIZE_FC_OPTIMIZATION': '1'}):

Review comment:
       Can you align this env variables?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670386574



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());

Review comment:
       ok




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-878922336


   Jenkins CI successfully triggered : [unix-gpu]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670413549



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;
+  auto min_data = std::stof(quantize->attrs.dict.at("min_calib_range"));
+  auto max_data = std::stof(quantize->attrs.dict.at("max_calib_range"));
+
+  FCInputIndex id(nnvm::get<MKLDNNFCFullParam>(fc->attrs.parsed));
+  auto in = fc->inputs;
+
+  float *min_weight = FindInArgByName(g, in[id.weight_min].node->attrs.name)->data().dptr<float>();

Review comment:
       thank you




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-878922285


   @mxnet-bot run ci [unix-gpu]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mozga-intel commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mozga-intel commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-887283171


   @mxnet-bot run ci [all]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] TaoLv commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
TaoLv commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-893551015


   what's the "fc fc pattern" in the description?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-887287960


   @mxnet-bot run ci [all]


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sfraczek edited a comment on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
sfraczek edited a comment on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-893520454


   I like it the way Andrzej wrote addressing. I'm open to suggestions but I don't see any better obvious solution. Duplication might be removed in near future PR. I don't want to mess up things more right now because other work depends on it.
   Bert uses those optimizations. 
   It is disabled by default. It should be enabled when we track down accuracy problem on other machine. Preferably after one more optimization which will be pushed when OneDNN is upgraded to newer version (zero point reorder is on public master available but not on current release).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] anko-intel commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
anko-intel commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r670199194



##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;
+  auto min_data = std::stof(quantize->attrs.dict.at("min_calib_range"));
+  auto max_data = std::stof(quantize->attrs.dict.at("max_calib_range"));
+
+  FCInputIndex id(nnvm::get<MKLDNNFCFullParam>(fc->attrs.parsed));
+  auto in = fc->inputs;
+
+  float *min_weight = FindInArgByName(g, in[id.weight_min].node->attrs.name)->data().dptr<float>();

Review comment:
       One suggestion below:
   ```suggestion
     float *min_weight = 
       FindInArgByName(g, in[idx.weight_min].node->attrs.name)->data().dptr<float>();
   ```
   or maybe add another helper function which already return pointer to float to avoid "->data().dptr<float>()" in the end of each line 
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mozga-intel edited a comment on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mozga-intel edited a comment on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-893503256


   We need to figure out how to remove some duplications i.e: `QuantizeFcShiftedQuantization` `FcFcShiftedQuantization` [(here](https://github.com/apache/incubator-mxnet/pull/20430/files#diff-93eeb14143daedfc06e4bf9be25dd477cd13ad536c124e45bed92e1787101854R139)). Calculate a position of particular input in the input vector could be more error-proofing. It could refer to the implementation of a more specific structure that gives an ability to set up those attributes. [here](https://github.com/apache/incubator-mxnet/pull/20430/files#diff-fe163642508b98c6d809888be8dad197d653e7b602a639846751a7ac1bcc64c8R111). In this way we can lack errors and unexpected mistakes.  
   
   
   Last questions: 
   - Could you please tell me which model uses that? 
   - Does the following patten is disabled by default, If the answer is 'yes', will we have a plan to enable those things?
   
   But otherwise, this looks good! Thanks! 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r671273941



##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,67 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+#if MXNET_USE_MKLDNN == 1

Review comment:
       This #if is redundant, please have a look at the line: 30 (we don't need to have double-check) 

##########
File path: src/operator/quantization/asymmetric_quantize_graph_pass.cc
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file asymmetric_quantize_graph_pass.cc
+ * \brief
+ */
+#if MXNET_USE_MKLDNN == 1
+#include "quantize_graph_pass.h"
+
+namespace mxnet {
+namespace op {
+namespace asym_quant {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1

Review comment:
       [double-check] 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mozga-intel commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mozga-intel commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r671273941



##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,67 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+#if MXNET_USE_MKLDNN == 1

Review comment:
       This #if is redundant, please have a look at the line: 30 (we don't need to check it again ) 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] anko-intel commented on a change in pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
anko-intel commented on a change in pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#discussion_r669900780



##########
File path: src/operator/nn/mkldnn/mkldnn_fully_connected-inl.h
##########
@@ -80,6 +86,39 @@ struct MKLDNNFCFullParam {
   std::vector<float> output_scales = {0.0f};
 };
 
+// Forward declaration
+class FCInputIndex {

Review comment:
       Why not put the full implementation of FCInputIndex here?

##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;

Review comment:
       instead of 0 it could be idx.data

##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());
+    }
+  }
+#endif
+  return false;
+}
+
+static inline bool IsQuantize(const ObjectPtr& n) {
+  if (n->op() == Op::Get("_contrib_quantize_v2")) {
+    auto const &param = nnvm::get<QuantizeV2Param>(n->attrs.parsed);
+    if (param.min_calib_range.has_value() &&
+        param.min_calib_range.value() < 0.0f) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static NDArray* FindInArgByName(const Graph &g, const std::string& name) {
+  const std::vector<std::string>& in_arg_names =
+      g.GetAttr<std::vector<std::string>>("in_arg_names");
+  size_t i = std::distance(in_arg_names.begin(),
+                           std::find(in_arg_names.begin(), in_arg_names.end(), name));
+  if (i == in_arg_names.size()) {
+    LOG(FATAL) << name << " not found in in_arg_names";
+  }
+  return g.GetAttr<NDArray **>("in_args")[i];
+}
+
+// Rescales weights, min_weight and max_weight. Returns bias_int32_rescale.
+static inline float RescaleWeights(const Graph &g, const ObjectPtr &fc, NDArray* weight_tensor) {
+  ObjectPtr &quantize = fc->inputs[0].node;
+  auto min_data = std::stof(quantize->attrs.dict.at("min_calib_range"));
+  auto max_data = std::stof(quantize->attrs.dict.at("max_calib_range"));
+
+  FCInputIndex id(nnvm::get<MKLDNNFCFullParam>(fc->attrs.parsed));

Review comment:
       I call it "idx" in mkldnn_fc.cc - could you rename it to have the same convention?

##########
File path: src/operator/quantization/quantize_graph_pass.h
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2021 by Contributors
+ * \file quantize_graph_pass.h
+ * \brief
+ */
+#ifndef MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+#define MXNET_OPERATOR_QUANTIZATION_QUANTIZE_GRAPH_PASS_H_
+
+#include <mxnet/op_attr_types.h>
+#include <nnvm/graph.h>
+#include <nnvm/pass.h>
+#include <queue>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+#include <string>
+#include "quantize_v2-inl.h"
+#include "../nn/mkldnn/mkldnn_fully_connected-inl.h"
+#include "../../common/utils.h"
+
+namespace mxnet {
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::ObjectPtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+inline ObjectPtr CreateNode(std::string op_name, std::string node_name) {
+  ObjectPtr node = Node::Create();
+  node->attrs.name = node_name;
+  if (op_name == "nullptr") {
+    node->attrs.op = nullptr;
+    // ugly workaround because VariableParam is not exposed
+    node->attrs.parsed =
+      nnvm::Symbol::CreateVariable(node->attrs.name).outputs[0].node->attrs.parsed;
+  } else {
+    node->attrs.op = Op::Get(op_name);
+  }
+  return node;
+}
+
+template <bool require_bias>
+static inline bool IsOneDNNFullyConnected(const ObjectPtr& n) {
+#if MXNET_USE_MKLDNN == 1
+  if (n->op() == Op::Get("_sg_mkldnn_fully_connected")) {
+    auto const& param = nnvm::get<MKLDNNFCFullParam>(n->attrs.parsed);
+    if (!(param.mkldnn_param.channel_wise_quantize.has_value() &&
+          param.mkldnn_param.channel_wise_quantize.value())) {
+      return !require_bias || (param.default_param.no_bias == false &&
+                               n->inputs[2].node->is_variable());

Review comment:
       instead of "2" it could be idx.bias




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #20430: [FEATURE] Asymmetric fc fc

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #20430:
URL: https://github.com/apache/incubator-mxnet/pull/20430#issuecomment-887288058


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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@mxnet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org