Add reshape op supported by MKL-DNN #12980

huangzhiyuan · 2018-10-26T08:36:13Z

Description

This PR indents to add reshape op supported by MKL-DNN. It will help reduce the unnecessary reorder operations after a MKL-DNN operation. @pengzhao-intel @ZhennanQin

Test network:

data = mx.symbol.Variable('data')
conv1 = mx.symbol.Convolution(data=data, num_filter=16384, kernel=(1, 1), pad=(0, 0), stride=(1, 1))
res = mx.symbol.reshape(data=conv1, shape=(16384,4,16,16))
trans = mx.symbol.transpose(data=res)
res1 = mx.symbol.reshape(data=trans, shape=(1, 1024, 1, 16384))
conv2 = mx.symbol.Convolution(data=res1, num_filter=16384, kernel=(1, 1), pad=(0,0), stride=(1,1))

Origin verbose log:

mkldnn_verbose,exec,convolution,jit:avx512_common,forward_inference,fsrc:nchw fwei:Ohwi16o fbia:x fdst:nChw16c,alg:convolution_direct,mb4_g1ic3oc16384_ih16oh16kh1sh1dh0ph0_iw16ow16kw1sw1dw0pw0,2.08691
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_nChw16c out:f32_nchw,num:1,4x16384x16x16,2.44995
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_nchw out:f32_nChw16c,num:1,4x16384x16x16,1.64697
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_nChw16c out:f32_nchw,num:1,4x16384x16x16,2.30298
mkldnn_verbose,exec,reorder,simple:any,undef,in:f32_nchw out:f32_nchw,num:1,16384x4x16x16,2.1731
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_nchw out:f32_nChw16c,num:1,1x1024x1x16384,2.40698
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_oihw out:f32_OIhw16i16o,num:1,16384x1024x1x1,2.29297
mkldnn_verbose,exec,convolution,jit_1x1:avx512_common,forward_inference,fsrc:nChw16c fwei:OIhw16i16o fbia:x fdst:nChw16c,alg:convolution_direct,mb1_g1ic1024oc16384_ih1oh1kh1sh1dh0ph0_iw16384ow16384kw1sw1dw0pw0,457.164

After reshape op supported by MKL-DNN verbose log:

mkldnn_verbose,exec,convolution,jit:avx512_common,forward_inference,fsrc:nchw fwei:Ohwi16o fbia:x fdst:nChw16c,alg:convolution_direct,mb4_g1ic3oc16384_ih16oh16kh1sh1dh0ph0_iw16ow16kw1sw1dw0pw0,1.86304
mkldnn_verbose,exec,reorder,simple:any,undef,in:f32_nchw16c out:f32_nchw,num:1,4x16384x16x16,2.35913
mkldnn_verbose,exec,reorder,simple:any,undef,in:f32_nchw out:f32_nchw,num:1,16x16x4x16384,1.55103
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_nchw out:f32_nChw16c,num:1,1x1024x1x16384,2.6958
mkldnn_verbose,exec,reorder,jit:uni,undef,in:f32_oihw out:f32_OIhw16i16o,num:1,16384x1024x1x1,2.49683
mkldnn_verbose,exec,convolution,jit_1x1:avx512_common,forward_inference,fsrc:nChw16c fwei:OIhw16i16o fbia:x fdst:nChw16c,alg:convolution_direct,mb1_g1ic1024oc16384_ih1oh1kh1sh1dh0ph0_iw16384ow16384kw1sw1dw0pw0,432.984

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

ankkhedia · 2018-10-29T20:21:18Z

@huangzhiyuan Thanks for the contribution!
Could you please make sure that the changes in the PR passes CI
@mxnet-label-bot [pr-awaiting-testing , MKLDNN, operator]

ankkhedia · 2018-10-30T20:31:34Z

@huangzhiyuan It seems that the PR is still failing on CI. Request you to take a look.

huangzhiyuan · 2018-11-01T02:56:12Z

@ankkhedia please take a view, ths :)

ankkhedia · 2018-11-01T16:40:37Z

