[TFLite 16x8] ADD/SUB operators: fixes + tests for versioning #42373
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…se + tests for versioning. Change-Id: Ib24a2c2b837eaeb4868afdf1445aa332965a41af
- POT int16x8: create a new BroadcastSub16POTSlow function to manage the POT scaling. - General int16x8: the BroadcastAdd4DSlow should be used instead of BroadcastSubSlow as the sign of input2 multiplier is changed in PrepareGeneralSubOp. Change-Id: Id8042d089af51f402cba72b1db9bb5d948ba5cbc
suharshs
requested review from
daverim and
jianlijianli
and removed request for
suharshs
akarmi
reviewed
Oct 2, 2020
| @@ -913,6 +913,7 @@ OperatorProperty GetOperatorProperty(const ModelT* model, int subgraph_index, | |||
| property.inputs = {{0, {}}, {1, {}}}; | |||
| property.outputs = {{0, {}}}; | |||
| property.version = 2; | |||
| property.restrict_same_input_output_scale = true; | |||
There was a problem hiding this comment.
The properties for ADD and SUB ops are different. Is it intended? What is the reason for that?
There was a problem hiding this comment.
Thanks for spotting! corrected
Change-Id: I0ddc0cd4db9604d3e84f5ce55ecbf8acc55c08d0
akarmi
approved these changes
Oct 6, 2020
google-ml-butler
bot
added
kokoro:force-run
tensorflowbutler
removed
the
stat:awaiting response
gbaned
added
stat:awaiting response
Change-Id: Ia7f43fe9d77596e6ba9ff85d9776b94440362b0a
fredrec
reviewed
Nov 18, 2020
| float GetTolerance(float min, float max) { | ||
| float kQuantizedStep = (max - min) / (std::numeric_limits<T>::max() - | ||
| std::numeric_limits<T>::min()); | ||
| return 2.0 * kQuantizedStep; |
There was a problem hiding this comment.
Where does the 2.0 come from ?
There was a problem hiding this comment.
Hi @fredrec Thanks for the review.
Tolerance should be 2 quantized steps for the result:
a_float = a_int16 +- quantized_step
if i do difference, then upper bound is 2*quantized_step.
gbaned
added
the
stat:awaiting tensorflower
fredrec
approved these changes
Dec 15, 2020
google-ml-butler
bot
added
kokoro:force-run
gbaned
added
ready to pull
gbaned
added
ready to pull
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR:
maximum version of ADD has been changed to 3 for 16x8, so I updated places, where it is mentioned as 4
both inputs for SUB operator should be quantized to int16 as for it is done for ADD
It has been suggested to modify quantize_model.cc file and set option pot_scale_int16 to false during quantization - implementation is done this way.