ashutosh-arm commented on code in PR #13498:
URL: https://github.com/apache/tvm/pull/13498#discussion_r1033514668
##########
python/tvm/relay/op/contrib/cmsisnn.py:
##########
@@ -310,8 +310,8 @@ def check_qnn_max_pool2d(pattern):
return (
pooling.attrs.layout == "NHWC"
and int(input_op.checked_type.shape[0]) == 1
- and input_op.checked_type.dtype == "int8"
- and output.checked_type.dtype == "int8"
+ and (input_op.checked_type.dtype == "int8" or
input_op.checked_type.dtype == "int16")
+ and (output.checked_type.dtype == "int8" or
output.checked_type.dtype == "int16")
Review Comment:
Consider the following check for better filtering:
```
(input_op.checked_type.dtype == "int8" and output.checked_type.dtype ==
"int8" ) or
(input_op.checked_type.dtype == "int16" and output.checked_type.dtype ==
"int16")
```
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -456,8 +467,13 @@ class RelayToTIRVisitor : public MixedModeMutator {
clip_min = clip_attrs->a_min;
clip_max = clip_attrs->a_max;
} else {
- clip_min = -128;
- clip_max = 127;
+ if (dtype_bits == 8) {
+ clip_min = std::numeric_limits<int8_t>::min();
+ clip_max = std::numeric_limits<int8_t>::max();
+ } else {
+ clip_min = std::numeric_limits<int16_t>::min();
+ clip_max = std::numeric_limits<int16_t>::max();
Review Comment:
This looks so much better :smile:
##########
tests/python/contrib/test_cmsisnn/test_pooling.py:
##########
@@ -103,18 +105,17 @@ def test_op_int8(
compiler_cpu,
cpu_flags,
):
- """Tests QNN pooling op for int8 inputs"""
+ """Tests QNN pooling op for int8 and int16 inputs"""
Review Comment:
At present, we support int8, int16 input and output for the pool. Instead of
`input`, we could say `pooling`. This also suggests that we should have a
negative test where dtypes of the input and output to a pool op sequence do not
match. For such cases, the CMSIS-NN partitioner should fail. To be clear,
(int8, int16)/(int16,int8) kind of combinations should get mapped to TVM
inplace of CMSIS-NN.
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -442,7 +449,11 @@ class RelayToTIRVisitor : public MixedModeMutator {
pool_size_h = qnn::get_const_int(attrs->pool_size[0]);
pool_size_w = qnn::get_const_int(attrs->pool_size[1]);
} else {
- cmsisnn_api = "arm_max_pool_s8";
+ if (dtype_bits == 8)
+ cmsisnn_api = "arm_max_pool_s8";
+ else
+ cmsisnn_api = "arm_max_pool_s16";
Review Comment:
nit: Adding braces in the if-else block should make it more aligned to the
Google coding style. Reference:
https://google.github.io/styleguide/cppguide.html#Conditionals
##########
src/relay/backend/contrib/cmsisnn/tir_to_runtime.cc:
##########
@@ -118,7 +118,8 @@ class CodeGenCMSISNN : public codegen::CodeGenCHost {
} else if (cmsis_func_name == "arm_fully_connected_s8" ||
cmsis_func_name == "arm_fully_connected_s16") {
EmitFullyConnected(op);
- } else if (cmsis_func_name == "arm_avgpool_s8" || cmsis_func_name ==
"arm_max_pool_s8") {
+ } else if (cmsis_func_name == "arm_avgpool_s8" || cmsis_func_name ==
"arm_avgpool_s16" ||
+ cmsis_func_name == "arm_max_pool_s8" || cmsis_func_name ==
"arm_max_pool_s16") {
Review Comment:
I wish C++ had a single line of looking up into a list the way python allows
:crying_cat_face:
--
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]
