fix: Treat equivalent pooling qinfo as non-requantized#1293
Conversation
gunes-arm
left a comment
gunes-arm
left a comment
There was a problem hiding this comment.
Please link tickets if any
|
|
||
| namespace | ||
| { | ||
| bool requires_requantization(const ITensorInfo *src, const ITensorInfo *dst) |
There was a problem hiding this comment.
I'd make this inline. But, I think this is a wider problem. Many other places check if quantization informations are equal based on == operator overload of the QuantizationInfo class.
The most common one is in error_on_mismatching_quantization_info.
This must be handled in QuantizationInfo's operator== overload function. If all of the scales and offsets vectors have less than two elements we should check uniform quantization equality. If not, we can continue to check strict equality like we do now.
Of course, tests would need to be added for QuantizationInfo class and we can remove the changes here and in Pool2d tests.
There was a problem hiding this comment.
Very good points, I agree. Would you like me to fix this issues in this PR ? Alternatively, I'll create a new JIRA and follow up with a PR addressing those problems.
There was a problem hiding this comment.
Addressed in latest patchset, please have a look
Update QuantizationInfo::operator== to compare normalized uniform() values when both operands are effectively per-layer, while preserving strict vector equality for per-channel quantization info. This makes equivalent qinfo such as QuantizationInfo(1.0f) and QuantizationInfo(1.0f, 0) compare equal, avoiding incorrect dispatch differences for paths that rely on QuantizationInfo equality. Add UNIT coverage for equivalent uniform qinfo, differing uniform qinfo, and per-channel equality behavior, and remove the pooling-specific regression coverage. Signed-off-by: Pablo Marquez Tello <pablo.tello@arm.com> Change-Id: I54ab2cc9b2fae810b9d2767676097858eb8621c8
morgolock
force-pushed
the
pr/fix_pool_layer_perf
branch
from
6c1271a to
338544f
Compare
gunes-arm
left a comment
gunes-arm
left a comment
There was a problem hiding this comment.
fix: treat equivalent uniform QuantizationInfo as equal
Please start with capital letter, i.e. "fix: Treat ..."
Also, please change the PR title and description accordingly.
|
|
||
| TEST_CASE(EquivalentUniformImplicitAndExplicitZeroOffset, framework::DatasetMode::ALL) | ||
| { | ||
| const arm_compute::QuantizationInfo implicit_zero(1.0f); |
There was a problem hiding this comment.
We don't need arm_compute:: prefix (there is more below)
|
|
||
| #include "tests/framework/Asserts.h" | ||
| #include "tests/framework/Macros.h" | ||
|
|
| const auto src_qinfo = src->quantization_info().uniform(); | ||
| const auto dst_qinfo = dst->quantization_info().uniform(); | ||
|
|
||
| if (src_qinfo != dst_qinfo) |
There was a problem hiding this comment.
I think we don't need to touch Pool2d anymore.
2 participants
CpuPool2dAssemblyWrapperKernel::configure() decided whether to use the requantized pooling path by comparing the raw QuantizationInfo objects, while validate() compared their normalized uniform() values.
That made numerically equivalent qinfo such as QuantizationInfo(1.0f) and QuantizationInfo(1.0f, 0) take the slower differing-qinfo pooling path even though they represent the same scale and zero point.
Use normalized qinfo equality for pooling dispatch as well, and add NEON pooling coverage for the equivalent-qinfo NHWC QASYMM8 padded MAX case.
Change-Id: I54ab2cc9b2fae810b9d2767676097858eb8621c8