-
Notifications
You must be signed in to change notification settings - Fork 74k
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
Cadence HiFi4 NN Library v2.2.0 update #40943
Cadence HiFi4 NN Library v2.2.0 update #40943
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
865cf87
to
2e274bb
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
7ac6995
to
b06b94c
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.
I have a few style nits. Other than that, looks good.
err = xa_nn_vec_activation_min_max_asym8_asym8( | ||
out_data_ptr, inp_data_ptr, 0, 255, flat_size); // Is 255 right? | ||
err = xa_nn_vec_activation_min_max_asym8_asym8(out_data_ptr, inp_data_ptr, | ||
zero, 255, flat_size); |
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 use std::numeric_limits<uint8_t>::min() and std::numeric_limits<uint8_t>::max() here?
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.
Updated as per suggestion.
&output_activation_max); | ||
tflite::ArithmeticParams op_params; | ||
SetActivationParams(output_activation_min, output_activation_max, &op_params); | ||
#define TF_LITE_ADD(opname) \ |
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.
Prefer to avoid defining this and only using once. Please inline it.
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.
Here we need the macro at couple of places, so keeping it. When the vector floating point unit (VFPU) hardware is not available, we need to fall back to the reference implementation.
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 like I missed some of the uses below. Looks good.
return kTfLiteError; | ||
} | ||
|
||
ALLOCATE_XTENSA_NNLIB_SCRATCH_MEM; | ||
p_scratch = xtensa_nnlib_scratch_buf; | ||
p_scratch = (char*)xtensa_nnlib_scratch_buf; |
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.
please use c++ style static_cast if possible or reinterpret_cast here and elsewhere in this file / change.
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.
Updated the code in conv.cc and depthwise_conv.cc to clean up type casts.
The following changes are done to the HiFi4 implementation of TFLM 1. Update the kernel files and the make setup to use HiFi4 NN Library v2.2.0. 2. Update the kernel files as per the latest reference implementation. 3. Add xtenas_hifi kernel implementations for the add and mul operators.
b06b94c
to
3b0642f
Compare
I have updated the PR to address the above suggestions, please review. |
&output_activation_max); | ||
tflite::ArithmeticParams op_params; | ||
SetActivationParams(output_activation_min, output_activation_max, &op_params); | ||
#define TF_LITE_ADD(opname) \ |
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 like I missed some of the uses below. Looks good.
The following changes are done to the HiFi4 implementation of TF Lite Micro