fix: Enable assembly kernel for int8 MAX pooling.#1278
fix: Enable assembly kernel for int8 MAX pooling.#1278
Conversation
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.
| } | ||
| 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.
There was a problem hiding this comment.
Updated the commit msg to mention just QASYMM8 for now.
I'll look into adding support for QASYMM8_SIGNED in a different patch to keep this change small.
There was a problem hiding this comment.
Can you also update the PR title and description? I also have another comment regarding the commit category, which is around the top of the main conversation panel.
There was a problem hiding this comment.
I created the tickets below for the additional investigation work:
- MLCE-1821
- MLCE-1822
I'll update the commit msg and pr title to perf: Enable assembly kernel for QASYMM8 MAX pooling
There was a problem hiding this comment.
PR title is still the same, isn't it?
morgolock
force-pushed
the
pr/enable_asm_int8_max_pool
branch
from
48ff668 to
52cc9bf
Compare
morgolock
force-pushed
the
pr/enable_asm_int8_max_pool
branch
from
52cc9bf to
f43873f
Compare
Change-Id: I96cdf94dc29348a01b8f47aba1b6afd6d82fbcb9 Signed-off-by: Pablo Marquez Tello <pablo.tello@arm.com>
morgolock
force-pushed
the
pr/enable_asm_int8_max_pool
branch
from
f43873f to
ada1be8
Compare
| !info.exclude_padding && has_padding, | ||
| "Assembly kernels do not support padding for QASYMM8 with same src/dst quantization info"); | ||
| ARM_COMPUTE_RETURN_ERROR_ON_MSG(info.pool_type != PoolingType::MAX && !info.exclude_padding && has_padding, | ||
| "Assembly kernels only support padded AVG pooling for QASYMM8 with " |
There was a problem hiding this comment.
The comment should probably slightly change to reflect the != MAX check. Such as
Assembly kernels only support MAX pooling if there is padding and exclude_padding is false.
2 participants
Change-Id: I96cdf94dc29348a01b8f47aba1b6afd6d82fbcb9