fix: Enable assembly kernel for int8 MAX pooling.#1278
fix: Enable assembly kernel for int8 MAX pooling.#1278
Conversation
Change-Id: I96cdf94dc29348a01b8f47aba1b6afd6d82fbcb9 Signed-off-by: Pablo Marquez Tello <pablo.tello@arm.com>
gunes-arm
left a comment
gunes-arm
left a comment
There was a problem hiding this comment.
Regarding the commit message, I think we should add the tickets and the GitHub issue number.
Question to you: Is this a fix or something else? Looks like a performance improvement to me.
| ARM_COMPUTE_RETURN_ERROR_ON_MSG( | ||
| !info.exclude_padding && has_padding, | ||
| "Assembly kernels do not support padding for QASYMM8 with same src/dst quantization info"); | ||
| info.pool_type == PoolingType::AVG && !info.exclude_padding && has_padding, |
There was a problem hiding this comment.
I think checking for != MAX is easier. This sentence alone implies L2 pooling gets along well with paddings. So, checking for types that we know they're supported conveys the message more clearly.
| } | ||
| else | ||
| { | ||
| if (src->data_type() == DataType::QASYMM8) |
There was a problem hiding this comment.
I think the speciality around QASYMM8 is that the minimum value is 0 and the initial value is set to this. So, !exclude_padding is not supported when the type is QASYMM8_SIGNED as the minimum value must be -128. I wonder if it's an easy piece of code that we can change to support both types. Given that it's only QASYMM8 for now, we should state it in the commit message as is, not as Int8.
| int32_t dst_multiplier{}; | ||
| int32_t dst_shift{}; | ||
| ARM_COMPUTE_RETURN_ERROR_ON( | ||
| quantization::calculate_quantized_multiplier(multiplier, &dst_multiplier, &dst_shift)); |
There was a problem hiding this comment.
This part is wildly weird because it looks like we don't check for all the padding related issues if the quantization informations are different. What is going on here?
There was a problem hiding this comment.
I don't think this is the right place to add the test. tests/validation/NEON/PoolingLayer.cpp is the correct place. I'd not call it as a Smoke test as well. It's a proper test imo.
morgolock
force-pushed
the
pr/enable_asm_int8_max_pool
branch
from
48ff668 to
52cc9bf
Compare
2 participants
Change-Id: I96cdf94dc29348a01b8f47aba1b6afd6d82fbcb9