-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CMSIS-NN] Add int16 add and mul operator support #13920
Conversation
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
Would be great to get some reviews on this if/when you folk have time! 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor points @FranklandJack, otherwise seems sensible to me 😸
4034492
to
55e4b4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall @FranklandJack. Just a nit below.
const auto& pattern = ParseBinaryElementwiseOpClipPattern(expr); | ||
Call mul_call = pattern.binary_op; | ||
const auto bit_width = mul_call->type_as<TensorTypeNode>()->dtype.bits(); | ||
int32_t output_min = -(1 << (bit_width - 1)); | ||
int32_t output_max = (1 << (bit_width - 1)) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we savestd::numeric_limits()
as part of the class and use it where needed? Looks more standard than bit based calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good shout. I've tried to replace some code duplication throughout this file, so let me konw if that's what you had in mind :D
55e4b4d
to
6b0fc76
Compare
The CMSIS-NN backend will now partition quantized `int16` additions and multiplication operators and emit calls to `arm_elementwise_add_s16` and `arm_elementwise_mul_s16` respectively during codegen. Because `arm_elementwise_mul_s16` and `arm_elementwise_add_s16` do not handle non-zero shift parameters at present, for non-zero zero point values (which map directly to shift in the CMSIS-NN backend) partitioning fails and we fall back on the regular C codegen path. This patch also adds `int16` tests, including testing for the non-zero zero point edge case described above.
6b0fc76
to
27ec6a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @FranklandJack! Thanks @Mousius for the reviews.
The CMSIS-NN backend will now partition quantized
int16
additions and multiplication operators and emit calls toarm_elementwise_add_s16
andarm_elementwise_mul_s16
respectively during codegen.Because
arm_elementwise_mul_s16
andarm_elementwise_add_s16
do not handle non-zero shift parameters at present, for non-zero zero point values (which map directly to shift in the CMSIS-NN backend) partitioning fails and we fall back on the regular C codegen path.This patch also adds
int16
tests, including testing for the non-zero zero point edge case described above.