Skip to content

Conversation

@asparkhi
Copy link
Contributor

@asparkhi asparkhi commented Aug 3, 2021

This RFC details integration of CMSIS-NN API into TVM to support code generation for Arm(R) Cortex-M using the following library: https://github.com/ARM-software/CMSIS_5/tree/develop/CMSIS/NN

cc : @areusch @mbaret @tqchen @Mousius @manupa-arm

@u99127
Copy link

u99127 commented Aug 3, 2021

Minor nit - the title of the RFC should really read - [RFC] Use CMSIS-NN with TVM.

@manupa-arm , @Mousius ..

@asparkhi asparkhi changed the title [RFC] Markdown for CMSIS-NN integration [RFC] Use CMSIS-NN with TVM Aug 4, 2021
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits here and there. Generally looks very good.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of other acronyms.

@tqchen
Copy link
Member

tqchen commented Aug 6, 2021

Would be great to update the RFC naming per #17. cc @comaniac @zhiics who might have some insights from BYOC and @echuraev who might draw some parallels from Apple lib integration

@tqchen tqchen added the status: need review RFC needs review label Aug 6, 2021
Ashutosh Parkhi added 4 commits August 6, 2021 20:50
Change-Id: I3b0954f3fdb4d54b3e38a84de0ab649c1e79bca8
Change-Id: I6142c001175cdf41c58b5bb555a39e07c834254f
Change-Id: Id63d1866cd783f5e59b568f36c9177ee8715bc4d
Change-Id: Id54bae4bd2ca4bd9c3ab734e8cae966ebbe332b2
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. One thing I'm a little bit worry about is this RFC depends on another two RFCs (target hook and Ethos-U integration). I would be better if this RFC can also discuss the impact and alternative plan without other 2 RFCs so that this RFC won't be blocked.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashutosh-arm apologies for the delay! just a few points to clarify here.

Ashutosh Parkhi added 2 commits August 10, 2021 13:46
Change-Id: I56a5f9bf319576d342a5bdc3771402262584e8c4
… with some terminologies

Change-Id: I002be9cc67b72444ea27fe0a31769549fb6fd452
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashutosh-arm i did another round here to clarify your intentions. i think this should be the bulk of my questions on this RFC

@areusch
Copy link
Contributor

areusch commented Aug 28, 2021

Merging as Cody said LGTM in his last review

@areusch areusch merged commit 39124b1 into apache:main Aug 28, 2021
@asparkhi asparkhi deleted the cmsis branch August 31, 2021 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need review RFC needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants