manupa-arm commented on a change in pull request #9682:
URL: https://github.com/apache/tvm/pull/9682#discussion_r766579957
##########
File path: python/tvm/relay/op/contrib/cmsisnn.py
##########
@@ -135,8 +135,8 @@ def check_qnn_conv2d(pattern):
return (
conv2d.attrs.out_dtype == "int32"
- and conv2d.attrs.padding[2] == 0
- and conv2d.attrs.padding[3] == 0
+ and int(conv2d.attrs.padding[2]) == 0
Review comment:
I believe this is the actual fix. Am I right ? (because others are
changes to test code).
What test would fail in this PR without this change ?
I think we could have a simpler test to check whether graph partitioning
works with conv2d with padding values for top, left, bottom and right.
##########
File path: tests/python/contrib/test_cmsisnn/utils.py
##########
@@ -86,18 +86,20 @@ def make_module(func):
return mod
-def get_same_padding(data, kernel, dilation, stride, cmsisnn_padding=True):
+def get_padding(data, kernel, dilation, stride, padding):
Review comment:
Is this change necessary to fix the bug here ?
##########
File path: tests/python/contrib/test_cmsisnn/test_generate_constants.py
##########
@@ -94,16 +94,14 @@ def make_model(
kernel_h = kernel_shape[h_index]
kernel_w = kernel_shape[w_index]
a = relay.var("input", shape=shape, dtype=dtype)
- p = (0, 0, 0, 0)
- if padding == "SAME":
Review comment:
This seems orthogonal to the fix here
##########
File path: tests/python/contrib/test_cmsisnn/test_pooling.py
##########
@@ -45,10 +45,9 @@
def make_model(pool_op, shape, pool_size, strides, padding, dtype, scale,
zero_point, relu_type):
"""Return a model and any parameters it may have"""
op = relay.var("input", shape=shape, dtype=dtype)
- pad_ = (0, 0, 0, 0)
- if padding == "SAME":
- dilation = (1, 1)
- pad_ = get_same_padding((shape[1], shape[2]), pool_size, dilation,
strides)
+ dilation = (1, 1)
+ pad_, result = get_padding((shape[1], shape[2]), pool_size, dilation,
strides, padding)
+ if result:
Review comment:
I have added a comment below, it is a bit difficult to follow what
"result" means here. I think a named tuple might help.
However, looking at this feel, it is better to keep the get_padding(..) just
to return the pad top, bottom, left, right values while here we could use
padding check create the nn.pad rather than passing it inside the get_padding
just and returning that check as the 'result'
##########
File path: tests/python/contrib/test_cmsisnn/utils.py
##########
@@ -86,18 +86,20 @@ def make_module(func):
return mod
-def get_same_padding(data, kernel, dilation, stride, cmsisnn_padding=True):
+def get_padding(data, kernel, dilation, stride, padding):
"""Provides CMSIS-NN padding when output dim == input dim"""
+ if padding == "VALID":
+ return (0, 0, 0, 0), False
dilated_kernel_h = dilation[0] * (kernel[0] - 1) + 1
dilated_kernel_w = dilation[1] * (kernel[1] - 1) + 1
out = int(math.ceil(float(data[0]) / float(stride[0])))
pad = max(0, (out - 1) * stride[0] + dilated_kernel_h - data[0])
- pad_top, pad_bottom = (pad, 0) if cmsisnn_padding else (0, pad)
+ pad_top, pad_bottom = (pad, 0) if padding == "SAME" else (0, pad)
Review comment:
What pad mode does the else correspond to here ?
##########
File path: tests/python/contrib/test_cmsisnn/utils.py
##########
@@ -86,18 +86,20 @@ def make_module(func):
return mod
-def get_same_padding(data, kernel, dilation, stride, cmsisnn_padding=True):
+def get_padding(data, kernel, dilation, stride, padding):
"""Provides CMSIS-NN padding when output dim == input dim"""
+ if padding == "VALID":
+ return (0, 0, 0, 0), False
dilated_kernel_h = dilation[0] * (kernel[0] - 1) + 1
dilated_kernel_w = dilation[1] * (kernel[1] - 1) + 1
out = int(math.ceil(float(data[0]) / float(stride[0])))
pad = max(0, (out - 1) * stride[0] + dilated_kernel_h - data[0])
- pad_top, pad_bottom = (pad, 0) if cmsisnn_padding else (0, pad)
+ pad_top, pad_bottom = (pad, 0) if padding == "SAME" else (0, pad)
out = int(math.ceil(float(data[1]) / float(stride[1])))
pad = max(0, (out - 1) * stride[1] + dilated_kernel_w - data[1])
- pad_left, pad_right = (pad, 0) if cmsisnn_padding else (0, pad)
- return [pad_top, pad_left, pad_bottom, pad_right]
+ pad_left, pad_right = (pad, 0) if padding == "SAME" else (0, pad)
+ return [pad_top, pad_left, pad_bottom, pad_right], True
Review comment:
Please add documentation as to what are returned with this function.
It used to return the pad top,left,bottom and right values which are bit
simpler, However augmenting that with a boolean is confusing.
I would recommend using a named tuple as the return here to indicate what
the boolean correspond to
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
