[NNVM][CONVOLUTION] Group convolution generalization for NHWC by srkreddy1238 · Pull Request #1232 · apache/tvm

srkreddy1238 · 2018-06-05T15:16:56Z

No description provided.

srkreddy1238 · 2018-06-06T05:42:27Z

@Huyuwei
Pls review.
This change due to different way of expecting the weights shape for NHWC and NCHW.
I have checked the compute definition of both depthwise and arrived at this.

srkreddy1238 · 2018-06-06T05:43:05Z

If we are good with this, I will add a test case for NHWC-depthwise.

Huyuwei · 2018-06-09T04:18:03Z

+ if (param.layout == "NHWC") {
+ wshape[kernel_layout.indexof('I')] *= param.groups;
+ } else {
+ wshape[0] *= param.groups;


when param.layout == "NCHW", kernel_layout.indexof('I') = 0?

yes, because the weights format for NCHW sent as CHNW from front end and expectation is also same at compute.
Ref.
Frontend:
https://github.com/dmlc/tvm/blob/b877687fbeb8a6c12b62aac9869f7e54091395f4/nnvm/python/nnvm/frontend/keras.py#L145
Compute:
https://github.com/dmlc/tvm/blob/b877687fbeb8a6c12b62aac9869f7e54091395f4/topi/python/topi/nn/depthwise_conv2d.py#L38

I think wshape[kernel_layout.indexof('O')] *= param.groups can handle both cases
You can try some tests

No it doesn't work this way as kernel layout for NCHW deviated from the format. (it's actually CNHW passed from frontend and same is expected in compute.).

Huyuwei · 2018-06-12T04:13:29Z

