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 2020/12/21 21:05:34 UTC
[GitHub] [tvm] Wheest opened a new pull request #7142: Asymmetric padding in conv2d workload
Wheest opened a new pull request #7142:
URL: https://github.com/apache/tvm/pull/7142
The goal of this pull request is to make asymmetric padding a first-class citizen in 2D convolution in TOPI.
The current workload description has `"hpad"` and `"wpad"`, however this is not representative of all of the possible configurations. Most TOPI conv2d implementations in TVM already support asymmetric padding, so I think this should be reflected in the workload description.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-752253140
Have added all specific requested changes @comaniac, and have added tests to:
**test_topi_conv2d_int8.py**
- [x] verify_conv2d_nchw_int8
**test_topi_conv2d_nchw.py**
- [x] verify_conv2d_nchw
**test_topi_depthwise_conv2d.py**
- [x] depthwise_conv2d_with_workload_nchw
These three tests cover all the workloads that are touched by this PR. Other data layouts use the same workload definitions. A similar test could be added to every single conv2d test, but am unsure if that is worth it right now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] FrozenGene commented on a change in pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r549682930
##########
File path: python/tvm/topi/nn/depthwise_conv2d.py
##########
@@ -38,20 +38,26 @@
"out_filter",
"hkernel",
"wkernel",
- "hpad",
- "wpad",
+ "padt",
+ "padl",
+ "padb",
+ "padr",
+ "hdilation",
+ "wdilation",
"hstride",
"wstride",
],
)
-def _get_workload(data, kernel, stride, padding, out_dtype):
+def _get_workload(data, kernel, stride, padding, dilation, out_dtype):
""" Get the workload structure. """
_, in_channel, height, width = [x.value for x in data.shape]
channel, channel_multiplier, kh, kw = [x.value for x in kernel.shape]
out_channel = channel * channel_multiplier
- HPAD, WPAD, _, _ = get_pad_tuple(padding, kernel)
+ pt, pl, pb, pr = get_pad_tuple(padding, kernel)
+ hdilation, wdilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation)
Review comment:
Should be in the workload tuple too.
In fact, it should also be `stride_h` / `stride_w` in the code style.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-752271578
Thanks @Wheest @FrozenGene
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest edited a comment on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest edited a comment on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-750293513
Thanks for the tip! I've added a test for `conv2d_nchw_int8`, with a structure much like you suggested. It fails in `main`, but is fixed in this PR. Through the test, I identified a silly mistake I had made in the workload definition in the original commit, now fixed.
Does the style of this test meet the standards you expect? E.g. is the test being called as a nested function of `verify_conv2d_nchw_int8` the right place for this?
If so, I will add a similar test for all of the ops touched by this PR. Otherwise, happy to take some suggestions on how to improve this test before I reproduce it elsewhere.
*EDIT*
CI still failed, I should have run *all* tests locally, rather than just the one I added. I have discovered that dilation is also not used in the workload, so this PR will also add dilation to avoid this issue. Will push the fix when available, earlier question about the test standard remains.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac merged pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #7142:
URL: https://github.com/apache/tvm/pull/7142
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-749775593
As you pointed out, the workload doesn't handle asymmetric padding as the compute implementation, which looks like a bug to me. However, it never triggers CI errors before, meaning that there aren't existing test cases for it. As a result, I'm expecting to have a test case that requires this PR to pass. For example, `conv2d_avx_1x1` has `out_height = (wkl.height + 2 * HPAD - wkl.hkernel) // HSTR + 1` before this PR. Then you will get a wrong `out_height` if you provide a workload with asymmetric padding without this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-750293513
Thanks for the tip! I've added a test for `conv2d_nchw_int8`, with a structure much like you suggested. It fails in `main`, but is fixed in this PR. Through the test, I identified a silly mistake I had made in the workload definition in the original commit, now fixed.
Does the style of this test meet the standards you expect? E.g. is the test being called as a nested function of `verify_conv2d_nchw_int8` the right place for this?
If so, I will add a similar test for all of the ops touched by this PR. Otherwise, happy to take some suggestions on how to improve this test before I reproduce it elsewhere.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-749822386
Thanks, I understand better what a good test for this PR would be: one that fails on the current `main` branch but not this PR.
I've been working on devising a test like this, but haven't got one that fails yet. Will keep working on it, but here's my reasoning so far:
afaik the workload data is only used the creation of fallback schedules. e.g. [for creating the `"tile_ow"` parameter for spatial pack convolution](https://github.com/apache/tvm/blob/main/python/tvm/topi/x86/conv2d_avx_common.py#L54).
So I imagine I would want to create a test that has padding such that something like `"tile_ow"` is generated with an incorrect value, creating a schedule that causes an invalid transformation.
e.g. `verify_conv2d_nchw(1, 64, 8, 128, 3, 1, (6, 6, 0, 0))`
If we are to focus on NCHWc convolution, which uses `"tile_ow"` in its schedule (via SplitEntity with `reg` in the `_fallback_schedule`) [`python/tvm/topi/x86/conv2d_avx_common.py#L119`](https://github.com/apache/tvm/blob/main/python/tvm/topi/x86/conv2d_avx_common.py#L119). There is no value we can set `reg` or `"tile_ow"` to that will cause an invalid transformation, even through silly hardcoding like `out_width = 10000` or `ow_chunk, ow_block = s[C].split(ow, factor=10000)`
It always works. Though perhaps it suffers a performance regression?
It could be happenstance that none of the conv2d schedules make transformations that are rendered invalid by having an incorrect value for the output height/width. That being the case, I'm unsure how I would devise a test for this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] FrozenGene commented on a change in pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r549674333
##########
File path: python/tvm/topi/nn/depthwise_conv2d.py
##########
@@ -38,20 +38,26 @@
"out_filter",
"hkernel",
"wkernel",
- "hpad",
- "wpad",
+ "padt",
+ "padl",
+ "padb",
+ "padr",
+ "hdilation",
+ "wdilation",
"hstride",
"wstride",
],
)
-def _get_workload(data, kernel, stride, padding, out_dtype):
+def _get_workload(data, kernel, stride, padding, dilation, out_dtype):
""" Get the workload structure. """
_, in_channel, height, width = [x.value for x in data.shape]
channel, channel_multiplier, kh, kw = [x.value for x in kernel.shape]
out_channel = channel * channel_multiplier
- HPAD, WPAD, _, _ = get_pad_tuple(padding, kernel)
+ pt, pl, pb, pr = get_pad_tuple(padding, kernel)
+ hdilation, wdilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation)
Review comment:
name them `dilation_h` and `dilation_w` as other places' code style
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-749830449
I see what you meant. How about we just simply add a test in `test_topi_conv2d_int8.py` that directly calls `fallback_schedule_cpu_common_int8` that takes a workload generated by `_get_workload`? Something like:
```python
def test_worload_with_asmmetric_padding():
cfg = ...
wkl = _get_workload(...) # with asmmetric padding
int32_lanes = ...
num_int8_elements = ...
fallback_schedule_cpu_common_int8(cfg, wkl, int32_lanes, num_int8_elements)
assert cfg["tile_ow"] ... # check if tile_ow candidates are the factors of the right output weight.
```
So does the other ops changed by this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] FrozenGene commented on a change in pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
FrozenGene commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r549687060
##########
File path: python/tvm/topi/nn/depthwise_conv2d.py
##########
@@ -38,20 +38,26 @@
"out_filter",
"hkernel",
"wkernel",
- "hpad",
- "wpad",
+ "padt",
+ "padl",
+ "padb",
+ "padr",
+ "hdilation",
+ "wdilation",
"hstride",
"wstride",
],
)
-def _get_workload(data, kernel, stride, padding, out_dtype):
+def _get_workload(data, kernel, stride, padding, dilation, out_dtype):
""" Get the workload structure. """
_, in_channel, height, width = [x.value for x in data.shape]
channel, channel_multiplier, kh, kw = [x.value for x in kernel.shape]
out_channel = channel * channel_multiplier
- HPAD, WPAD, _, _ = get_pad_tuple(padding, kernel)
+ pt, pl, pb, pr = get_pad_tuple(padding, kernel)
+ hdilation, wdilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation)
Review comment:
yeah.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r548090943
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -385,6 +387,18 @@ def get_ref_data():
a_np, w_np, b_np, c_np = get_ref_data()
+ def test_worload_padding(output_data, data, kernel, strides, padding, out_dtype):
+ _, _, out_height, out_width = get_const_tuple(output_data.shape)
+ wkl = _get_workload(data, kernel, strides, padding, out_dtype)
+ int32_lanes, num_int8_elements = 16, 4
+
+ # check if tile_ow candidates are the factors of the right output weight.
+ cfg = autotvm.get_config()
+ fallback_schedule_cpu_common_int8(cfg, wkl, int32_lanes, num_int8_elements)
+ ow_tile = cfg["tile_ow"].size[-1] * cfg["tile_ow"].size[-2]
Review comment:
```suggestion
ow_tile_prod = np.prod(cfg["tile_ow"].size)
```
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -435,6 +449,7 @@ def check_device(device):
)
func(a, w, c)
tvm.testing.assert_allclose(c.asnumpy(), c_np, rtol=1e-5)
+ test_worload_padding(C, A, W, (stride, stride), padding, dtype)
Review comment:
It's ok to put this check in `verify_conv2d_nchw_int8` so that we can cover all existing and future unit tests. On the other hand, it seems like you don't need to test this function per device? For example, `fallback_schedule_cpu_common_int8 ` is only for CPU, so it doesn't make sense to test it within `check_device["cuda"]`. Maybe you can simply put this check before `check_device`?
```
verify_workload_padding(...)
for device in ["cuda"]:
check_device(device)
```
##########
File path: tests/python/topi/python/test_topi_conv2d_int8.py
##########
@@ -385,6 +387,18 @@ def get_ref_data():
a_np, w_np, b_np, c_np = get_ref_data()
+ def test_worload_padding(output_data, data, kernel, strides, padding, out_dtype):
Review comment:
1. Avoid using the prefix `test_`unless this is a standalone test function.
2. This function name is too vague, as you are testing a very specific schedule (i.e., `fallback_schedule_cpu_common_int8`). Something like `verify_fallback_schedule_cpu_padding` might be better.
```suggestion
def verify_worload_padding(output_data, data, kernel, strides, padding, out_dtype):
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on a change in pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r549681173
##########
File path: python/tvm/topi/nn/depthwise_conv2d.py
##########
@@ -38,20 +38,26 @@
"out_filter",
"hkernel",
"wkernel",
- "hpad",
- "wpad",
+ "padt",
+ "padl",
+ "padb",
+ "padr",
+ "hdilation",
+ "wdilation",
"hstride",
"wstride",
],
)
-def _get_workload(data, kernel, stride, padding, out_dtype):
+def _get_workload(data, kernel, stride, padding, dilation, out_dtype):
""" Get the workload structure. """
_, in_channel, height, width = [x.value for x in data.shape]
channel, channel_multiplier, kh, kw = [x.value for x in kernel.shape]
out_channel = channel * channel_multiplier
- HPAD, WPAD, _, _ = get_pad_tuple(padding, kernel)
+ pt, pl, pb, pr = get_pad_tuple(padding, kernel)
+ hdilation, wdilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation)
Review comment:
Should this also apply to their names in the Workload tuples? Or just the variables in the `_get_workload()` functions?
I named them `hdilation`, and `wdilation` in the Workload to follow the `"hstride"`, `"wstride",` style.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on pull request #7142: Asymmetric padding in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#issuecomment-749763887
Happy to add a test case if necessary, though I'm still to get familiar with the testing infrastructure for TVM.
Existing specific TOPI conv2d implementations are tested with asymmetric padding under `tests/python/topi/python/` (e.g. [test_topi_conv2d_nchw.py#L227](https://github.com/apache/tvm/blob/main/tests/python/topi/python/test_topi_conv2d_nchw.py#L227)).
This change is just ensuring that data is held in the workload too. If all of the existing tests pass, is that sufficient?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Wheest commented on a change in pull request #7142: Asymmetric padding and dilation in conv2d workload
Posted by GitBox <gi...@apache.org>.
Wheest commented on a change in pull request #7142:
URL: https://github.com/apache/tvm/pull/7142#discussion_r549684653
##########
File path: python/tvm/topi/nn/depthwise_conv2d.py
##########
@@ -38,20 +38,26 @@
"out_filter",
"hkernel",
"wkernel",
- "hpad",
- "wpad",
+ "padt",
+ "padl",
+ "padb",
+ "padr",
+ "hdilation",
+ "wdilation",
"hstride",
"wstride",
],
)
-def _get_workload(data, kernel, stride, padding, out_dtype):
+def _get_workload(data, kernel, stride, padding, dilation, out_dtype):
""" Get the workload structure. """
_, in_channel, height, width = [x.value for x in data.shape]
channel, channel_multiplier, kh, kw = [x.value for x in kernel.shape]
out_channel = channel * channel_multiplier
- HPAD, WPAD, _, _ = get_pad_tuple(padding, kernel)
+ pt, pl, pb, pr = get_pad_tuple(padding, kernel)
+ hdilation, wdilation = dilation if isinstance(dilation, (tuple, list)) else (dilation, dilation)
Review comment:
Thanks for the clarification, happy to change that too in my next commit. While I'm at it, should I change `"hkernel"` and `"wkernel",` in the workloads to `kernel_h`, `kernel_w`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org