Samsung / ONE

On-device Neural Engine
Other
434 stars 157 forks source link

[onert] Suspicious logic in cker optimized quantized brodcast binary arithmetic kernels #4540

Open krayzemli opened 4 years ago

krayzemli commented 4 years ago

Browsing the code I noticed that BinaryBroadcastFiveFold swaps arguments, so that broadcast tensor is always the first one. This is ok for commutative float operations, but quantized ops may use different quantization settings for inputs, and these settings are not swapped together with tensor data. This needs checking. If I'm right, this may also mean that the test set for binary arithmetics is incomplete.

glistening commented 4 years ago

@krayzemli BinaryBroadcastFiveFold seems to be used in Add and Mul only.

krayzemli commented 4 years ago

Both BroadcastAddDispatchQuant8 and BroadcastMulDispatchQuant8 use BinaryBroadcastFiveFold with inputs (but not quantization) swapping, and both are used in BroadcastBinaryArithmeticOp specialization for uint8_t. BTW, quantized SUB is implemented by using ADD and negating quantization multiplicator of the second argument, and so, quantized broadcast SUB would also break when the second argument is the one to be broadcasted, even if quantization of both arguments is the same,

periannath commented 4 years ago

@krayzemli

but quantized ops may use different quantization settings for inputs, and these settings are not swapped together with tensor data.

You're right. Quantization params should be swapped along with tensor data. There is a nnapi test which tests subtraction of two tensors with different scales (sub_quantized_different_scales.mod.py). I think we should enable this test.