[GitHub] [tvm] comaniac commented on a change in pull request #7142: Asymmetric padding in conv2d workload

[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