Skip to content

[3/10] Moved TIR generation from Python to C++ for CMSIS-NN - #8951

Merged
areusch merged 3 commits into
apache:mainfrom
asparkhi:tirincpp
Sep 19, 2021
Merged

[3/10] Moved TIR generation from Python to C++ for CMSIS-NN#8951
areusch merged 3 commits into
apache:mainfrom
asparkhi:tirincpp

Conversation

@asparkhi

@asparkhiasparkhi commented Sep 7, 2021

Copy link
Copy Markdown
Contributor

This PR moves changes from Python to C++ for Softmax via CMSIS-NN. This is a follow up PR to #8833.

It is step 2a in #8646.

@asparkhiasparkhi mentioned this pull request Sep 7, 2021
Closed
@asparkhiasparkhi changed the title [CMSIS-NN] Moved TIR generation from Python to C++[3/10] Moved TIR generation from Python to C++ for CMSIS-NNSep 7, 2021
@asparkhiasparkhi mentioned this pull request Sep 7, 2021
Merged
Mousius
Mousius requested changes Sep 7, 2021
View reviewed changes

@MousiusMousius left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This approach feels a lot cleaner overall @ashutosh-arm, could you update the PR title to 2a ? 😸

Comment threadpython/tvm/relay/backend/contrib/cmsisnn/codegen.py Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
manupak
manupak requested changes Sep 7, 2021
View reviewed changes

@manupakmanupak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! I did a pass, see if you agree.

Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadpython/tvm/relay/backend/contrib/cmsisnn/codegen.py Outdated
Comment threadpython/tvm/relay/backend/contrib/cmsisnn/codegen.py Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
asparkhi
asparkhi commented Sep 8, 2021
View reviewed changes

@asparkhiasparkhi left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the quick reviews @Mousius and @manupa-arm .

Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadpython/tvm/relay/backend/contrib/cmsisnn/codegen.py Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc Outdated
manupak
manupak requested changes Sep 8, 2021
View reviewed changes

@manupakmanupak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another round of comments! :)

Sorry for doing this piecemeal.

Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc Outdated
Ashutosh Parkhi
Deleted self import for cmsisnn
1cbc7f5
Change-Id: I2cdcd7a90aa4749877c48bc6c7c4d27328856860
Ashutosh Parkhi
Reusing CodeGenC VistiExpr for softmax
8dd9d13
Change-Id: Ie41b695fa06468cd3b0bfe428c360e98438a9180
Mousius
Mousius approved these changes Sep 16, 2021
View reviewed changes

@MousiusMousius left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One minor point but happy to see it in the follow up.

Comment threadsrc/relay/backend/contrib/cmsisnn/codegen_cmsisnn.cc
manupak
manupak approved these changes Sep 16, 2021
View reviewed changes

@manupakmanupak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

areusch
areusch requested changes Sep 16, 2021
View reviewed changes

@areuschareusch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks @ashutosh-arm for the PR, just a couple questions

Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/relay_to_tir.cc

Map<String, ObjectRef> dict_attrs;
dict_attrs.Set("global_symbol", func_name_);
dict_attrs.Set("tir.noalias", Bool(true));

@areuschareuschSep 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i thiiink this is a no-op on c target, but can you explain what you're going for here?

@asparkhiasparkhiSep 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

What is tir.noalias used for? I am not sure. It is being used everywhere in the tests, so I thought it must be something that is being used in TIR passes.

Comment threadsrc/relay/backend/contrib/cmsisnn/tir_to_runtime.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/tir_to_runtime.cc
Comment threadsrc/relay/backend/contrib/cmsisnn/tir_to_runtime.cc
@Mousius

Mousius commented Sep 17, 2021

Copy link
Copy Markdown
Member

Hi @areusch, I acknowledge your questions but do we need to block the PR on them rather than moving forwards with the integration? I'm sure @ashutosh-arm will honourably follow up in a future PR in this series.

areusch
areusch approved these changes Sep 19, 2021
View reviewed changes

@areuschareusch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Mousius @ashutosh-arm ok given this is part of a larger set of PRs belonging to an RFC in-flight, let us merge this and add a follow-up.

@areusch
areusch merged commit 44b644c into apache:mainSep 19, 2021
AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Sep 20, 2021
@AndrewZhaoLuo
Merge branch 'main' into aluo/onnx/unsqueeze-alt
3f5e26d
* main:
[3/10] Moved TIR generation from Python to C++ for CMSIS-NN (apache#8951)
Support match pvar with dtype constraint (apache#9016)
@areuschareusch mentioned this pull request Sep 20, 2021
Merged
@MousiusMousius mentioned this pull request Sep 21, 2021
Merged
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@asparkhi@ylc
[3/10] Moved TIR generation from Python to C++ for CMSIS-NN (apache#8951
9c185bd
)
* [CMSIS-NN] Moved TIR Generation to C++
* Deleted self import for cmsisnn
Change-Id: I2cdcd7a90aa4749877c48bc6c7c4d27328856860
* Reusing CodeGenC VistiExpr for softmax
Change-Id: Ie41b695fa06468cd3b0bfe428c360e98438a9180
@junrushaojunrushao mentioned this pull request Nov 1, 2021
Closed
@asparkhi
asparkhi deleted the tirincpp branch December 7, 2021 10:09
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@asparkhi@ylc
[3/10] Moved TIR generation from Python to C++ for CMSIS-NN (apache#8951
e67536a
)
* [CMSIS-NN] Moved TIR Generation to C++
* Deleted self import for cmsisnn
Change-Id: I2cdcd7a90aa4749877c48bc6c7c4d27328856860
* Reusing CodeGenC VistiExpr for softmax
Change-Id: Ie41b695fa06468cd3b0bfe428c360e98438a9180
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MousiusMousiusMousius approved these changes
@areuschareuschareusch approved these changes
@manupakmanupakmanupak approved these changes
@anijain2305anijain2305Awaiting requested review from anijain2305
@comaniaccomaniacAwaiting requested review from comaniac
@jroeschjroeschAwaiting requested review from jroesch
@junrushaojunrushaoAwaiting requested review from junrushao
@jwfrommjwfrommAwaiting requested review from jwfromm
@leandronleandronAwaiting requested review from leandron
@MarisaKirisameMarisaKirisameAwaiting requested review from MarisaKirisame
@mbaretmbaretAwaiting requested review from mbaret
@mbrookhartmbrookhartAwaiting requested review from mbrookhart
@merrymercymerrymercyAwaiting requested review from merrymercy
@slyubomirskyslyubomirskyAwaiting requested review from slyubomirsky
@tqchentqchenAwaiting requested review from tqchen
@trevor-mtrevor-mAwaiting requested review from trevor-m
@vinx13vinx13Awaiting requested review from vinx13
@wweicwweicAwaiting requested review from wweic
@yzhliuyzhliuAwaiting requested review from yzhliu
@zhiicszhiicsAwaiting requested review from zhiics
@ZihengJiangZihengJiangAwaiting requested review from ZihengJiang

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@asparkhi@Mousius@areusch@manupak