[TFLite] Support quantized GREATER op in TFLite frontend

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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

[dchauhan-arm]

Support GREATER quantization operation conversion as part of issue #9187

[dchauhan-arm]

cc @leandron and @lhutton1 for a second look

[lhutton1]

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

[leandron]

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

[dchauhan-arm]

@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]

@lhutton1 can you have another look on this one?

[lhutton1]

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

[dchauhan-arm]

cc @lhutton1 for a second (second) look

[lhutton1]

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

[lhutton1]

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]

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]

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