Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CMSIS-NN] Convert CMSIS-NN to use Target Hooks #9397

Merged
merged 3 commits into from
Nov 9, 2021
Merged

[CMSIS-NN] Convert CMSIS-NN to use Target Hooks #9397

merged 3 commits into from
Nov 9, 2021

Conversation

Mousius
Copy link
Member

@Mousius Mousius commented Oct 29, 2021

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

Found a few things whilst doing this:

  • Forgot to mutate PrimFunc arguments in LowerTE which meant functions weren't getting lowered passed the first function in test_networks
  • Target cmsis-nn needs to match external code generator cmsis-nn to connect the Target with the external code generator
  • Partition Graph needed to sanitise compiler names to generate them properly in C

@Mousius
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks
0316e87 
This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

Found a few things whilst doing this:
* Forgot to mutate PrimFunc arguments in LowerTE which meant functions weren't getting lowered passed the first function in test_networks
* Target `cmsis-nn` needs to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Partition Graph needed to sanitise compiler names to generate them properly in C
@Mousius
Copy link
Member Author

Mousius commented Oct 29, 2021

CC @ashutosh-arm @manupa-arm

@Mousius Mousius requested a review from leandron as a code owner October 29, 2021 10:25
ashutosh-arm
ashutosh-arm reviewed Nov 1, 2021
View reviewed changes
Copy link
Contributor

@ashutosh-arm ashutosh-arm left a comment

Choose a reason for hiding this comment

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

Thanks @Mousius. LGTM. Minor comments in there.

