[microTVM] Improve code reuse in Corstone300 conv2d tests #13051

guberti · 2022-10-12T09:23:07Z

This pull request contains two changes:

Adds support for specifying an out_layout to the Tensordot-based convolution schedules for Arm Cortex-M. This is useful, as when compiling models like MobileNetV1, we will want to alternate between NHWC and NCHW layers (see this comment for more).
Re-organizes the Corstone300 tests for regular, grouped, and depthwise conv2d to use a shared base class. This removes a lot of code re-use that used to exist in tests/python/relay/strategy/arm_cpu.

I chose to make these changes as one PR instead of two, since I had to change some of the testing logic to support the out_layout parameter (and copying that into four places would be kinda gross). Would love a look from @mkatanbaf!

tvm-bot · 2022-10-12T09:23:10Z

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

cc @alanmacd, @gromero, @mehrdadh _{See #10317 for details}
Built docs for commit 99cb58c can be found here.

_{Generated by tvm-bot}

guberti · 2022-10-12T10:07:08Z

When reorganizing the tests, I noticed there was duplicate code in tests besides the conv2d ones. Each file has its own calls to relay.op.nn.[operator], which are constructed the same way and then passed to compile_and_run.

Would it make sense to create an even-more-abstract class that handled graph creation and running for all the tests in this directory, not just the conv2d variants?
I would prefer to test our schedules by comparing them to external operator implementations - say, the Tensorflow ones - rather than by comparing them to our x86 schedules. It would be much simpler to do this as well.

mkatanbaf

Thanks @guberti. LGTM. I added a few minor comments

mkatanbaf · 2022-10-13T02:28:14Z

tests/python/relay/strategy/arm_cpu/test_depthwise_conv2d.py

-    kernel_layout = tvm.testing.parameter("HWOI")
-    schedule_name = tvm.testing.parameter("depthwise_conv2d_nhwc_dsp.arm_cpu")
+    dilation = parameter(1)
+    data_layout = parameter("NHWC")


the data_shape is always in NHWC layout. should we rename the data_layout to in_layout to avoid any confusion?

I'd prefer to keep the name data_shape, as it's the name used by relay.op.nn.conv2d. However, I added a comment clarifying this to test_generalized_conv2d.py.

mkatanbaf · 2022-10-13T02:29:10Z

tests/python/relay/strategy/arm_cpu/test_depthwise_conv2d.py

-    kernel_layout = tvm.testing.parameter("OIHW")
-    schedule_name = tvm.testing.parameter("depthwise_conv2d_nchw_oihw_dsp.arm_cpu")
+    dilation = parameter(1)
+


nit: remove new line. there are a couple more in other files too.

I prefer the newline to separate "computation" parameters from "layout" parameters, and IMO it helps with readability. I could be convinced otherwise, though.

tests/python/relay/strategy/arm_cpu/test_depthwise_conv2d.py

mkatanbaf · 2022-10-13T03:12:22Z

* Would it make sense to create an even-more-abstract class that handled graph creation and running for all the tests in this directory, not just the `conv2d` variants?

When I was working on adding corstone to the platforms supported by the project API, we talked about migrating from this test structure and use Project API for these tests too. But then the fvp transport layer was not as stable as we have hoped for, and we didn't continue that work at that time. One argument in favor of creating a more abstract class is that it makes it easier to change the test structure.

guberti · 2022-10-13T10:40:14Z

Thanks for the review @mkatanbaf! I'm a big fan of the current test runner and of the AOTTestModule and compile_and_run helper functions - they've proven to be stable, and when running the tests locally they're relatively fast too.

It is possible we change the test structure in the future though - your argument makes sense. Creating a more abstract class is probably OOS for this PR, but I'd be open to it in the future.

mehrdadh · 2022-10-14T00:29:09Z

@guberti the problem with current test setup is that we cannot test it on hardware out of the box. So ideally we want to move to project API for corstone so we have the same standard API for testing Zephyr/Arduino/Corstone300 or any other microtvm platform with ARM chip.
This is however out of the scope of this PR. But I suggest we don't create another abstract test class until we figure out the issue with corstone project API

guberti · 2022-10-14T08:13:39Z

@mehrdadh thanks for explaining, that makes sense. Tests should pass now, would you mind reviewing and merging?

mkatanbaf

Thanks @guberti, LGTM

python/tvm/topi/utils.py

tests/python/relay/strategy/arm_cpu/test_generalized_conv2d.py

areusch · 2022-10-18T13:44:48Z

tests/python/relay/strategy/arm_cpu/test_generalized_conv2d.py

+        # Our x86 depthwise implementation only supports HWOI with NHWC, so we need to change our
+        # kernel layout to work around this. We can't just change the whole thing to HWIO or
+        # something else, as then group conv2d would not work. Eventually, we should switch to using
+        # TensorFlow to create the reference output so we can ensure our implementation is right.


can you file a bug for this and link to it from this comment?

Done, see #13137. I also link to it from the comment.

tests/python/relay/strategy/arm_cpu/test_group_conv2d.py

guberti · 2022-10-19T15:38:30Z

Thanks for the review @areusch! Your comments should be addressed by fed35c2.

mehrdadh

@guberti @mkatanbaf @areusch thanks!
This PR is merged now.

* Add support for out_layout to tensordot schedules and tests * Move shared conv2d test logic into new file * Rework depthwise and grouped convolutions to use common logic * Linting and bugfixes * Fix tests * Fix depthwise and grouped tests * More linting fixes * Address code review comments * Fix unit tests * Address code review comments from Andrew * Fix imports

mkatanbaf reviewed Oct 13, 2022

View reviewed changes

guberti mentioned this pull request Oct 17, 2022

[HotFix] Fix python import #13099

Merged

guberti force-pushed the micro/improve-corstone-tests branch from ee6096c to 2ffad17 Compare October 17, 2022 16:37

mkatanbaf approved these changes Oct 17, 2022

View reviewed changes

areusch reviewed Oct 18, 2022

View reviewed changes

areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022

guberti added 9 commits October 19, 2022 08:29

Add support for out_layout to tensordot schedules and tests

e8cd925

Move shared conv2d test logic into new file

1516b84

Rework depthwise and grouped convolutions to use common logic

a69331f

Linting and bugfixes

52993e5

Fix tests

821a483

Fix depthwise and grouped tests

7ce9888

More linting fixes

24d66f3

Address code review comments

a3b53c2

Fix unit tests

22bc3c8

guberti force-pushed the micro/improve-corstone-tests branch from 51c96eb to 6a12eca Compare October 19, 2022 15:30

Address code review comments from Andrew

fed35c2

guberti force-pushed the micro/improve-corstone-tests branch from 6a12eca to fed35c2 Compare October 19, 2022 15:32

Fix imports

99cb58c

mehrdadh approved these changes Oct 21, 2022

View reviewed changes

mehrdadh merged commit e7a72af into apache:main Oct 21, 2022

leandron mentioned this pull request Feb 1, 2023

TVM v0.11.0 Release Candidate Notes #13899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[microTVM] Improve code reuse in Corstone300 conv2d tests #13051

[microTVM] Improve code reuse in Corstone300 conv2d tests #13051

guberti commented Oct 12, 2022

tvm-bot commented Oct 12, 2022 •

edited

Loading

guberti commented Oct 12, 2022

mkatanbaf left a comment

mkatanbaf Oct 13, 2022

guberti Oct 13, 2022

mkatanbaf Oct 13, 2022

guberti Oct 13, 2022

mkatanbaf commented Oct 13, 2022

guberti commented Oct 13, 2022

mehrdadh commented Oct 14, 2022 •

edited

Loading

guberti commented Oct 14, 2022

mkatanbaf left a comment

areusch Oct 18, 2022

guberti Oct 19, 2022 •

edited

Loading

guberti commented Oct 19, 2022

mehrdadh left a comment •

edited

Loading

[microTVM] Improve code reuse in Corstone300 conv2d tests #13051

[microTVM] Improve code reuse in Corstone300 conv2d tests #13051

Conversation

guberti commented Oct 12, 2022

tvm-bot commented Oct 12, 2022 • edited Loading

guberti commented Oct 12, 2022

mkatanbaf left a comment

Choose a reason for hiding this comment

mkatanbaf Oct 13, 2022

Choose a reason for hiding this comment

guberti Oct 13, 2022

Choose a reason for hiding this comment

mkatanbaf Oct 13, 2022

Choose a reason for hiding this comment

guberti Oct 13, 2022

Choose a reason for hiding this comment

mkatanbaf commented Oct 13, 2022

guberti commented Oct 13, 2022

mehrdadh commented Oct 14, 2022 • edited Loading

guberti commented Oct 14, 2022

mkatanbaf left a comment

Choose a reason for hiding this comment

areusch Oct 18, 2022

Choose a reason for hiding this comment

guberti Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

guberti commented Oct 19, 2022

mehrdadh left a comment • edited Loading

Choose a reason for hiding this comment

tvm-bot commented Oct 12, 2022 •

edited

Loading

mehrdadh commented Oct 14, 2022 •

edited

Loading

guberti Oct 19, 2022 •

edited

Loading

mehrdadh left a comment •

edited

Loading