Skip to content
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

Add CMSIS-NN SVDF kernel. #42651

Merged

Conversation

jenselofsson
Copy link
Contributor

@jenselofsson jenselofsson commented Aug 25, 2020

This pull request adds the optimized version of the (int8) SVDF kernel in CMSIS-NN to TensorFlow Lite Micro.

The pull request to CMSIS-NN is still pending, and the CMSIS version in third_party_downloads.inc will need to be updated once that is merged.

Fixes #42650

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Aug 25, 2020
@gbaned gbaned self-assigned this Aug 25, 2020
@gbaned gbaned added comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers labels Aug 25, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 25, 2020
@gbaned gbaned requested a review from petewarden August 25, 2020 11:50
@gbaned gbaned added the awaiting review Pull request awaiting review label Aug 27, 2020
@advaitjain advaitjain requested review from njeffrie and advaitjain and removed request for petewarden August 28, 2020 17:39
Copy link
Contributor

@njeffrie njeffrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great - just one nitpick:
Since this is the first time we are duplicating the SVDF float code (xtensa_hifimini does not implement a floating point SVDF), do you think we can extract the contents of EvalFLoatSVDF into tensorflow/lite/kernels/internal/reference/svdf.h and share the implementation between the CMSIS-NN kernel and the reference kernel?

Let me know if this sounds reasonable to you.

@jenselofsson
Copy link
Contributor Author

This looks great - just one nitpick:
Since this is the first time we are duplicating the SVDF float code (xtensa_hifimini does not implement a floating point SVDF), do you think we can extract the contents of EvalFLoatSVDF into tensorflow/lite/kernels/internal/reference/svdf.h and share the implementation between the CMSIS-NN kernel and the reference kernel?

Let me know if this sounds reasonable to you.

Sounds good, I'll update the pull request with that change.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Sep 3, 2020
@jenselofsson
Copy link
Contributor Author

jenselofsson commented Sep 3, 2020

This looks great - just one nitpick:
Since this is the first time we are duplicating the SVDF float code (xtensa_hifimini does not implement a floating point SVDF), do you think we can extract the contents of EvalFLoatSVDF into tensorflow/lite/kernels/internal/reference/svdf.h and share the implementation between the CMSIS-NN kernel and the reference kernel?

Let me know if this sounds reasonable to you.

I had a bit of a look at it, and there seems to already be a EvalFloatSVDF() in tensorflow/kernels/inernal/reference/svdf.h. The difference between that EvalFloatSVDF() and the one found in micro is (among other things) that the micro-version have TfLiteEvalTensor as input. Should I still copy over the EvalFloatSVDF() function from micro to that file?

EDIT: On second, that won't work due to the fact that tensorflow/lite/kernels/reference/svdf.h depends on tensorflow/lite/kernels/reference/tensor_utils.h, which in turn depends on Eigen. @njeffrie do you have a preferred solution for this?

@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 8, 2020
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Sep 10, 2020
njeffrie
njeffrie previously approved these changes Sep 10, 2020
Copy link
Contributor

@njeffrie njeffrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we need to do some internal clean-up for the shared SVDF code before it will work for this. Let's land this first then we can internally manage extracting the float version into a common kernel in a future change.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 10, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 10, 2020
@gbaned gbaned removed the awaiting review Pull request awaiting review label Sep 11, 2020
@jenselofsson
Copy link
Contributor Author

It looks like we need to do some internal clean-up for the shared SVDF code before it will work for this. Let's land this first then we can internally manage extracting the float version into a common kernel in a future change.

Sounds good! Let me know if you need anything from my end.

@Abhipray
Copy link

Hi @jenselofsson, I am curious how you use this kernel? Is there a python implementation of svdf that converts to this tf lite op?

@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Sep 24, 2020
@gbaned
Copy link
Contributor

gbaned commented Sep 29, 2020

@jenselofsson Can you please resolve conflicts? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed ready to pull PR ready for merge process labels Sep 29, 2020
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Sep 30, 2020
@jenselofsson
Copy link
Contributor Author

@gbaned Done!

njeffrie
njeffrie previously approved these changes Sep 30, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Sep 30, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 30, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 30, 2020
@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label Oct 1, 2020
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Oct 2, 2020
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Oct 2, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 2, 2020
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Oct 2, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 2, 2020
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Oct 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 2, 2020
@tensorflow-copybara tensorflow-copybara merged commit 260d464 into tensorflow:master Oct 5, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Oct 5, 2020
@jenselofsson jenselofsson deleted the cmsisnn_svdf_kernel branch October 6, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

Add optimized SVDF kernel from CMSIS-NN
8 participants