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
[TFLM] Added optimized softmax kernel (int32 and float) for CEVA-BX1 and CEVA SP500 #47783
[TFLM] Added optimized softmax kernel (int32 and float) for CEVA-BX1 and CEVA SP500 #47783
Conversation
Thanks for contributing to TensorFlow Lite Micro. To keep this process moving along, we'd like to make sure that you have completed the items on this list:
We would like to have a discussion on the Github issue first to determine the best path forward, and then proceed to the PR review. |
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.
small comments only.
Happy to make a PR that removes uint8 support from the reference kernel by this Friday.
#if defined(CEVA_BX1) || defined(CEVA_SP500) | ||
extern int32_t* CEVA_TFLM_KERNELS_SCRATCH; | ||
extern int32_t CEVA_TFLM_KERNELS_SCRATCH_SIZE_VAL; | ||
#endif |
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.
worth putting these in a ceva_common.h or something similar to avoid repeating the same include and extern in each of the kernels?
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.
Done
const TfLiteEvalTensor* input, | ||
TfLiteEvalTensor* output, | ||
const SoftmaxParams& op_data) { | ||
if (input->type == kTfLiteUInt8) { |
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.
drop uint8 support?
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.
Done
#ifdef MCPS_MEASUREMENT | ||
#include "tensorflow/lite/micro/kernels/ceva/mcps_macros.h" | ||
#endif |
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.
you could add these to ceva_common.h as well?
No change needed in this PR. If you think it is worthwhile then you can make the change in all the kernels in a follow-up PR.
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'd like to have our measurement macros in all kernels, but only some of them require scratch memory, so I'll leave it as is for the time being. Eventually I think we will do some work on the whole scratch usage issue, exact size calculations per kernel etc and hopefully come up with something maintainable.
Relevant github issue:#45607