+ if (param.layout == "NHWC") {
+ wshape[kernel_layout.indexof('I')] *= param.groups;
+ } else {
+ wshape[0] *= param.groups;


I think wshape[kernel_layout.indexof('O')] *= param.groups can handle both cases
You can try some tests

Huyuwei · 2018-06-12T04:19:25Z

 wshape = ConvertLayout(wshape, kOIHW, kernel_layout);
- wshape[0] *= param.groups;
+
+ // Depthwise


it's group convolution, and depthwise is just a special case
In fact, only depthwise convolution with multiplier=1 is supported now since it can be expressed as group convolution, see https://github.com/dmlc/tvm/blob/master/nnvm/python/nnvm/top/nn.py#L100-L101

This generalisation works only if we pass kernel_layout as CNHW (IOHW).
IOHW is agreement between frontend and nchw_deptiwise_compute which infer shape doesn't know.
In my case also multiplier is 1.

Alternatively we could change frontend to pass kernel_layout as IOHW and generalise it to indexof('I') .

Huyuwei · 2018-06-12T21:36:42Z

@srkreddy1238 Could you first add a test case that drives this change? It will help me better follow what's going on. You may need to modify these two files to test NHWC layout:
https://github.com/dmlc/tvm/blob/master/nnvm/python/nnvm/top/nn.py#L78
https://github.com/dmlc/tvm/blob/master/nnvm/tests/python/compiler/test_top_level2.py#L61

srkreddy1238 · 2018-06-13T00:56:08Z

@Huyuwei

We don't have a frontend with NHWC and depthwise now to give a clear test case. I could do this once we have tensorflow frontend.

Ref: https://github.com/srkreddy1238/tvm/tree/mobilenet
Head of this branch has the changes with tensorflow and mobilenet to demonstrate this change.

srkreddy1238 · 2018-06-14T16:46:21Z

@Huyuwei

Test case added for NHWC, pls check.

Huyuwei · 2018-06-15T20:04:14Z

My suggestion is we use wshape[kernel_layout.indexof('O')] *= param.groups to handle all layouts.

NHWC depthwise convolution can be expressed as

depthconv_nhwc=sym.conv2d(data, layout="NHWC", kernel_layout="HWOI")

while a normal grouped NHWC convolution can be expressed as

groupconv_nhwc=sym.conv2d(data, layout="NHWC", kernel_layout="HWIO")

The order of in_channel and out_channel in the kernel layout of topi.conv2d_nchw and topi.conv2d_nhwc is different, but the order of in_channel and multiplier in the kernel layout of topi.depthwise_conv2d_nchw and topi.depthwise_conv2d_nhwc is the same. It's better to deal with this asymmetry in frontend part, not c++ code.

@srkreddy1238 what do you think?

Huyuwei · 2018-06-15T20:08:10Z

@srkreddy1238 May I ask one question? just for curiosity
Since convolution.cc is basically copied from mxnet source code, is there also a bug in grouped NHWC convolution is mxnet?
https://github.com/apache/incubator-mxnet/blob/master/src/operator/nn/convolution.cc#L152

srkreddy1238 · 2018-06-15T23:50:21Z

", but the order of in_channel and multiplier in the kernel layout of topi.depthwise_conv2d_nchw and topi.depthwise_conv2d_nhwc is the same"

No, they are not same.

Let me check mxnet compute implementation..

srkreddy1238 · 2018-06-16T05:03:15Z

@Huyuwei

Ref. https://github.com/apache/incubator-mxnet/blob/9b8eb567273d9521c639ea783e1117fe09cb3d64/src/operator/nn/convolution-inl.h#L169

mxnet doesn't implement NHWC but we do.

Huyuwei · 2018-06-16T05:54:17Z

@srkreddy1238 aha, thanks for the pointer.

In the kernel layout of both topi.depthwise_conv2d_nchw and topi.depthwise_conv2d_nhwc is, in_channel is before multiplier, is this right?
https://github.com/dmlc/tvm/blob/master/topi/python/topi/nn/depthwise_conv2d.py#L21-L22
https://github.com/dmlc/tvm/blob/master/topi/python/topi/nn/depthwise_conv2d.py#L76-L77

srkreddy1238 · 2018-06-16T06:01:56Z

Yes, channel is before multiplier.

Huyuwei · 2018-06-17T23:30:49Z

@srkreddy1238 For topi.conv2d_nchw, out_channel is before in_channel in kernel layout, while for topi.conv2d_nhwc, out_channel is after in_channel. And there is a mismatch between conv2d_nhwc and depthwise_conv2d_nhwc.

wshape[kernel_layout.indexof('O')] *= param.groups works for general group convolutions. Since group conv is not yet implemented in topi, now we dispatch depthwise conv to support a special case of group conv. To solve the kernel layout mismatch between conv2d_nhwc and depthwise_conv2d_nhwc, we need to specify kernel_layout="HWOI" for depthwise conv, which for general group conv is kernel_layout="HWIO" , as previously commented.

srkreddy1238 · 2018-06-18T03:40:25Z

@Huyuwei

Thanks for the info. This makes clear to me.
I will work on modifications based on this.

FusedBatchNorm, Relu6, DepthwiseConv2dNative, Shape

srkreddy1238 · 2018-06-18T15:58:35Z

@Huyuwei
Now works for both NCHW & NHWC. Pls review.

Huyuwei · 2018-06-19T03:06:41Z

- return topi.generic.schedule_depthwise_conv2d_nchw(outs)
+ elif groups == channels and layout == "NCHW":
+ return topi.generic.schedule_depthwise_conv2d_nchw(outs)
+ elif groups == channels and layout == "NHWC":


elif groups == channels and layout == "NHWC" and kernel_layout == "HWOI":

Huyuwei · 2018-06-19T03:09:46Z

@@ -98,9 +98,14 @@ def compute_conv2d(attrs, inputs, _):
 if groups == 1:
 out = topi.nn.conv2d(inputs[0], kernel, strides, padding, layout)
 elif groups == get_const_int(inputs[0].shape[1]) and groups == channels:


add more checks here
eliflayout=="NCHW"andgroups==get_const_int(inputs[0].shape[1]) andgroups==channels: out=eliflayout=="NHWC"andkernel_layout=="HWOI"andgroups==get_const_int(inputs[0].shape[3]) andgroups==channels: out=

Huyuwei · 2018-06-19T03:25:56Z

+ elif layout == "NCHW" and \
+ groups == get_const_int(inputs[0].shape[1]) and \
+ groups == channels:
 # NCHW


then this comment can be removed

Huyuwei · 2018-06-19T03:26:12Z

+ kernel_layout == "HWOI" and \
+ groups == get_const_int(inputs[0].shape[3]) and \
+ groups == channels:
 # NHWC


and this line

Huyuwei · 2018-06-19T13:48:07Z

@srkreddy1238 Can mobilenet be converted now? If so, add it to frontend tests.

srkreddy1238 · 2018-06-19T13:59:01Z

Ok. I will update frontend testcases with mobilenet this weekend.

tqchen · 2018-06-24T16:09:25Z

This is merged as @Huyuwei approved the change. Please add mobilnet test in a separate PR

…#1232)

srkreddy1238 force-pushed the tf branch 3 times, most recently from b0aed78 to 11738bfCompare June 6, 2018 05:29

Huyuwei reviewed Jun 9, 2018
View reviewed changes

Huyuwei reviewed Jun 12, 2018
View reviewed changes

srkreddy1238 force-pushed the tf branch from 11738bf to 8dcbf3cCompare June 14, 2018 16:43

srkreddy1238 mentioned this pull request Jun 15, 2018
[NNVM][FRONTEND] Tensorflow frontend support #1188
Merged

srkreddy1238 force-pushed the tf branch from 8dcbf3c to 79286d1Compare June 15, 2018 14:48

srkreddy1238 added 3 commits June 18, 2018 21:17

[NNVM][CONVOLUTION] Use generic index for group multiply - bugfix
c10865e

[NNVM][CONVOLUTION] deptiwise convolution support for tensorflow.
7f56f4a

[NNVM][FRONTEND] More operators for tensorflow (mobilenet).
e7eeb51
FusedBatchNorm, Relu6, DepthwiseConv2dNative, Shape

srkreddy1238 force-pushed the tf branch from 79286d1 to e7eeb51Compare June 18, 2018 15:47

srkreddy1238 changed the title ~~[NNVM][CONVOLUTION] Generic index for group multiply - bug fix~~[NNVM][CONVOLUTION] Group comvolution generalization for NHWCJun 18, 2018

srkreddy1238 changed the title ~~[NNVM][CONVOLUTION] Group comvolution generalization for NHWC~~[NNVM][CONVOLUTION] Group convolution generalization for NHWCJun 18, 2018

Huyuwei requested changes Jun 19, 2018
View reviewed changes

Huyuwei reviewed Jun 19, 2018
View reviewed changes

[NNVM][CONVOLUTION] Review comments.
34fe3d9

srkreddy1238 force-pushed the tf branch from 8dc92a7 to 34fe3d9Compare June 19, 2018 03:58

Huyuwei approved these changes Jun 19, 2018
View reviewed changes

srkreddy1238 mentioned this pull request Jun 19, 2018
[FRONTEND][ONNX] 'Shape' operator not implemented #1297
Closed

tqchen merged commit 1e66d3c into apache:masterJun 24, 2018

tqchen pushed a commit to tqchen/tvm that referenced this pull request Jul 6, 2018
[NNVM][CONVOLUTION] Group convolution generalization for NHWC (apache…
65ad6f9
…#1232)

mnuyens pushed a commit to mnuyens/tvm that referenced this pull request Jul 10, 2018
[NNVM][CONVOLUTION] Group convolution generalization for NHWC (apache…
bb7df69
…#1232)

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018
[NNVM][CONVOLUTION] Group convolution generalization for NHWC (apache…
7cc74ba
…#1232)

Uh oh!

Conversation

srkreddy1238 commented Jun 5, 2018

Uh oh!

srkreddy1238 commented Jun 6, 2018

Uh oh!

srkreddy1238 commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srkreddy1238Jun 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srkreddy1238Jun 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huyuwei commented Jun 12, 2018

Uh oh!

srkreddy1238 commented Jun 13, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srkreddy1238 commented Jun 14, 2018

Uh oh!

Huyuwei commented Jun 15, 2018

Uh oh!

Huyuwei commented Jun 15, 2018

Uh oh!

srkreddy1238 commented Jun 15, 2018

Uh oh!

srkreddy1238 commented Jun 16, 2018

Uh oh!

Huyuwei commented Jun 16, 2018

Uh oh!

srkreddy1238 commented Jun 16, 2018

Uh oh!

Huyuwei commented Jun 17, 2018

Uh oh!

srkreddy1238 commented Jun 18, 2018

Uh oh!

srkreddy1238 commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huyuwei commented Jun 19, 2018

Uh oh!

srkreddy1238 commented Jun 19, 2018

Uh oh!

tqchen commented Jun 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

srkreddy1238Jun 10, 2018•
edited
Loading

srkreddy1238Jun 12, 2018•
edited
Loading

srkreddy1238 commented Jun 13, 2018•
edited
Loading