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] SVDF kernel refactoring #47911
[TFLM] SVDF kernel refactoring #47911
Conversation
Moved prepare and float eval to svdf_common.cc
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 naming nits.
@@ -52,284 +41,12 @@ constexpr int kOutputTensor = 0; | |||
* and resizes the output tensor. Micro runtime does not support tensor | |||
* resizing. | |||
*/ |
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.
move the comment to svdf_common.cc as well
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
tensorflow/lite/micro/kernels/svdf.h
Outdated
constexpr int kInputTensor = 0; | ||
constexpr int kWeightsFeatureTensor = 1; | ||
constexpr int kWeightsTimeTensor = 2; | ||
constexpr int kBiasTensor = 3; | ||
// This is a variable tensor, and will be modified by this op. | ||
constexpr int kInputActivationStateTensor = 4; | ||
|
||
// Output tensor. | ||
constexpr int kOutputTensor = 0; |
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.
make extern const and add Svdf prefix (e.g. kSvdfInputTensor)
tensorflow/tensorflow/lite/micro/kernels/fully_connected.h
Lines 43 to 46 in d73ba75
extern const int kFullyConnectedInputTensor; | |
extern const int kFullyConnectedWeightsTensor; | |
extern const int kFullyConnectedBiasTensor; | |
extern const int kFullyConnectedOutputTensor; |
See section on constants in header files: https://abseil.io/tips/140
Since we are not using c++17, we can not use inline constexpr and and extern const is the way to go for us.
And add definition in svdf_common.cc:
tensorflow/tensorflow/lite/micro/kernels/fully_connected_common.cc
Lines 29 to 32 in d73ba75
const int kFullyConnectedInputTensor = 0; | |
const int kFullyConnectedWeightsTensor = 1; | |
const int kFullyConnectedBiasTensor = 2; | |
const int kFullyConnectedOutputTensor = 0; |
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
tensorflow/lite/micro/kernels/svdf.h
Outdated
@@ -46,6 +57,16 @@ void EvalIntegerSvdfReference(TfLiteContext* context, TfLiteNode* node, | |||
TfLiteEvalTensor* output_tensor, | |||
const OpData& data); | |||
|
|||
void EvalFloatSVDF(TfLiteContext* context, TfLiteNode* node, |
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: change name to EvalFloatSvdfReference
note Svdf instead of SVDF.
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
tensorflow/lite/micro/kernels/svdf.h
Outdated
int scratch_tensor_index, TfLiteEvalTensor* activation_state, | ||
TfLiteEvalTensor* output); | ||
|
||
TfLiteStatus PrepareSVDF(TfLiteContext* context, TfLiteNode* node); |
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.
PrepareSvdf
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
In preparation for a PR for CEVA-DSP optimized SVDF kernel - moved the Prepare function into svdf_common as we did for previous kernels + since we have an optimized implementation for float32 as well, moved the reference code into svdf_common too so it can be used a fallback.
(please ignore the depthwise commit in there, was removed)