[GitHub] [tvm] cconvey commented on a change in pull request #9473: cleanup Hexagon conv2d tests

[GitBox <gi...@apache.org>]

cconvey commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r745771983



##########
File path: tests/python/contrib/test_hexagon/infrastructure.py
##########
@@ -18,36 +18,43 @@
 """ Hexagon testing infrastructure """
 
 import tvm
+from tvm import te
 import numpy
 
 
 def ceildiv(o, d):
     return tvm.tir.floordiv(o + d - 1, d)
 
 
-def get_packed_activation_layout(shape_nhwc, block_shape, packed_C=True):
+def get_block_shape():
+    return 8, 8, 32
+
+
+def get_filter_block_shape():
+    return 8, 32, 4
+
+
+def get_packed_shape(shape_nhwc):
     assert len(shape_nhwc) == 4
     shape = [shape_nhwc[0]]
+    block_shape = get_block_shape()
     off_h, off_w, off_c = block_shape
     shape.append(ceildiv(shape_nhwc[1], off_h))
     shape.append(ceildiv(shape_nhwc[2], off_w))
-    if packed_C:
-        shape.append(ceildiv(shape_nhwc[3], off_c))
-        shape.extend(block_shape)
-    else:
-        shape.extend([off_h, off_w, shape_nhwc[3]])
+    shape.append(ceildiv(shape_nhwc[3], off_c))

Review comment:
       In this code we handle `ceildiv` as-is, but in the `get_packed_filter_shape` function (below) we cast its result to a Python `int` type.
   
   Is there a good reason for doing it two different ways?  
   
   Specifically: casting to `int` works when the runtime return type of `ceildiv` is `tvm.tir.expr.IntImm`.  But I'm wondering if we're okay with this test making that assumption. I.e., are we testing everything we _intend_ to test when we assume that the exact shapes are known at this point in time?

##########
File path: tests/python/contrib/test_hexagon/infrastructure.py
##########
@@ -18,36 +18,43 @@
 """ Hexagon testing infrastructure """
 
 import tvm
+from tvm import te
 import numpy
 
 
 def ceildiv(o, d):
     return tvm.tir.floordiv(o + d - 1, d)
 
 
-def get_packed_activation_layout(shape_nhwc, block_shape, packed_C=True):
+def get_block_shape():
+    return 8, 8, 32
+
+
+def get_filter_block_shape():
+    return 8, 32, 4
+
+
+def get_packed_shape(shape_nhwc):

Review comment:
       Should we rename the parameter to `logical_shape_nhwc`?  It might slightly clarify the function's behavior, since we have several competing concepts of "shape" in this code.

##########
File path: tests/python/contrib/test_hexagon/infrastructure.py
##########
@@ -18,36 +18,43 @@
 """ Hexagon testing infrastructure """
 
 import tvm
+from tvm import te
 import numpy
 
 
 def ceildiv(o, d):
     return tvm.tir.floordiv(o + d - 1, d)
 
 
-def get_packed_activation_layout(shape_nhwc, block_shape, packed_C=True):
+def get_block_shape():

Review comment:
       Maybe outside the scope of this PR, but should we add a comment saying where these magic numbers come from?  Unexplained magic numbers sometimes obscure the programmer's intent.

##########
File path: tests/python/contrib/test_hexagon/infrastructure.py
##########
@@ -18,36 +18,43 @@
 """ Hexagon testing infrastructure """
 
 import tvm
+from tvm import te
 import numpy
 
 
 def ceildiv(o, d):
     return tvm.tir.floordiv(o + d - 1, d)
 
 
-def get_packed_activation_layout(shape_nhwc, block_shape, packed_C=True):
+def get_block_shape():
+    return 8, 8, 32
+
+
+def get_filter_block_shape():
+    return 8, 32, 4
+
+
+def get_packed_shape(shape_nhwc):

Review comment:
       IIUC, this function implements a policy decision regarding where padding (if any) is added.  Would it make sense to add a doc string stating what the function does in this regard?




-- 
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