[Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout#13414
[Hexagon] Enable depthwise conv2d NHWC with an HWIO kernel layout#13414Lunderberg merged 11 commits intoapache:mainfrom
Conversation
|
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 by @-ing them in a comment.
Generated by tvm-bot |
Lunderberg
left a comment
There was a problem hiding this comment.
I like the feature, but I'm not quite following how the permutation specified results in a HWIO kernel layout.
| * Filter[ | ||
| di, dj, idxdiv(c, channel_multiplier), idxmod(c, channel_multiplier) | ||
| ].astype(out_dtype) | ||
| * Filter.__getitem__( |
There was a problem hiding this 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.
There was a problem hiding this comment.
Oh I see, I have changed this as your recommendation.
| 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)) |
There was a problem hiding this 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?
There was a problem hiding this comment.
@Lunderberg You are right, my bad. Not sure why I thought this would result in HWIO. Thanks so much for catching this bug.
| 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) |
There was a problem hiding this comment.
Is there a benefit to defining in terms of kernel_permutation_to instead of directly defining kernel_permutation_from?
There was a problem hiding this comment.
I tried to follow this 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.
There was a problem hiding this 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.
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}')There was a problem hiding this 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.
There was a problem hiding this comment.
Changed the way of permutation from your recommendation. @Lunderberg
|
@farshidsp Is kernel layout or filter layout a better word to indicate the change in layout? |
In other topi files, we have used |
Lunderberg
left a comment
There was a problem hiding this comment.
Thank you for making the changes, and LGTM!
…ache#13414) Enable depthwise conv2d NHWC with HWIO kernel layout. The default kernel layout is HWOI, matched to previous behavior.
…ache#13414) Enable depthwise conv2d NHWC with HWIO kernel layout. The default kernel layout is HWOI, matched to previous behavior.
This PR adds support for
depthwise_conv2d_NHWCwith an HWIO kernel layout.@Lunderberg @mehrdadh