Support for DECODE operator #3132
Conversation
@tensorflow/micro Add initial support for DECODE operator. Add reference implementation. Add LUT decompression support. Update op resolvers. Update Makefiles and Bazel BUILD files. Add kernel unit test. bug=fixes tensorflow#3131
…em). Cleanup unit test.
veblush
left a comment
veblush
left a comment
There was a problem hiding this comment.
Thanks for the PR! This looks great, I've just left a few minor suggestions.
|
|
||
| namespace tflite { | ||
|
|
||
| struct DecodeState { |
There was a problem hiding this comment.
Please change this to class per https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
|
|
||
| namespace tflite { | ||
|
|
||
| struct DecodeStateLUT : public DecodeState { |
There was a problem hiding this comment.
Please rename this to DecodeStateLut per https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
| #include "tensorflow/lite/micro/micro_profiler_interface.h" | ||
|
|
||
| namespace tflite { | ||
|
|
There was a problem hiding this comment.
What do you think about converting DecodeState into an interface called IDecodeAlgorithm? This would clarify its role, and we could then reorganize the helper functions into a separate container class.
There was a problem hiding this comment.
Perhaps as another PR, after we refactor the DECODE code for size? I don't know yet what DecodeState will look like after the refactor.
| @@ -0,0 +1,344 @@ | |||
| /* Copyright 2025 The TensorFlow Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
Would it be clearer if we split this into separate tests, with each one focused on a specific algorithm?
There was a problem hiding this comment.
This will be done starting with the next PR (pruning).
| }; | ||
|
|
||
| // Align the tensor data the same as a Buffer in the TfLite schema | ||
| alignas(16) const uint8_t kEncodedLUT[] = {0x1B, 0xE4}; |
There was a problem hiding this comment.
It'd be useful to have a comment as to how this is constructed.
| count -= 1; | ||
| } | ||
|
|
||
| // process elements in current channel in groups of 16 |
There was a problem hiding this comment.
Out of curiosity, why 16 instead of 8?
There was a problem hiding this comment.
I believe it was just copy/paste of DecompressToBufferWidth4_16 with modification of the masks/shifts. I don't remember any other reason.
| @@ -0,0 +1,344 @@ | |||
| /* Copyright 2025 The TensorFlow Authors. All Rights Reserved. | |||
|
|
|||
| Licensed under the Apache License, Version 2.0 (the "License"); | |||
There was a problem hiding this comment.
LUT has many combinations to cover. Then, do you have a plan to expand the coverage a bit?
There was a problem hiding this comment.
For the current decompression code, we have a comprehensive test of LUT decompression in tensorflow/lite/micro/kernels/decompress_test.cc
This test will be copied and made to work with DecodeState in a future PR.
|
@veblush |
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
@Mergifyio requeue |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
|
@Mergifyio refresh |
✅ Pull request refreshed |
3 participants
@tensorflow/micro
Add initial support for DECODE operator.
Add reference implementation.
Add LUT decompression support.
Update op resolvers.
Update Makefiles and Bazel BUILD files.
Add kernel unit test.
bug=fixes #3131