lhutton1 commented on code in PR #15407:
URL: https://github.com/apache/tvm/pull/15407#discussion_r1277396906
##########
tests/python/contrib/test_cmsisnn/test_softmax.py:
##########
@@ -91,6 +91,50 @@ def test_op_int8(zero_point, scale, compiler_cpu, cpu_flags):
)
+@skip_if_no_reference_system
[email protected]_cmsisnn
[email protected](["zero_point", "scale"], [[0, 1.0 / 32768]])
[email protected](
+ "compiler_cpu, cpu_flags", [("cortex-m55", "+nomve"), ("cortex-m55", ""),
("cortex-m7", "")]
+)
+def test_op_int16(zero_point, scale, compiler_cpu, cpu_flags):
Review Comment:
could this test be combined into the test above and use pytest's
parameterization to cover both the cases? and then rename that test to
`test_op`?
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -517,36 +536,111 @@ class RelayToTIRVisitor : public MixedModeMutator {
// calculate multiplier and shift for CMSIS-NN softmax API
// Note: TensorFlow Lite Micro assumptions
- // Output zero point and scale are fixed to -128 and 1 / 256
+ // Output zero point and scale are fixed to -128 and 1 / 256 in the case
of an int8 operator
+ // or to 0 and 1 / 32768.
// kScaledDiffIntegerBits, kInputBits, kBeta are described on the
following github page
- //
https://github.com/tensorflow/tflite-micro/blob/d97cd0908d8cf5021e9d86f05a49888bee28c2a4/tensorflow/lite/micro/kernels/softmax_common.cc#L47
- double beta_multiplier = (kBeta * quant_scale * (1 << (31 - kInputBits)));
- beta_multiplier = std::min<double>(beta_multiplier, (1ll << 31) - 1.0);
- auto mult_shift_pair =
tvm::relay::qnn::GetFixedPointMultiplierShift(beta_multiplier);
- int32_t mult = std::get<0>(mult_shift_pair);
- int32_t shift = std::get<1>(mult_shift_pair);
- int32_t diff_min = (1 << kScaledDiffIntegerBits) - 1;
- diff_min <<= (31 - kScaledDiffIntegerBits);
- diff_min >>= shift;
- diff_min *= -1;
+ //
https://github.com/tensorflow/tflite-micro/blob/d97cd0908d8cf5021e9d86f05a49888bee28c2a4/tensorflow/lite/exp_zero_pointmicro/kernels/softmax_common.cc#L47
Review Comment:
this links to a 404 😥 maybe unintentional change?
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -107,8 +117,16 @@ class RelayToTIRVisitor : public MixedModeMutator {
{context_buffer_size}, tir::const_true(), body);
}
+ for (int i = 0; i < static_cast<int>(context_const_buffer_vars.size());
i++) {
+ body =
tir::AllocateConst(Downcast<tir::Var>(context_const_buffer_vars[i].buffer_var),
+
DataType::Int(context_const_buffer_vars[i].num_bits),
+ context_const_buffer_vars[i].extents,
Review Comment:
Can we get `num_bits` and `extent` from the NDArray itself? (shape and dtype
may help here)
##########
src/relay/backend/contrib/cmsisnn/compute_luts.cc:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.
+ */
+
Review Comment:
nit: is it possible to add a comment here to explain what the file is for,
similar to:
https://github.com/apache/tvm/blob/da8335378af0a8454bb23b1aa5638a520fc5cb94/src/relay/backend/contrib/cmsisnn/fuse_pads.cc#L19
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -517,36 +536,111 @@ class RelayToTIRVisitor : public MixedModeMutator {
// calculate multiplier and shift for CMSIS-NN softmax API
// Note: TensorFlow Lite Micro assumptions
- // Output zero point and scale are fixed to -128 and 1 / 256
+ // Output zero point and scale are fixed to -128 and 1 / 256 in the case
of an int8 operator
+ // or to 0 and 1 / 32768.
Review Comment:
should this sentence have `for an int16 operator` on the end? So it reads:
`Output zero point and scale are fixed to -128 and 1 / 256 in the case of an
int8 operator or to 0 and 1 / 32768 for an int16 operator`
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -89,11 +90,20 @@ class RelayToTIRVisitor : public MixedModeMutator {
private:
inline IntImm ToArg(int32_t value) { return IntImm(DataType::Int(32),
value); }
+ // struct used to allocated const NDArray
+ struct user_const {
+ tir::Var buffer_var;
+ int num_bits;
+ Array<PrimExpr> extents;
+ tvm::runtime::NDArray ndarray;
+ };
+
void CreatePrimFuncForExtern(const GlobalVar& global_var, Array<tir::Var>
func_signature,
const Map<tir::Var, tir::Buffer>& buffer_map,
tvm::Array<PrimExpr> call_extern_args,
PrimExpr context_buffer_var = PrimExpr(),
- int context_buffer_size = 0, int num_bits = 8) {
+ int context_buffer_size = 0, int num_bits = 8,
+ std::vector<user_const>
context_const_buffer_vars = {}) {
Review Comment:
I'm wondering if it's possible to extend the `BufferCreator` class to
support constants, this way we can avoid the need for passing in this
additional `context_const_buffer_vars` parameter and avoid the need for the
`user_const` struct? Maybe I've missed something though as it seems
`context_buffer` has a similar situation. Maybe @ashutosh-arm knows more about
this?
##########
src/relay/backend/contrib/cmsisnn/relay_to_tir.cc:
##########
@@ -517,36 +536,111 @@ class RelayToTIRVisitor : public MixedModeMutator {
// calculate multiplier and shift for CMSIS-NN softmax API
// Note: TensorFlow Lite Micro assumptions
- // Output zero point and scale are fixed to -128 and 1 / 256
+ // Output zero point and scale are fixed to -128 and 1 / 256 in the case
of an int8 operator
+ // or to 0 and 1 / 32768.
// kScaledDiffIntegerBits, kInputBits, kBeta are described on the
following github page
- //
https://github.com/tensorflow/tflite-micro/blob/d97cd0908d8cf5021e9d86f05a49888bee28c2a4/tensorflow/lite/micro/kernels/softmax_common.cc#L47
- double beta_multiplier = (kBeta * quant_scale * (1 << (31 - kInputBits)));
- beta_multiplier = std::min<double>(beta_multiplier, (1ll << 31) - 1.0);
- auto mult_shift_pair =
tvm::relay::qnn::GetFixedPointMultiplierShift(beta_multiplier);
- int32_t mult = std::get<0>(mult_shift_pair);
- int32_t shift = std::get<1>(mult_shift_pair);
- int32_t diff_min = (1 << kScaledDiffIntegerBits) - 1;
- diff_min <<= (31 - kScaledDiffIntegerBits);
- diff_min >>= shift;
- diff_min *= -1;
+ //
https://github.com/tensorflow/tflite-micro/blob/d97cd0908d8cf5021e9d86f05a49888bee28c2a4/tensorflow/lite/exp_zero_pointmicro/kernels/softmax_common.cc#L47
Review Comment:
this links to a 404 😥 maybe unintentional change?
##########
tests/python/contrib/test_cmsisnn/test_softmax.py:
##########
@@ -91,6 +91,50 @@ def test_op_int8(zero_point, scale, compiler_cpu, cpu_flags):
)
+@skip_if_no_reference_system
[email protected]_cmsisnn
[email protected](["zero_point", "scale"], [[0, 1.0 / 32768]])
[email protected](
+ "compiler_cpu, cpu_flags", [("cortex-m55", "+nomve"), ("cortex-m55", ""),
("cortex-m7", "")]
+)
+def test_op_int16(zero_point, scale, compiler_cpu, cpu_flags):
+ """Tests int16 QNN Softmax for CMSIS-NN"""
+ print()
Review Comment:
nit: remove print
--
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]