@azai91 @mseth10 Could you please take a look into this PR?

pengzhao-intel · 2018-11-07T05:55:03Z

ping @zheng-da @azai91 @mseth10

kalyc · 2018-11-13T00:44:07Z

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

stu1130 · 2018-11-21T20:33:55Z

ping @azai91 @mseth10 to review

vandanavk · 2018-11-28T21:47:09Z

@mxnet-label-bot add [MKLDNN]

ZhennanQin · 2018-12-05T04:57:51Z

src/operator/tensor/matrix_op.cc

+ return;
+ }
+ FallBackCompute(UnaryOp::IdentityCompute<cpu>, attrs, ctx, inputs, req,
+ outputs);


ZhennanQin · 2018-12-05T04:58:23Z

src/operator/tensor/matrix_op.cc

@@ -261,7 +317,9 @@ Example::
 .set_num_outputs(1)
 .set_attr<nnvm::FInferShape>("FInferShape", FlattenShape)
 .set_attr<nnvm::FInferType>("FInferType", ElemwiseType<1, 1>)
+#if MXNET_USE_MKLDNN == 1


If MKLDNN supports this op, please add .set_attr<bool>("TIsMKLDNN", true) for it.

This attribute is in Line 327.

ZhennanQin

Only some slight comments. Overall LGTM.

huangzhiyuan · 2018-12-05T08:18:36Z

@azai91 @mseth10 could you help review the PR? This has been out for review for a long long time...

TaoLv · 2018-12-05T08:43:12Z

src/operator/tensor/matrix_op.cc

@@ -210,24 +272,18 @@ static void FlattenEx(const nnvm::NodeAttrs& attrs,
 #endif
 }

+#if MXNET_USE_MKLDNN == 1


Seems this will change the original behavior when MKL-DNN is not enabled. FlattenStorageType is not defined then.

This is followed new style with other mkldnn op, that is, defining InferStorageType function within MKLDNN macro. Most other ops are refactored into this style by Luobao, I guess this one is missing.

TaoLv · 2018-12-05T08:47:50Z

src/operator/tensor/matrix_op.cc

@@ -914,7 +972,7 @@ NNVM_REGISTER_OP(depth_to_space)
 .describe(R"code(Rearranges(permutes) data from depth into blocks of spatial data.
 Similar to ONNX DepthToSpace operator:
 https://github.com/onnx/onnx/blob/master/docs/Operators.md#DepthToSpace.
-The output is a new tensor where the values from depth dimension are moved in spatial blocks 


Thank you for fixing these white spaces at line-end. Does cpplint complain about them? If not, I would like to keep them as is since these changes are not related to this PR.

Have fixed the white space same as before, could you help review again? thanks :)

TaoLv · 2018-12-07T09:06:23Z

Is there any unit test to cover this change?

huangzhiyuan · 2018-12-10T08:45:23Z

@TaoLv @azai91 @mseth10 I've made the change and unit test based on your suggestions. What do you think?

anirudh2290 · 2018-12-11T00:38:45Z

src/operator/tensor/matrix_op.cc

 .set_attr<nnvm::FInferType>("FInferType", ElemwiseType<1, 1>)
 .set_attr<nnvm::FGradient>("FGradient", ElemwiseGradUseNone{"_backward_copy"})
 .set_attr<FCompute>("FCompute<cpu>", UnaryOp::IdentityCompute<cpu>)
+#if MXNET_USE_MKLDNN == 1


I know that this is not the first op which is implemented this way, but is there a reason for the two different if blocks here ?

Nothing different, it has been merged into one block here. Thanks for your review! :)

mseth10

LGTM

This reverts commit c4a619c. Conflicts: src/operator/tensor/matrix_op.cc tests/python/mkl/test_mkldnn.py

Add reshape op supported by MKL-DNN

a5ed940

huangzhiyuan requested a review from anirudh2290 as a code owner October 26, 2018 08:36

marcoabreu added MKLDNN Operator pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 29, 2018

huangzhiyuan added 2 commits October 30, 2018 16:55

fix build issue

