[CMSIS-NN] Fix typo in EmitPool2D

[PhilippvK]

This bug, caused that no context_buffers are generated for AvgPool2D function calls breaking the following configuration:

• flags.dsp=1
• flags.mvei=0

cc @areusch

[PhilippvK]

@Mousius @ashutosh-arm @manupa-arm

[asparkhi]

Thanks @PhilippvK for the fix.

Could you please check why the existing test does not catch this?

tvm/src/relay/backend/contrib/cmsisnn/buffer_size.cc

Line 74 in 27b0aad

int AvgPoolBufferSize(CMSISNNFlags flags, int32_t input_c) {

If you case is different, then please add a new unit test.

[PhilippvK]

@ashutosh-arm AFAIK there currently exist no test for the RelayToTIRVisitor itself. The routine AvgPoolBufferSize is correctly implemented but never invoked due to a typo in the if-statement in relay_to_tir.cc.

If we want to catch that sort of bug in the future, we need to write Tests for the individual Emit* functions.

A test would probably have failed if cortex-m33(DSP only) instead of cortex-m55 (MVEI+DSP) would have been used in integrations tests with CMSIS-NN (Assuming that an avg_pool2d is part of a used model). However I am not sure in which degree cmsisnn is currently involved in integration tests.

[asparkhi]

@PhilippvK You are correct there are no direct tests for RelayToTIRVisitor.

I get it why the buffer checks pass in this case. Thanks!

Since we don't test execution for anything other than Cortex-M55 at the moment, we should probably use this API to pass on the correct cpu arch by preparing PassConfig first 🤔

[areusch]

@manupa-arm should we merge this and file a follow-on GH issue to investigate?

[PhilippvK]

@ashutosh-arm

Since we don't test execution for anything other than Cortex-M55 at the moment, we should probably use this API to pass on the correct cpu arch by preparing PassConfig first

While we could run the test for cortex-m55 with MVEI+DSP support, cortex-m33 with DSP and let's say cortex-m0 without special instructios using the mcpu pass config we also need adapt the Makefiles which are currently hardcoded for the cortex-m55.

@areusch As the fixed issue breaks CMSISNN BYOC support for models with pooling layers on DSP-only devices I would appreciate getting this merged to main. If anything is missing please let me know.

[manupak]

Can we not check with cortex-m55+nomve ? If so it just need to modify the test runner (maybe a new one) to cover this case.

Here :

tvm/python/tvm/micro/testing/aot_test_utils.py

Lines 36 to 47 in e7851ed

	AOT_CORSTONE300_RUNNER = AOTTestRunner(
	makefile="corstone300",
	prologue="""
	uart_init();
	""",
	includes=["uart.h"],
	pass_config={
	"relay.ext.cmsisnn.options": {
	"mcpu": "cortex-m55",
	}
	},
	)

Then adding passing mcpu value to here (similiar to NPU_VARIANT) :

https://github.com/apache/tvm/blob/main/tests/python/relay/aot/corstone300.mk#L41

(Happy to take as a follow up and this is what's needed to be addressed to close the issue -- if we deciding defer that change)

[asparkhi]

I think this PR can be merged now that we have upcoming support for passing +nomve flag around from test runner: #12132

[manupak]

@ashutosh-arm, can you approve the PR explicitly ?
(as a token of confirmation that this change will be tested in the follow up you mentioned)

[asparkhi]

These changes will automatically get tested with this support: #12132

[manupak]

LGTM

[manupak]

Thanks @PhilippvK @ashutosh-arm @areusch !