[4/10] Code generation for Conv2D via CMSIS-NN #9331

ashutosh-arm · 2021-10-20T16:08:35Z

This PR is for support of Conv2D via CMSIS-NN.

Here is the tracking issue: #8646

ashutosh-arm · 2021-10-20T16:09:18Z

Adding @Mousius @manupa-arm @grant-arm @areusch for review.

python/tvm/relay/op/contrib/cmsisnn.py

src/relay/backend/contrib/cmsisnn/extract_constants.cc

src/relay/backend/contrib/cmsisnn/generate_constants.cc

src/relay/backend/contrib/cmsisnn/relay_to_tir.cc

tests/python/contrib/test_cmsisnn/test_conv2d.py

tests/python/contrib/test_cmsisnn/test_networks.py

src/relay/backend/contrib/cmsisnn/extract_constants.cc

src/relay/backend/contrib/cmsisnn/generate_constants.cc

manupak

Hi @ashutosh-arm

Thanks for the work! It seems not trivial compared to other operators.
I did a pass. PTAL.

python/tvm/relay/op/contrib/cmsisnn.py

src/relay/backend/contrib/cmsisnn/extract_constants.cc

src/relay/backend/contrib/cmsisnn/generate_constants.cc

manupak

Broadly looks good! Thanks @ashutosh-arm.
Just a few questions.

manupak · 2021-11-01T18:14:30Z

src/relay/backend/contrib/cmsisnn/extract_constants.cc

+
+    // Since the constants are kicked out of the local partitioned functions
+    // a new call to local function is needed
+    if (auto* func_node = call->op.as<FunctionNode>()) {


python/tvm/relay/op/contrib/cmsisnn.py

src/relay/backend/contrib/cmsisnn/generate_constants.cc

manupak · 2021-11-04T10:08:52Z

src/relay/backend/contrib/cmsisnn/relay_to_tir.cc

    primfunc_ = tir::PrimFunc(func_signature, body, VoidType(), Map<tir::Var, tir::Buffer>(),
                              DictAttrs(dict_attrs));
  }

