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

[TFLite] Support quantized GREATER op in TFLite frontend #11519

Closed
wants to merge 0 commits into from

Conversation

dchauhan-arm
Copy link
Contributor

Support GREATER quantization operation conversion as part of issue #9187

@dchauhan-arm
Copy link
Contributor Author

dchauhan-arm commented Jun 22, 2022

cc @leandron and @lhutton1 for a second look

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

If I inspect the Relay graph(s) that is tested when running _test_forward_elemwise_quantized(_test_greater, True), it seems as though it doesn't get quantized in any way e.g.

def @main(%inq_0: Tensor[(3, 6), float32] /* ty=Tensor[(3, 6), float32] */, %inq_1: Tensor[(3, 6), float32] /* ty=Tensor[(3, 6), float32] */, output_tensor_names=["Greater"]) -> Tensor[(3, 6), bool] {
  greater(%inq_0, %inq_1) /* ty=Tensor[(3, 6), bool] */
}

So then it seems we're still only testing the float32 implementation of the operator? For a quantized operator I'd expect int/uint inputs being passed to the greater op

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

@dchauhan-arm just a friendly ping to update this PR with quantised testing for the operator.

@dchauhan-arm
Copy link
Contributor Author

@dchauhan-arm just a friendly ping to update this PR with quantised testing for the operator.

yes, what I think I'll do is rewrite a test specifically for the greater op, and compare output of tvm against tflite.

@leandron
Copy link
Contributor

@lhutton1 can you have another look on this one?

@lhutton1
Copy link
Contributor

I believe its still waiting on an update, @dchauhan-arm please let me know if that's not the case

@dchauhan-arm
Copy link
Contributor Author

cc @lhutton1 for a second (second) look

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks for persisting with this @dchauhan-arm, the quantized graph we are testing looks better now! Just a small nit, but otherwise LGTM :)

tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
@lhutton1
Copy link
Contributor

hmmm looks like there's a seg fault happening in test_elemwise[_test_greater] on aarch64 CI, so maybe there is an issue with this version of tensorflow on aarch64

@NicolaLancellotti
Copy link
Contributor

With tflite 2.9.1 test_elemwise[_test_greater] does not fail any more. We should wait for #12130 and #12131 to be merged.

@leandron
Copy link
Contributor

leandron commented Sep 9, 2022

This is now unblocked by #12738 - @dchauhan-arm can you remove the conflict and resubmit this for CI to check?

dchauhan-arm added a commit to dchauhan-arm/tvm that referenced this pull request Sep 12, 2022
Support GREATER quantization operation conversion as part of issue apache#9187
Continuation of apache#11519
lhutton1 pushed a commit that referenced this pull request Sep 12, 2022
Support GREATER quantization operation conversion as part of issue #9187 Continuation of #11519.
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
Support GREATER quantization operation conversion as part of issue apache#9187 Continuation of apache#11519.
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.

4 participants