-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Seems like the build failed, but not because of your changes @anirudh2290. Can you force the build once again, please? |
@Ishitori I am digging more into it, since I am suspecting it is related to this PR. |
@mxnet-label-bot add [Operator, C++, pr-awaiting-review] |
cc @Vikas89 @mseth10 @apeforest for review |
@@ -316,11 +316,9 @@ NNVM_REGISTER_OP(_backward_FullyConnected) | |||
const FullyConnectedParam& params = nnvm::get<FullyConnectedParam>(attrs.parsed); | |||
return params.no_bias ? 2 : 3; | |||
}) | |||
#if MXNET_USE_MKLDNN == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we need additional temp space now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ciyongch could you help take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ciyongch @pengzhao-intel on a side note, https://github.com/apache/incubator-mxnet/blob/master/src/operator/nn/fully_connected.cc#L222 . The backward still doesnt use MKLDNN. Do you know why and is there a plan to fix this ? |
@anirudh2290 Currently, we're focusing on inference pass, and facing the big code refactor due to API change with the coming MKL-DNN v1.0. We'll take a look and see whether to fix it in the new MKL-DNN v1.0 or in the current version. |
This reverts commit 6cf964a.
* Improve FC perf when no_bias=False * Add Issue number in comment * Correct req
) This reverts commit 6cf964a.
Description
Improve FullyConnected op performance when no_bias is set to False. Fixes: #14938
Perf Results:
@NRauschmayr @Ishitori
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments