fix fp32 flatten issue #15351

wuxun-zhang · 2019-06-25T09:44:04Z

Description

This PR should fix issue #15267. The previous FP32 flatten op seems not work properly in some situations. So, we reimplement it by using mkldnn reshape op.
@pengzhao-intel @ciyongch @TaoLv please help review. Thanks

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 · 2019-06-27T04:49:05Z

@TaoLv @ciyongch please help to take a review.

ciyongch · 2019-06-28T01:48:11Z

src/operator/tensor/matrix_op.cc

-    // is larger than 2, we should use the default layout.
-    if (outputs[0].IsMKLDNNData() && inputs[0].shape().ndim() > 2)
-      const_cast<NDArray &>(outputs[0]).Reorder2Default();
+  if (SupportMKLDNNArray(inputs[0].dtype(), inputs[0].shape())) {


SupportMKLDNNArray doesn't support 3D tensor, flatten should have same coverage as reshape, right?

Yes, you're right.

Use the same conditions in SupportMKLDNNReshape.

ciyongch · 2019-06-28T01:52:17Z

src/operator/nn/mkldnn/mkldnn_reshape.cc

@@ -98,62 +75,63 @@ class MKLDNNReshapeForward {
    } else {
      LOG(FATAL) << "not supported req type: " << req;
    }


indent from Line38 to 77?

ciyongch · 2019-06-28T01:53:10Z

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

@@ -119,12 +119,11 @@ void MKLDNNTransposeForward(const nnvm::NodeAttrs& attrs,
                            const OpReqType &req,
                            const NDArray &output);

-void MKLDNNReshapeForward(const nnvm::NodeAttrs &attrs,
+void MKLDNNFlattenForward(const nnvm::NodeAttrs &attrs,


Better to keep both flatten and reshape function declaration here.

ciyongch · 2019-06-28T01:55:24Z

src/operator/nn/mkldnn/mkldnn_flatten.cc

+    : MKLDNNReshapeFwd(req, input, output) {}
+};
+
+static MKLDNNFlattenFwd &GetFlattenForward(const OpReqType &req,


Is it possible to combine GetFlattenForward and GetRehshapeForward into one, and call them via passing different template parameter? So that we can still reuse most of the function when implementing other ops like expand_dims?

Seems cannot combine these two functions into one. Because reshape op have a parameter ReshapeParam while flatten op don't, so when we try to create key, for reshape we use MKLDNNReshapeSignature key(ReshapeParam), but for flatten we use OpSignature key. So, this function should be designed differently.
Also, expand_dims op also have a parameter, and can reuse this function with reshape op.

anirudhacharya · 2019-06-28T18:24:43Z

@mxnet-label-bot add [pr-awaiting-review]

wuxun-zhang · 2019-07-01T01:50:02Z

@ciyongch @TaoLv Please help review again if these changes are appropriate. Thanks.

TaoLv · 2019-07-01T02:04:58Z

@arcadiaphy, it would be highly appreciated if you can help to verify this fix with the java demo case. Hope this PR can fix the issue in #15267.

arcadiaphy · 2019-07-01T11:34:25Z

@TaoLv I've tested the java demo, problem solved. Thanks!

wuxun-zhang · 2019-07-05T01:15:20Z

@pengzhao-intel @TaoLv @ciyongch CI has passed. Please take a review again. Thanks.

pengzhao-intel

Thanks for the improvements.

pengzhao-intel · 2019-07-05T05:26:02Z

https://mxnet.incubator.apache.org/versions/master/tutorials/mkldnn/operator_list.html

Please also add the OP in the MKLDNN supported list.

wuxun-zhang · 2019-07-05T10:57:19Z

@pengzhao-intel @TaoLv Thanks for your advice. Updated.

pengzhao-intel · 2019-07-08T02:07:14Z

Thanks for your contribution. Merging now.

* Fix flatten issue before slice op * fix cpplint * address comments * retrigger CI * trigger CI * retrigger CI * use SupportMKLDNNReshape and update operator list

wuxun-zhang added 2 commits June 21, 2019 15:02

Fix flatten issue before slice op

d3fcabc

fix cpplint

436ffa4

wuxun-zhang force-pushed the fix_fp32_flatten branch from ce19fd5 to 436ffa4 Compare June 25, 2019 11:13

ciyongch reviewed Jun 28, 2019

View reviewed changes

marcoabreu added the pr-awaiting-review PR is waiting for code review label Jun 28, 2019

address comments

2c3472f

wuxun-zhang force-pushed the fix_fp32_flatten branch from 667dd37 to 2c3472f Compare July 1, 2019 01:41

wuxun-zhang added 3 commits July 1, 2019 21:00

retrigger CI

f659a72

trigger CI

233a941

retrigger CI

8ae01d4

pengzhao-intel approved these changes Jul 5, 2019

View reviewed changes

use SupportMKLDNNReshape and update operator list

d0dd24e

wuxun-zhang requested a review from szha as a code owner July 5, 2019 07:46

TaoLv approved these changes Jul 8, 2019

View reviewed changes

pengzhao-intel merged commit 091fece into apache:master Jul 8, 2019

wuxun-zhang deleted the fix_fp32_flatten branch July 12, 2019 01:04

TaoLv pushed a commit that referenced this pull request Aug 12, 2019

fix fp32 flatten issue (#15351) (#15802)

386ad26

* Fix flatten issue before slice op * fix cpplint * address comments * retrigger CI * trigger CI * retrigger CI * use SupportMKLDNNReshape and update operator list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix fp32 flatten issue #15351

fix fp32 flatten issue #15351

wuxun-zhang commented Jun 25, 2019

pengzhao-intel commented Jun 27, 2019

ciyongch Jun 28, 2019

wuxun-zhang Jun 28, 2019

TaoLv Jul 5, 2019

ciyongch Jun 28, 2019

ciyongch Jun 28, 2019

ciyongch Jun 28, 2019

wuxun-zhang Jun 28, 2019

anirudhacharya commented Jun 28, 2019

wuxun-zhang commented Jul 1, 2019

TaoLv commented Jul 1, 2019

arcadiaphy commented Jul 1, 2019

wuxun-zhang commented Jul 5, 2019

pengzhao-intel left a comment

pengzhao-intel commented Jul 5, 2019

wuxun-zhang commented Jul 5, 2019

pengzhao-intel commented Jul 8, 2019

fix fp32 flatten issue #15351

fix fp32 flatten issue #15351

Conversation

wuxun-zhang commented Jun 25, 2019

Description

Checklist

Essentials

Changes

Comments

pengzhao-intel commented Jun 27, 2019

ciyongch Jun 28, 2019

Choose a reason for hiding this comment

wuxun-zhang Jun 28, 2019

Choose a reason for hiding this comment

TaoLv Jul 5, 2019

Choose a reason for hiding this comment

ciyongch Jun 28, 2019

Choose a reason for hiding this comment

ciyongch Jun 28, 2019

Choose a reason for hiding this comment

ciyongch Jun 28, 2019

Choose a reason for hiding this comment

wuxun-zhang Jun 28, 2019

Choose a reason for hiding this comment

anirudhacharya commented Jun 28, 2019

wuxun-zhang commented Jul 1, 2019

TaoLv commented Jul 1, 2019

arcadiaphy commented Jul 1, 2019

wuxun-zhang commented Jul 5, 2019

pengzhao-intel left a comment

Choose a reason for hiding this comment

pengzhao-intel commented Jul 5, 2019

wuxun-zhang commented Jul 5, 2019

pengzhao-intel commented Jul 8, 2019