e34944d

fix lint

80c2473

huangzhiyuan added 3 commits October 31, 2018 09:43

fix lint

6bd9ec4

fix lint

6798a86

fix lint

0e008f2

fix lint

58a12bd

marcoabreu added pr-awaiting-review PR is waiting for code review and removed MKLDNN Operator pr-awaiting-testing PR is reviewed and waiting CI build and test labels Nov 13, 2018

marcoabreu added the MKLDNN label Nov 28, 2018

Merge remote-tracking branch 'upstream/master' into mkldnn_reshape

2c2a7f3

ZhennanQin reviewed Dec 5, 2018

View reviewed changes

fix lint

b5033a0

ZhennanQin approved these changes Dec 5, 2018

View reviewed changes

TaoLv reviewed Dec 5, 2018

View reviewed changes

fix white space

d26fd04

add unit test

91b9bf9

anirudh2290 reviewed Dec 11, 2018

View reviewed changes

merge if blocks

890682d

pengzhao-intel mentioned this pull request Dec 12, 2018

fallback to dense version for grad(reshape), grad(expand_dims) #13599

Merged

5 tasks

TaoLv approved these changes Dec 13, 2018

View reviewed changes

Merge branch 'master' into mkldnn_reshape

26075fe

mseth10 approved these changes Dec 13, 2018

View reviewed changes

anirudh2290 approved these changes Dec 13, 2018

View reviewed changes

anirudh2290 merged commit c4a619c into apache:master Dec 13, 2018

huangzhiyuan deleted the mkldnn_reshape branch December 14, 2018 01:04

juliusshufan added a commit to juliusshufan/incubator-mxnet that referenced this pull request Apr 21, 2019

Revert "Add reshape op supported by MKL-DNN (apache#12980)"

b49d732

This reverts commit c4a619c. Conflicts: src/operator/tensor/matrix_op.cc tests/python/mkl/test_mkldnn.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reshape op supported by MKL-DNN #12980

Add reshape op supported by MKL-DNN #12980

huangzhiyuan commented Oct 26, 2018 •

edited

Loading

ankkhedia commented Oct 29, 2018

ankkhedia commented Oct 30, 2018

huangzhiyuan commented Nov 1, 2018

ankkhedia commented Nov 1, 2018

pengzhao-intel commented Nov 7, 2018

kalyc commented Nov 13, 2018

stu1130 commented Nov 21, 2018

vandanavk commented Nov 28, 2018

ZhennanQin Dec 5, 2018

ZhennanQin Dec 5, 2018

huangzhiyuan Dec 5, 2018

ZhennanQin left a comment

huangzhiyuan commented Dec 5, 2018

TaoLv Dec 5, 2018

ZhennanQin Dec 6, 2018

TaoLv Dec 5, 2018

huangzhiyuan Dec 6, 2018

TaoLv commented Dec 7, 2018

huangzhiyuan commented Dec 10, 2018

anirudh2290 Dec 11, 2018

huangzhiyuan Dec 11, 2018 •

edited

Loading

mseth10 left a comment

Add reshape op supported by MKL-DNN #12980

Add reshape op supported by MKL-DNN #12980

Conversation

huangzhiyuan commented Oct 26, 2018 • edited Loading

Description

Comments

ankkhedia commented Oct 29, 2018

ankkhedia commented Oct 30, 2018

huangzhiyuan commented Nov 1, 2018

ankkhedia commented Nov 1, 2018

pengzhao-intel commented Nov 7, 2018

kalyc commented Nov 13, 2018

stu1130 commented Nov 21, 2018

vandanavk commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhennanQin left a comment

Choose a reason for hiding this comment

huangzhiyuan commented Dec 5, 2018

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

TaoLv commented Dec 7, 2018

huangzhiyuan commented Dec 10, 2018

Choose a reason for hiding this comment

huangzhiyuan Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

mseth10 left a comment

Choose a reason for hiding this comment

huangzhiyuan commented Oct 26, 2018 •

edited

Loading

huangzhiyuan Dec 11, 2018 •

edited

Loading