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/09/28 20:01:59 UTC

[GitHub] [incubator-mxnet] bgawrych commented on a change in pull request #20599: [FEATRURE] Add fusing FullyConnected with element-wise add

bgawrych commented on a change in pull request #20599:
URL: https://github.com/apache/incubator-mxnet/pull/20599#discussion_r717690034



##########
File path: src/operator/subgraph/mkldnn/mkldnn_fc_sum_fuse.h
##########
@@ -163,13 +186,21 @@ class SgMKLDNNFCSumFuseProperty : public SubgraphProperty {
     if (ew_add_node != nullptr) {
       CHECK_NOTNULL(fc_node->attrs.subgraphs[0]);
       auto fc_orginal = fc_node->attrs.subgraphs[0]->outputs[0].node;
+
       if (fc_orginal->op() == Op::Get("FullyConnected")) {
         nnvm::Symbol new_sym;
-        nnvm::NodeEntry& ew_input_with_fc = (ew_add_node->inputs[1].node == fc_node)
-                                                ? ew_add_node->inputs[1]
-                                                : ew_add_node->inputs[0];
-        ew_input_with_fc.node             = fc_orginal;
-        new_sym.outputs.emplace_back(ew_add_node);
+        nnvm::ObjectPtr n = nnvm::Node::Create();

Review comment:
       Consider adding comment why elemwise_add node must be placed in subgraph (InferShape/InferType)

##########
File path: src/operator/subgraph/mkldnn/mkldnn_fc_sum_fuse.h
##########
@@ -163,13 +186,21 @@ class SgMKLDNNFCSumFuseProperty : public SubgraphProperty {
     if (ew_add_node != nullptr) {
       CHECK_NOTNULL(fc_node->attrs.subgraphs[0]);
       auto fc_orginal = fc_node->attrs.subgraphs[0]->outputs[0].node;
+
       if (fc_orginal->op() == Op::Get("FullyConnected")) {
         nnvm::Symbol new_sym;
-        nnvm::NodeEntry& ew_input_with_fc = (ew_add_node->inputs[1].node == fc_node)
-                                                ? ew_add_node->inputs[1]
-                                                : ew_add_node->inputs[0];
-        ew_input_with_fc.node             = fc_orginal;
-        new_sym.outputs.emplace_back(ew_add_node);
+        nnvm::ObjectPtr n = nnvm::Node::Create();

Review comment:
       Consider adding comment why elemwise_add node must be placed in subgraph (InferShape/InferType). LGTM




-- 
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