+  Array<PrimExpr> CMSISNNDimensions(const Array<PrimExpr>& shape) {
+    ICHECK(shape.size() == 4) << "Supports only CMSIS-NN shapes of dimension 4.";


Do we need to test these or not ? Im not sure now. @Mousius ?

manupak · 2021-11-05T15:49:01Z

@ashutosh-arm could you retrigger this one? the docker image is updated now.

Mousius · 2021-11-10T13:53:48Z

@manupa-arm / @ashutosh-arm in general if we're adding things that raise exceptions as part of their interface we should be testing them.

manupak · 2021-11-11T08:57:11Z

@manupa-arm / @ashutosh-arm in general if we're adding things that raise exceptions as part of their interface we should be testing them.

I agree for user facing exceptions. What about internal assertions that are not meant to be triggered in a user execution flow and are only meant to be triggered when a "developer" is changing the compilation stack ?

Generally these assertions are derived from assumptions that a user flow will never trigger them and usually are about inputs to a particularly component.

Having a test that does another assertion to check an assertion on input is working does only seem to give confidence that the function call is successfully able to pass in an argument to trigger the assertion. Ideally, a release build should remove these internal assertions as they were only dev facing assertions.

Let me know if you still think it is worthwhile adding a test check an dev facing assertion that gives us confidence that a component is successfully able to pass in an input that violate an assumption that the developer took that a user flow will never trigger. If so we might want to check the assertion in this PR (here or in a follow up)

Mousius · 2021-11-11T09:54:32Z

Generally these assertions are derived from assumptions that a user flow will never trigger them and usually are about inputs to a particularly component.

I would question why we're adding them if they're in unreachable code paths, but if we do think they're worth adding I would consider them part of the contract of the internal function and thus should have associated tests for the behaviour (throwing an exception is now part of the functions behaviour).

As per TVM Code Review Guidelines, we should add test coverage for anything we contribute. If we're happy that at some point someone may inadvertently change the behaviour of this function, then potentially it's not a necessary line of code and we can apply the YAGNI principle from extreme programming 😸

manupak · 2021-11-11T10:29:33Z

I would question why we're adding them if they're in unreachable code paths, but if we do think they're worth adding I would consider them part of the contract of the internal function and thus should have associated tests for the behaviour (throwing an exception is now part of the functions behaviour).

There is a fine line between defining a failing internal assert as a "behaviour" of the function and removing the assert just because it not executed in the present organization of the components.

If we're happy that at some point someone may inadvertently change the behaviour of this function, then potentially it's not a necessary line of code and we can apply the YAGNI principle from extreme programming

Enforcing YAGNI to remove internal asserts will discourage developers from using asserts just because the execution path is not enforced today.

In a summary I would support having internal assertions that makes it easier if another developer changes this code or re-uses some of it somewhere else. If it means we need a add a borderline meaningful test that uses as an assertion to see if the assertion inside the code is triggered, lets do that then.

I guess what you are saying is we need a test for the above ICHECK in this PR?

Mousius · 2021-11-11T10:39:32Z

If it means we need a add a borderline meaningful test that uses as an assertion to see if the assertion inside the code is triggered, lets do that then.

It doesn't, you're the committer responsible for this PR, if you genuinely disagree feel free to merge without test coverage - this has happened on other PRs I've reviewed.

I guess what you are saying is we need a test for the above ICHECK in this PR?

Yip, I would advocate for full test coverage in line with the TVM Code Review Guidelines, working on the assumption we want the code to continue working for the forsee-able future. Though I don't think we should turn this PR into a debate on software testing - that's been going for a long time now 😸 .

manupak · 2021-11-11T10:57:09Z

if you genuinely disagree feel free to merge without test coverage

My position is to keep the assert (as originally added by @ashutosh-arm -- not a request for a change). I only oppose removing it on the basis it is not tested. Im happy to do it in a follow up if thats what @ashutosh-arm wants to do here ?

his has happened on other PRs I've reviewed.

Hence the questioning here, because this exact point is questioned elsewhere but not here. Other places we committed to follow up if that is direction we want to take. cc : @ekalda @mbaret. Im pretty sure if we agree on this point we will do the necessary changes.

working on the assumption we want the code to continue working for the forsee-able future

Im pretty sure we all want this -- just being lenient on internal asserts -- that is all.

ashutosh-arm · 2021-11-11T11:21:18Z

In my view, I don't wish to remove the assert as this is a code that could be reused and fail if caller does not meet asserted criterion. At the same time, testing all internal asserts should not be made into a policy. I say this because at certain times, we know that the previous flow, before this stage is reached, assumes certain things and has its own set of asserts.

On another note, this cannot be tested in the existing CMSIS-NN testing infrastructure as there is no direct interface to access it. How do we test such APIs currently? Can someone please point me to an example?

ekalda · 2021-11-11T11:53:56Z

if you genuinely disagree feel free to merge without test coverage

My position is to keep the assert (as originally added by @ashutosh-arm -- not a request for a change). I only oppose removing it on the basis it is not tested. Im happy to do it in a follow up if thats what @ashutosh-arm wants to do here ?

his has happened on other PRs I've reviewed.

Hence the questioning here, because this exact point is questioned elsewhere but not here. Other places we committed to follow up if that is direction we want to take. cc : @ekalda @mbaret. Im pretty sure if we agree on this point we will do the necessary changes.

working on the assumption we want the code to continue working for the forsee-able future

Im pretty sure we all want this -- just being lenient on internal asserts -- that is all.

Yes, I think we agreed to add the tests in a follow-up PR if we decide to test the internal asserts in the future, so it would make sense for this PR to go in as it is as well.

My (unchanged) view on the topic is that I agree with @manupa-arm that we should test the user facing asserts, but not the simple developer facing asserts. While it is a good practice to test all the functionality, I think we should always evaluate these kind of guidelines in the context of the specific project if we want to avoid getting stuck in dogmas. It is a valid point that perhaps we should remove the unreachable asserts then, but I think TVM is very much still a project in progress and I have found the developer facing asserts very useful, so I don't think they do any harm. In general, the NPU side of codebase is in constant flux at the moment, so a lot of tests means a lot of extra code to maintain and makes making changes harder. So at that stage I think there is more harm in testing ICHECKs than benefits in a from maintenance overhead.

manupak · 2021-11-11T12:20:18Z

Alright thanks for the views! Lets not block PR on this -- it is certainly a high bandwidth discussion to be had.
I think this PR can go in and @ashutosh-arm would you be able to fix the conflicts ?-- so we could merge this.

ashutosh-arm · 2021-11-11T13:12:49Z

I am working on resolving the conflicts.

Change-Id: Ifa9ee3bb0b5292ba9911f9c4fc2af508957cd784

Co-authored-by: Chris Sidebottom <chris.sidebottom@arm.com> Change-Id: I15e7e7e4fa864ddc469a4e430c4f472057290e8a

Change-Id: Ie4c6f27318978d69aec9131d79f9a3f28c99f3b0

Change-Id: I47b64db6a14732420952aad58488438ac80b7816

manupak · 2021-11-17T09:53:30Z

Thanks @ashutosh-arm @Mousius @areusch!

This PR is for support of Conv2D via CMSIS-NN.

ashutosh-arm requested review from anijain2305, areusch, comaniac, jroesch, junrushao, jwfromm, manupak, MarisaKirisame, mbaret, mbrookhart, merrymercy, slyubomirsky, tqchen, trevor-m, vinx13, wweic, yzhliu, zhiics and ZihengJiang as code owners October 20, 2021 16:08

ashutosh-arm mentioned this pull request Oct 20, 2021

[RFC][Tracking Issue] CMSIS-NN Integration #8646

Closed

Mousius requested changes Oct 20, 2021

View reviewed changes

src/relay/backend/contrib/cmsisnn/extract_constants.cc Outdated Show resolved Hide resolved

src/relay/backend/contrib/cmsisnn/generate_constants.cc Outdated Show resolved Hide resolved

Mousius mentioned this pull request Oct 21, 2021

[4a/10] [CMSIS-NN] Calculate CMSIS-NN buffer size with respect to architecture extensions #9338

Merged

manupak requested changes Oct 22, 2021

View reviewed changes

ashutosh-arm mentioned this pull request Nov 1, 2021

[5/10] Code generation for Depthwise Convolution via CMSIS-NN #9409

Merged

manupak reviewed Nov 4, 2021

View reviewed changes

ashutosh-arm mentioned this pull request Nov 5, 2021

[6/10] Code generation for fully connected layer via CMSIS-NN #9456

Closed

ashutosh-arm force-pushed the conv2d branch from 49847bd to 864fc25 Compare November 5, 2021 16:10

ashutosh-arm added 4 commits November 15, 2021 10:35

code review: tests for new passes, clean up of relay_to_tir for cmsis-nn

48299f9

context buffer to be passed via tir primfunc arg

8de73b7

Change-Id: Ifa9ee3bb0b5292ba9911f9c4fc2af508957cd784

Include primfuncs in update metadata

cdebe57

Co-authored-by: Chris Sidebottom <chris.sidebottom@arm.com> Change-Id: I15e7e7e4fa864ddc469a4e430c4f472057290e8a

Some imports were failing from composite_target

1782b14

Change-Id: Ie4c6f27318978d69aec9131d79f9a3f28c99f3b0

ashutosh-arm force-pushed the conv2d branch from d17daf5 to 1782b14 Compare November 16, 2021 15:41

ashutosh-arm requested review from kparzysz-quic, liangfu, masahi and tmoreau89 as code owners November 16, 2021 15:41

Fixed linting issues

5f70418

Change-Id: I47b64db6a14732420952aad58488438ac80b7816

manupak merged commit ee23013 into apache:main Nov 17, 2021

mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021

[4/10] Code generation for Conv2D via CMSIS-NN (apache#9331)

75b3353

This PR is for support of Conv2D via CMSIS-NN.

mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021

[4/10] Code generation for Conv2D via CMSIS-NN (apache#9331)

871f575

This PR is for support of Conv2D via CMSIS-NN.

ashutosh-arm deleted the conv2d branch December 7, 2021 10:06

ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022

[4/10] Code generation for Conv2D via CMSIS-NN (apache#9331)

657aaa3

This PR is for support of Conv2D via CMSIS-NN.

yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022

[4/10] Code generation for Conv2D via CMSIS-NN (apache#9331)

8c5cdb5

This PR is for support of Conv2D via CMSIS-NN.

ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022

[4/10] Code generation for Conv2D via CMSIS-NN (apache#9331)

a00d3d4

This PR is for support of Conv2D via CMSIS-NN.

Mousius mentioned this pull request Apr 13, 2022

[AutoTVM] Use new target object #10963

Closed

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4/10] Code generation for Conv2D via CMSIS-NN #9331

[4/10] Code generation for Conv2D via CMSIS-NN #9331

ashutosh-arm commented Oct 20, 2021

ashutosh-arm commented Oct 20, 2021

manupak left a comment

manupak left a comment

manupak Nov 1, 2021

manupak Nov 4, 2021

manupak commented Nov 5, 2021

Mousius commented Nov 10, 2021

manupak commented Nov 11, 2021

Mousius commented Nov 11, 2021

manupak commented Nov 11, 2021

Mousius commented Nov 11, 2021

manupak commented Nov 11, 2021

ashutosh-arm commented Nov 11, 2021

ekalda commented Nov 11, 2021

manupak commented Nov 11, 2021

ashutosh-arm commented Nov 11, 2021

manupak commented Nov 17, 2021

[4/10] Code generation for Conv2D via CMSIS-NN #9331

[4/10] Code generation for Conv2D via CMSIS-NN #9331

Conversation

ashutosh-arm commented Oct 20, 2021

ashutosh-arm commented Oct 20, 2021

manupak left a comment

Choose a reason for hiding this comment

manupak left a comment

Choose a reason for hiding this comment

manupak Nov 1, 2021

Choose a reason for hiding this comment

manupak Nov 4, 2021

Choose a reason for hiding this comment

manupak commented Nov 5, 2021

Mousius commented Nov 10, 2021

manupak commented Nov 11, 2021

Mousius commented Nov 11, 2021

manupak commented Nov 11, 2021

Mousius commented Nov 11, 2021

manupak commented Nov 11, 2021

ashutosh-arm commented Nov 11, 2021

ekalda commented Nov 11, 2021

manupak commented Nov 11, 2021

ashutosh-arm commented Nov 11, 2021

manupak commented Nov 17, 2021