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 2021/11/08 22:10:18 UTC
[GitHub] [tvm] adstraw opened a new pull request #9473: cleanup Hexagon conv2d tests
adstraw opened a new pull request #9473:
URL: https://github.com/apache/tvm/pull/9473
Deletes unnecessary cases from the test_conv2d_blocked.py including cases covering logical `oihw` filter and packed `nhw8h8wc` packed input layouts. Moved quite a lot of shared code to the infrastructure file reducing maintenance costs. And, some other general cleanup items --- comments, variable names, etc.
--
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] tmoreau89 commented on pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#issuecomment-973221988
Thank you @adstraw @cconvey @csullivan ; the PR has been merged!
--
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] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r746092515
##########
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)
Review comment:
I added some asserts to catch cases where `o` or `d` are less than zero.
--
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] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r746088029
##########
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:
Indeed, casting to `int` is a means of converting from `tvm.tir.expr.IntImm` to `int`. I am not sure why it works differently for `get_packed_shape` (no cast) vs. `get_packed_filter_shape` (requires cast). Also not sure about your question RE knowing exact shapes. More investigation required.
NOTE: I did change `get_packed_filter_shape` to take a shape instead of a list of dimensions to match the API of `get_packed_shape`.
--
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] csullivan commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r745145830
##########
File path: tests/python/contrib/test_hexagon/conftest.py
##########
@@ -1,37 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements. See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership. The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied. See the License for the
-# specific language governing permissions and limitations
-# under the License.
-
-""" Hexagon testing fixtures used to deduce testing argument
- values from testing parameters """
-
-import tvm
-from .infrastructure import get_packed_filter_layout
-
-
-@tvm.testing.fixture
-def shape_nhwc(batch, in_channel, in_size):
- return (batch, in_size, in_size, in_channel)
-
-
-@tvm.testing.fixture
-def shape_oihw(out_channel, in_channel, kernel):
- return (out_channel, in_channel, kernel, kernel)
-
-
-@tvm.testing.fixture
-def shape_oihw8i32o4i(out_channel, in_channel, kernel):
- return get_packed_filter_layout(out_channel, in_channel, kernel, kernel)
Review comment:
The goal being to check to see if the testing fixtures are the source of the `i386` flakiness?
--
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] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r746093509
##########
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
Review comment:
Added a basic comment: "defines inner filter block shape: 8i32o41"
--
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] cconvey commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by 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
[GitHub] [tvm] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r746059867
##########
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:
Added a basic comment: "defines inner block shape: 8h8w32c"
--
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] cconvey commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
cconvey commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r745782263
##########
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)
Review comment:
Should we add a comment that this function might only work as intended for _positive_ numbers?
tl;dr:
It's not obvious if this has intuitive behavior when `o` and/or `d` are negative. AFAIK `o` and `d` will only be positive when we're calling this, but someone might decide to promote this function to a broader role in TVM.
##########
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
Review comment:
It's not obvious why these particular numbers are chosen. Should there be comments explaining the reasons for using them?
##########
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:
Since we have several competing versions of "shape" in this code, would it be helpful (for clarity) to rename `shape_nhwc` to `logical_shape_nhwc`?
--
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] tmoreau89 merged pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
tmoreau89 merged pull request #9473:
URL: https://github.com/apache/tvm/pull/9473
--
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] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r746099503
##########
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:
Renamed to `logical_shape_nhwc`. Also renamed other shape parameters in the infrastructure file for clarity.
In regards to padding the role of this function is to convert the post-padded logical shape into a physical packed shape. I think the intent of this function is clear, now, given the variable name change. Also added comments to this effect.
##########
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:
Done.
--
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] adstraw commented on pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#issuecomment-963621311
@csullivan and @cconvey please review
--
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] adstraw commented on a change in pull request #9473: cleanup Hexagon conv2d tests
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9473:
URL: https://github.com/apache/tvm/pull/9473#discussion_r745152878
##########
File path: tests/python/contrib/test_hexagon/conftest.py
##########
@@ -1,37 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements. See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership. The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied. See the License for the
-# specific language governing permissions and limitations
-# under the License.
-
-""" Hexagon testing fixtures used to deduce testing argument
- values from testing parameters """
-
-import tvm
-from .infrastructure import get_packed_filter_layout
-
-
-@tvm.testing.fixture
-def shape_nhwc(batch, in_channel, in_size):
- return (batch, in_size, in_size, in_channel)
-
-
-@tvm.testing.fixture
-def shape_oihw(out_channel, in_channel, kernel):
- return (out_channel, in_channel, kernel, kernel)
-
-
-@tvm.testing.fixture
-def shape_oihw8i32o4i(out_channel, in_channel, kernel):
- return get_packed_filter_layout(out_channel, in_channel, kernel, kernel)
Review comment:
I removed the fixtures because of their effect in terms of the ordering of parameters on the pytest command lines which I personally find confusing. I just realized, though, that the maxpool test in this same directory relies on these fixtures so I either need to remove that dependence or add back these fixtures.
--
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