You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/11/17 07:06:52 UTC
[GitHub] [tvm] farshidsp opened a new pull request, #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
farshidsp opened a new pull request, #13414:
URL: https://github.com/apache/tvm/pull/13414
This PR adds support for `depthwise_conv2d_NHWC` with an HWIO kernel layout.
@Lunderberg @mehrdadh
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1038523978
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
+
+ kernel_permutation_from = np.argsort(kernel_permutation_to)
Review Comment:
Ah, I see. It looks like that implementation is trying to be more clever, and to identify the permutation by inspecting the string. In this case, since we're only supporting two explicitly enabled shapes, I'd lean for defining a `kernel_permutation` directly for each one.
```python
if kernel_layout == 'HWOI':
kernel_permutaiton = [0,1,2,3]
elif kernel_layout == 'HWIO':
kernel_permutaiton = [0,1,3,2]
else:
raise ValueError(f'Unsupported kernel layout: {kernel_layout}')
```
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] TejashShah commented on pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
TejashShah commented on PR #13414:
URL: https://github.com/apache/tvm/pull/13414#issuecomment-1337969945
@farshidsp Is kernel layout or filter layout a better word to indicate the change in layout?
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13414:
URL: https://github.com/apache/tvm/pull/13414#issuecomment-1318184291
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] farshidsp commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
farshidsp commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1036549040
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -284,9 +295,13 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
j * stride_w + dj * dilation_w,
idxdiv(c, channel_multiplier),
].astype(out_dtype)
- * Filter[
- di, dj, idxdiv(c, channel_multiplier), idxmod(c, channel_multiplier)
- ].astype(out_dtype)
+ * Filter.__getitem__(
Review Comment:
Oh I see, I have changed this as your recommendation.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] farshidsp commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
farshidsp commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1036550184
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
+
+ kernel_permutation_from = np.argsort(kernel_permutation_to)
Review Comment:
I tried to follow [this](https://github.com/apache/tvm/blob/6782a35018d5df63486569f17296737d75eedc55/python/tvm/topi/nn/conv2d.py#L837) as much as possible so in the future we can add more features if needed. If you think this is not the best way I could change it.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1034074531
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -284,9 +295,13 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
j * stride_w + dj * dilation_w,
idxdiv(c, channel_multiplier),
].astype(out_dtype)
- * Filter[
- di, dj, idxdiv(c, channel_multiplier), idxmod(c, channel_multiplier)
- ].astype(out_dtype)
+ * Filter.__getitem__(
Review Comment:
The explicit `__getitem__` isn't required. `Filter[x,y,z]` is syntactic sugar for `Filter[(x,y,z)]`. If you already have a tuple, then `Filter[tuple( ... )]` can be called.
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
+
+ kernel_permutation_from = np.argsort(kernel_permutation_to)
Review Comment:
Is there a benefit to defining in terms of `kernel_permutation_to` instead of directly defining `kernel_permutation_from`?
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
Review Comment:
I'm not seeing how this permutation generates `HWIO`. This defines `kernel_permutation_to` as `[3, 2, 0, 1]`, so `kernel_permutation_from` is `[2, 3, 1, 0]`. With the usage below, that would permute from `[di, dj, c//channel_multiplier, c%channel_multiplier]` to `[c//channel_multiplier, c%channel_multiplier, dj, di]`, which would be `OIWH`.
Should this be `list(range(dim)) + [dim + 1, dim]` instead?
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] farshidsp commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
farshidsp commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1036548582
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
Review Comment:
@Lunderberg You are right, my bad. Not sure why I thought this would result in `HWIO`. Thanks so much for catching this bug.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1038524258
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
+
+ kernel_permutation_from = np.argsort(kernel_permutation_to)
Review Comment:
That said, if there are many locations where the same kernel permutation definitions are used, it may be useful to pull it out into a common utility.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] farshidsp commented on a diff in pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
farshidsp commented on code in PR #13414:
URL: https://github.com/apache/tvm/pull/13414#discussion_r1039887244
##########
python/tvm/topi/nn/depthwise_conv2d.py:
##########
@@ -252,8 +253,18 @@ def depthwise_conv2d_nhwc(Input, Filter, stride, padding, dilation, out_dtype=No
dilation_h, dilation_w = dilation
batch, in_height, in_width, in_channel = Input.shape
+
+ dim = len(Input.shape) - 2
+
# shape of dilated kernel
- filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ if kernel_layout == "HWOI":
+ filter_height, filter_width, filter_channel, channel_multiplier = Filter.shape
+ kernel_permutation_to = [0, 1] + list(range(2, dim + 2))
+ elif kernel_layout == "HWIO":
+ filter_height, filter_width, channel_multiplier, filter_channel = Filter.shape
+ kernel_permutation_to = [dim + 1, dim] + list(range(dim))
+
+ kernel_permutation_from = np.argsort(kernel_permutation_to)
Review Comment:
Changed the way of permutation from your recommendation. @Lunderberg
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] farshidsp commented on pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
farshidsp commented on PR #13414:
URL: https://github.com/apache/tvm/pull/13414#issuecomment-1337997362
> @farshidsp Is kernel layout or filter layout a better word to indicate the change in layout?
In other topi files, we have used `kernel_permutation` to indicate the change in the kernel layout and that's why I used the same term.
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg merged pull request #13414: [Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout
Posted by GitBox <gi...@apache.org>.
Lunderberg merged PR #13414:
URL: https://github.com/apache/tvm/pull/13414
--
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@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org