src/relay/backend/contrib/cmsisnn/relay_to_tir.cc
@@ -33,29 +34,46 @@ namespace relay {
namespace contrib {
namespace cmsisnn {

class RelayToTIRVisitor : public MixedModeVisitor {
class RelayToTIRVisitor : public MixedModeMutator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be renamed RelayToTIRVisitor --> RelayToTIRMutator?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll move this to a follow up 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't mean it as a blocking comment. A follow up is fine, should've marked as nit.

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

class CodeGenCMSISNN : public CodeGenC {
class CodeGenCMSISNN : public codegen::CodeGenCHost {
Copy link
Contributor

Choose a reason for hiding this comment

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

Its great that its working with CodeGenCHost now. Just looks tiny bit odd that we are generating target code by deriving from Host codegen. But I also want to say that I don't have any real problem if it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other Targets they get PrimFunc's which are essentially:

{ [Host Module] -> [Device Lib Module] } -> [Device]

So conceptually it makes sense to do:

{ [Host Module] } -> [Lib]

For CMSIS-NN and other library targets, as TVM is hosting the library - I think, would be interested in @manupa-arm's view here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine to me.

@ashutosh-arm ashutosh-arm mentioned this pull request Nov 1, 2021
Closed
@Mousius Mousius mentioned this pull request Nov 5, 2021
Merged
manupak
manupak approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM modulo the existing comments.

@Mousius
Copy link
Member Author

Mousius commented Nov 8, 2021

@ashutosh-arm could you take a look and explicitly approve if it's ok to move the name change to a follow up 😸

@manupak manupak merged commit b86aedd into apache:main Nov 9, 2021
@manupak
Copy link
Contributor

manupak commented Nov 9, 2021

Thanks @Mousius @ashutosh-arm !

@Mousius Mousius deleted the cmsis-target-hooks branch November 9, 2021 12:47
AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Nov 12, 2021
@AndrewZhaoLuo
Merge branch 'main' into aluo/qnn/conv2d-transpose-fq2i
da7a16b 
* main: (119 commits)
  [Topi][Op][PyTorch][Vitas] Fix inconsistent kernel layout conventions for conv2d_transpose (apache#9336)
  Fix repository URL in ubuntu_install_rocm.sh (apache#9425)
  Add LLVM-13 installation to Docker setup (apache#9498)
  [Relay] Use target_host determined at Relay level instead of recalculating it (apache#9499)
  Arm(R) Ethos(TM)-U NPU BinaryElementwise operators support (apache#9442)
  [COMMUNITY] Junru's and Wuwei's PGP key for ASF release (apache#9488)
  Add default for split op (apache#9489)
  [HOTFIX][TARGET] Change LOG in compilation config to DLOG (apache#9486)
  Fixed some warnings about lambda's closures that are bigger than necessary (apache#9481)
  [Support] Add libinfo into the runtime build (apache#9310)
  Change Call with TIRCallAttrs to call_lowered op (apache#9312)
  [ETHOSN] Streamline Ethos(TM)-N cross-compile rpc usage (apache#9477)
  [CMSIS-NN] Assert correct amount of CMSIS-NN artifacts in MLF (apache#9480)
  [MicroTVM][PyTest] Explicitly skip MicroTVM unittests. (apache#9335)
  [microNPU] Replace ICHECK with diagnostic context in type inference (apache#9470)
  Better host handling in CompilationConfig & debug printing (apache#9460)
  [AOT][Tests] Use pre-built libraries in Reference System tests (apache#9271)
  [TIR] Add type hint for TIR  (apache#9432)
  [TVMC] Add test for quantized pytorch model (apache#9467)
  [CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
  ...
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@Mousius @mehrdadh
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
692ecc7 
* [CMSIS-NN] Convert CMSIS-NN to use Target Hooks

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

* Mutated PrimFunc arguments in LowerTE so all functions are correctly lowered
* Made Target `cmsis-nn` to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Modified Partition Graph to sanitise compiler names to generate them properly in C
* Port tvmc fixes for hybrid targets
* Update NPU tests with new sanitisation
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@Mousius @mehrdadh
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
6b62ab4 
* [CMSIS-NN] Convert CMSIS-NN to use Target Hooks

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

* Mutated PrimFunc arguments in LowerTE so all functions are correctly lowered
* Made Target `cmsis-nn` to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Modified Partition Graph to sanitise compiler names to generate them properly in C
* Port tvmc fixes for hybrid targets
* Update NPU tests with new sanitisation
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@Mousius @ylc
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
2c55934 
* [CMSIS-NN] Convert CMSIS-NN to use Target Hooks

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

* Mutated PrimFunc arguments in LowerTE so all functions are correctly lowered
* Made Target `cmsis-nn` to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Modified Partition Graph to sanitise compiler names to generate them properly in C
* Port tvmc fixes for hybrid targets
* Update NPU tests with new sanitisation
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@Mousius @yangulei
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
bbc8309 
* [CMSIS-NN] Convert CMSIS-NN to use Target Hooks

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

* Mutated PrimFunc arguments in LowerTE so all functions are correctly lowered
* Made Target `cmsis-nn` to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Modified Partition Graph to sanitise compiler names to generate them properly in C
* Port tvmc fixes for hybrid targets
* Update NPU tests with new sanitisation
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@Mousius @ylc
[CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397)
198602b 
* [CMSIS-NN] Convert CMSIS-NN to use Target Hooks

This migrates CMSIS-NN to use Target Hooks instead of fully BYOC, which
means it will now go through any central passes the Driver API.

* Mutated PrimFunc arguments in LowerTE so all functions are correctly lowered
* Made Target `cmsis-nn` to match external code generator `cmsis-nn` to connect the Target with the external code generator
* Modified Partition Graph to sanitise compiler names to generate them properly in C
* Port tvmc fixes for hybrid targets
* Update NPU tests with new sanitisation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashutosh-arm ashutosh-arm ashutosh-arm left review comments

@manupak manupak manupak approved these changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@jwfromm jwfromm Awaiting requested review from jwfromm

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@mbaret mbaret Awaiting requested review from mbaret

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@merrymercy merrymercy Awaiting requested review from merrymercy

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@tqchen tqchen Awaiting requested review from tqchen

@trevor-m trevor-m Awaiting requested review from trevor-m

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

@leandron leandron Awaiting requested review from leandron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mousius @manupak @ashutosh-arm