lhutton1 commented on code in PR #16106:
URL: https://github.com/apache/tvm/pull/16106#discussion_r1390915499
##########
python/tvm/relay/op/strategy/arm_cpu.py:
##########
@@ -242,7 +242,13 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type,
target):
),
name="conv2d_NHWC_quantized_interleaved.arm_cpu",
)
- if (not is_aarch64) or (data.dtype not in ["int8", "uint8"]):
+ if is_aarch64 and data.dtype not in ["int8", "uint8"]:
+ strategy.add_implementation(
+
wrap_compute_conv2d(topi.arm_cpu.compute_conv2d_NHWC_fp32_hybrid),
+
wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_NHWC_fp32_hybrid),
+ name="conv2d_NHWC_fp32_hybrid.arm_cpu",
+ )
+ else:
Review Comment:
This seems to change the logic and allows the below schedule to be selected
when `is_aarch64=True` and `data.dtype in ["int8", "uint8]`, is that
intentional?
##########
python/tvm/relay/op/strategy/arm_cpu.py:
##########
@@ -517,9 +523,12 @@ def
conv2d_gemm_without_weight_transform_strategy_arm_cpu(attrs, inputs, out_typ
name="conv2d_NHWC_quantized_interleaved_without_transform.arm_cpu",
)
else:
- raise RuntimeError(
- f"Unsupported conv2d_NHWC_quantized_without_transform layout
{layout}"
- f"with datatype {data.dtype}"
+ strategy.add_implementation(
Review Comment:
I think there needs to be an `is_aarch64` check here?
As a side note, I wonder if and how this schedule performs on a non-aarch64
device? Maybe we can enable it in addition to the `spatial_pack` schedule.
Let's consider it in a separate change though
##########
python/tvm/topi/arm_cpu/arm_utils.py:
##########
@@ -36,59 +36,68 @@ def get_tiling_B_interleaved_t(interleave_A):
Parameters
----------
- interleave_A: bool
- determines if A is expected to be interleaved
+ interleave_A : bool
+ determines if A is expected to be interleaved
+ in_dtype : str
+ input datatype
Review Comment:
nit: I think the norm is to use one space after the ":" and one indent
before the sentence i.e.
```suggestion
interleave_A : bool
determines if A is expected to be interleaved
in_dtype : str
input datatype
```
##########
python/tvm/topi/arm_cpu/conv2d_gemm.py:
##########
@@ -90,7 +91,7 @@ def compute_conv2d_gemm_without_weight_transform(
OH = (IH + pad_top + pad_down - dilated_kernel_h) // HSTR + 1
OW = (IW + pad_left + pad_right - dilated_kernel_w) // WSTR + 1
- if pad_top or pad_left:
+ if pad_top or pad_left or pad_down or pad_right:
Review Comment:
Is there a test that covers this fix?
##########
tests/python/integration/test_arm_aprofile.py:
##########
@@ -49,6 +49,7 @@ def test_conv2d(dtype):
invar,
weight,
kernel_size=kernel_size,
+ channels=2,
Review Comment:
curious why this change is needed?
##########
python/tvm/relay/op/strategy/arm_cpu.py:
##########
@@ -242,7 +242,13 @@ def conv2d_strategy_arm_cpu(attrs, inputs, out_type,
target):
),
name="conv2d_NHWC_quantized_interleaved.arm_cpu",
)
- if (not is_aarch64) or (data.dtype not in ["int8", "uint8"]):
+ if is_aarch64 and data.dtype not in ["int8", "uint8"]:
+ strategy.add_implementation(
+
wrap_compute_conv2d(topi.arm_cpu.compute_conv2d_NHWC_fp32_hybrid),
+
wrap_topi_schedule(topi.arm_cpu.schedule_conv2d_NHWC_fp32_hybrid),
+ name="conv2d_NHWC_fp32_hybrid.arm_cpu",
Review Comment:
The schedule name suggests it can only be used for fp32, although it seems
applicable to other types as well like fp16. I think we either need to check
the types here i.e. `data.dtype == "float32"` or rename the schedule
--
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]
