[TFLite] Fix potential overflow in 64-bit MultiplyByQuantizedMultiplier function

[Tessil]

Hi,

This PR fixes potential overflows than can happen in the 64-bit version of MultiplyByQuantizedMultiplier.

Overflow example:

int32_t quantized_multiplier;
int shift;
double scale = 0.00097656;
QuantizeMultiplier(scale, &quantized_multiplier, &shift);

const int64_t x = 10000;
std::cout << x*scale << std::endl;
std::cout << MultiplyByQuantizedMultiplier(x, quantized_multiplier, shift) << std::endl;

Output:

9.7656
-10 // Should be 10

Thanks,
Thibaut

[Tessil]

Hi @suharshs, could you take a look to the PR? Thanks.

[advaitjain]

Apologies for the delay. If @suharshs can take a look, that would be great.

I would usually ask for a test that currently fails but passes with the change but I don't see any existing tests for MultiplyByQuantizedMultipler so I'll defer to @suharshs on this as well.

[gbaned]

@jianlijianli, @daverim Can you please take a look on this PR ? Thanks!

[talumbau]

Thanks for this change! Looks good. I looked around for the right spot for a test. It looks like it the test should go in tensorflow/lite/kernels/internal/quantization_util_test.cc (it already includes this common.h). We should have more tests for this function, so it would be good to start with at least one. Thanks!

[Tessil]

Thank you, I hesitated as there was no existing tests for any of the MultiplyByQuantizedMultiplier versions. I will check to add some tests as these are quite central functions.

[Tessil]

@talumbau I added some tests for the int32 and int64 versions of the MultiplyByQuantizedMultiplier function.

I had to modify a bit the BUILD file to link libm as I had some undefined reference to symbol 'round@@GLIBC_2.2.5 error when compiling the quantization_util_test.

[talumbau]

Hi Thibaut,

I looked at the Windows Bazel GPU CI build failure, as well as the Intel oneDNN build failure. They look spurious to me and not related to this change. Can you attempt to rebase and invoke the submit process again? I believe that change in 0cead48 should be good.

[Tessil]

I merged the latest changes, thanks.

[talumbau]

Looks like there are some builds with additional warnings

third_party/tensorflow/lite/kernels/internal/quantization_util_test.cc:453:49: error: operator '<<' has lower precedence than '-'; '-' will be evaluated first [-Werror,-Wshift-op-parentheses]
                  static_cast<double>(1ll << 31 - (-3)));
                                          ~~ ~~~^~~~~~
third_party/tensorflow/lite/kernels/internal/quantization_util_test.cc:453:49: note: place parentheses around the '-' expression to silence this warning
                  static_cast<double>(1ll << 31 - (-3)));
                                                ^

Can you fix please?

[Tessil]

Thank you, I fixed the warning.

Is there a reason to not enable these warnings and -Werror in the GitHub CI build? I have to see if we can modify a bit our internal CI to more closely match the warnings you use.

[talumbau]

That's a good idea. I have filed this issue internally. Thanks for your patience.

[talumbau]

Looks like the Github CI needs significant improvement. I see a failure with this command:

bazel --blazerc=/dev/null test --compilation_mode=opt --copt=-UNDEBUG tensorflow/lite/kernels/internal:quantization_util_test

it fails on the assert here:

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/lite/kernels/internal/common.h#L179

Can you reproduce?

[Tessil]

Thank you, the 64-bit MultiplyByQuantizedMultiplier effectively doesn't support negative multipliers. From what I gather it's an intentional behaviour and some HW implementations rely on the fact that the multiplier is >= 0. The TOSA spec also has an assert(multiplier >= 0); in apply_scale_32.

I removed the tests with negative multipliers for the 64-bit version but left them for the 32-bit version as some kernels use negative multipliers (notably the SUB operator to reuse the ADD kernel).