Arm(R) Ethos(TM)-U NPU Pooling operators support #9384
Conversation
NicolaLancellotti
requested review from
anijain2305,
areusch,
comaniac,
jroesch,
junrushao,
jwfromm,
MarisaKirisame,
mbrookhart,
merrymercy,
slyubomirsky,
tqchen,
vinx13,
wweic,
yzhliu,
zhiics and
ZihengJiang
as code owners
| if pooling_type == "MAX": | ||
| rewriter = legalize.MaxPoolingRewriter() | ||
| pattern_table = [ | ||
| ( | ||
| ethosu.MaxPool2DParams.composite_name, | ||
| ethosu.qnn_maxpool2d_pattern(), | ||
| lambda pat: ethosu.MaxPool2DParams(pat).is_valid(), | ||
| ), | ||
| ] | ||
| elif pooling_type == "AVG": | ||
| rewriter = legalize.AvgPoolingRewriter() | ||
| pattern_table = [ | ||
| ( | ||
| ethosu.AvgPool2DParams.composite_name, | ||
| ethosu.qnn_avgpool2d_pattern(), | ||
| lambda pat: ethosu.AvgPool2DParams(pat).is_valid(), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Rather than branching inside the test, it'd be clearer to have two separate tests which re-use the same helper functions.
There was a problem hiding this comment.
If it is not a problem, I'll do it in a following pr.
There was a problem hiding this comment.
I think that's a sensible thing to split out 😸
There was a problem hiding this comment.
Thinking about it, it should be ok to have only one test, that because MAX and AVG are the two possible values of the pooling_type attribute of the Ethos(TM)-U Pooling operator. We are not testing two different operators in one test, but different parameters of the same operator.
There was a problem hiding this comment.
Test cases aren't necessarily limited to being per operator, to get good coverage I usually find you need a lot of smaller tests rather than one large one - branches are then an indicator where you might need two small tests for the operator rather than one large one as the branch itself may need testing.
| if ifm_layout == "NHWC": | ||
| ifm_stride_c = 1 | ||
| ifm_stride_w = ifm_shape[3] | ||
| ifm_stride_h = ifm_shape[2] * ifm_shape[3] | ||
| ofm_height = (ifm_shape[1] - pool_shape[0] + padding[0] + padding[0]) // strides[0] + 1 | ||
| ofm_width = (ifm_shape[2] - pool_shape[1] + padding[1] + padding[1]) // strides[1] + 1 | ||
| else: | ||
| ifm_stride_w = 16 | ||
| ifm_stride_c = 16 * ifm_shape[3] | ||
| ifm_stride_h = 16 * ifm_shape[2] * ifm_shape[3] | ||
| ofm_height = (ifm_shape[1] - pool_shape[0] + padding[0] + padding[0]) // strides[0] + 1 | ||
| ofm_width = (ifm_shape[3] - pool_shape[1] + padding[1] + padding[1]) // strides[1] + 1 | ||
|
|
||
| if ofm_layout == "NHWC": | ||
| ofm_stride_c = 1 | ||
| ofm_stride_w = ofm_channels if ofm_width > 1 else 1 | ||
| ofm_stride_h = ofm_channels * ofm_width if ofm_height > 1 else 1 | ||
| else: | ||
| ofm_stride_w = 16 | ||
| ofm_stride_c = 16 * ofm_width | ||
| ofm_stride_h = 16 * ofm_width * ((ofm_channels - 1) // 16 + 1) |
There was a problem hiding this comment.
There's a few branches here, it might be worth having a test for each of these cases rather than branching within the test themselves.
There was a problem hiding this comment.
The tests for this and the other operators are quite verbose, and we have relied heavily on pytest.mark.parametrize to reduce the number of tests.
Unfortunately, not all things can be parametrised, so some ifs are necessary.
If we had to write multiple tests there would be a lot of code duplication.
What do other people think?
There was a problem hiding this comment.
It seems like if we wanted to get rid of these if/else statements, we would need a separate test case for each combination of ifm layout and ofm layout, so that would be 4 tests. I agree that there would be quite a bit of code duplication and I don't mind the ifs much myself, but if others find it clearer to have separate tests, I'd be fine with it :)
There was a problem hiding this comment.
A following pr will refactor this and the other operators, which are currently using the same structure.
There was a problem hiding this comment.
We should be able to create functions to compose the tests together rather than implementing it all as one test script - which should mean far less duplication but still getting the multiple test cases and letting pytest parametrize along the various axis we're interested in.
There was a problem hiding this comment.
Thanks @NicolaLancellotti, LGTM overall! Just a few small things I picked up on
python/tvm/relay/backend/contrib/ethosu/tir_to_cs_translator.py
Outdated
Show resolved
Hide resolved
| if ifm_layout == "NHWC": | ||
| ifm_stride_c = 1 | ||
| ifm_stride_w = ifm_shape[3] | ||
| ifm_stride_h = ifm_shape[2] * ifm_shape[3] | ||
| ofm_height = (ifm_shape[1] - pool_shape[0] + padding[0] + padding[0]) // strides[0] + 1 | ||
| ofm_width = (ifm_shape[2] - pool_shape[1] + padding[1] + padding[1]) // strides[1] + 1 | ||
| else: | ||
| ifm_stride_w = 16 | ||
| ifm_stride_c = 16 * ifm_shape[3] | ||
| ifm_stride_h = 16 * ifm_shape[2] * ifm_shape[3] | ||
| ofm_height = (ifm_shape[1] - pool_shape[0] + padding[0] + padding[0]) // strides[0] + 1 | ||
| ofm_width = (ifm_shape[3] - pool_shape[1] + padding[1] + padding[1]) // strides[1] + 1 | ||
|
|
||
| if ofm_layout == "NHWC": | ||
| ofm_stride_c = 1 | ||
| ofm_stride_w = ofm_channels if ofm_width > 1 else 1 | ||
| ofm_stride_h = ofm_channels * ofm_width if ofm_height > 1 else 1 | ||
| else: | ||
| ofm_stride_w = 16 | ||
| ofm_stride_c = 16 * ofm_width | ||
| ofm_stride_h = 16 * ofm_width * ((ofm_channels - 1) // 16 + 1) |
There was a problem hiding this comment.
It seems like if we wanted to get rid of these if/else statements, we would need a separate test case for each combination of ifm layout and ofm layout, so that would be 4 tests. I agree that there would be quite a bit of code duplication and I don't mind the ifs much myself, but if others find it clearer to have separate tests, I'd be fine with it :)
NicolaLancellotti
force-pushed
the
ethosu/pooling
branch
from
1b03875
to
4950a17
Compare
There was a problem hiding this comment.
Thanks for the PR. @NicolaLancellotti
To get this PR moved forward,
I would suggest adding tests for TypeReporter errors and lets follow up tests for other ICHECKs in P10 here : #9410.
@Mousius does that sound good ?
| const auto* param = attrs.as<EthosuPoolingAttrs>(); | ||
| ICHECK(param != nullptr) << "EthosuPoolingAttrs cannot be nullptr."; | ||
|
|
||
| ICHECK(param->pooling_type == "AVG" || param->pooling_type == "MAX") |
There was a problem hiding this comment.
I think this one could use the TypeReporter and possibly with a test.
| ICHECK(param->pooling_type == "AVG" || param->pooling_type == "MAX") | ||
| << "Expected pooling_type 'AVG' or 'MAX' but was" << param->pooling_type; | ||
|
|
||
| ICHECK(ifm->dtype == DataType::UInt(8) || ifm->dtype == DataType::Int(8)) |
There was a problem hiding this comment.
I think this one could use the TypeReporter and possibly with a test.
One thing I'd suggest we don't move to another PR is adding tests for the other activation functions, as I think that's core to the functionality introduced here. Though given this has a green tick, if it's a relatively immediate follow up then that'd actually be fine. |
There was a problem hiding this comment.
Thanks for addressing my comments @NicolaLancellotti, LGTM modulo others outstanding comments
As I mentioned in this comment, it is a bit unclear to me in what do you mean by testing other activation functions? Would you mind giving an example? |
Change-Id: I252ac10d6adfa1fbd538eff951fab0235a72de4d
NicolaLancellotti
force-pushed
the
ethosu/pooling
branch
from
4950a17
to
f703eeb
Compare
|
Could we add to the description that this includes a few minor rewording changes? This is just to be explicit to anyone scanning the commit history that this change doesn't solely add pooling. |
There was a problem hiding this comment.
LGTM.
I believe @Mousius concern of SerialActivation is addressed. @NicolaLancellotti please follow up with agreed refactors in a follow up PR.
This commit adds support for the max and average pooling primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the max and average pooling primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the max and average pooling primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the max and average pooling primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.