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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
