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 Q16x8 Depthwise Conv Support #2140

Merged
merged 5 commits into from
Aug 1, 2023
Merged

Add Q16x8 Depthwise Conv Support #2140

merged 5 commits into from
Aug 1, 2023

Conversation

mbrooksx
Copy link
Contributor

@mbrooksx mbrooksx commented Jul 20, 2023

Adds support for 16-bit activations + 8-bit weights for depthwise convolution in the reference kernel. Uses 64-bit bias to match TFLite. Also adds passthrough to the q16x8 reference kernel for Xtensa, CEVA, and ARC (CMSIS already has it's own implementation).

Tested:
depthwise_conv_test

BUG=2141

Adds support for 16-bit activations + 8-bit weights for depthwise
convolution in the reference kernel. Uses 64-bit bias to match TFLite.

Tested:
depthwise_conv_test
@mbrooksx mbrooksx requested a review from a team as a code owner July 20, 2023 21:42
@mbrooksx mbrooksx marked this pull request as draft July 20, 2023 21:48
For ARC, CEVA, and Xtensa add in a reference fallback for q16x8
depthwise convolution. This allows these platforms to still run the same
models as well as fixes failed tests.
@rascani rascani added the ci:run label Jul 21, 2023
@TFLM-bot TFLM-bot removed the ci:run label Jul 21, 2023
The Xtensa deptwise conv was correct for eval, but the Hifi prepare
expects 8-bit IO. Early exit in the prepare to skip the Hifi and P6
prepare (those evals will never be called on q16 data).

BUG: 2141
@rascani rascani added the ci:run label Jul 21, 2023
@TFLM-bot TFLM-bot removed the ci:run label Jul 21, 2023
@mbrooksx mbrooksx marked this pull request as ready for review July 24, 2023 22:25
Copy link
Contributor

@rascani rascani left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment clarification and will approve.

@@ -48,6 +48,16 @@ void* Init(TfLiteContext* context, const char* buffer, size_t length) {

TfLiteStatus Prepare(TfLiteContext* context, TfLiteNode* node) {
TF_LITE_ENSURE_OK(context, DepthwiseConvPrepare(context, node));
// Use only the default depthwise convolution for int16 input.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expand this comment a bit to explain why we only need to do the default for int16? Specifically, we should mention that we fall back to the reference kernels for int16, so there's no need to call the Xtensa-specific Prepare methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Updated. I did once again forget the bug on this commit but it should work overall with a squash commit (with the top-level message above).

@mergify mergify bot merged commit a7846a1 into tensorflow:main Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants