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.

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

[CLML][CODEGEN] CLML native codegen utility #13837

Merged
merged 4 commits into from
Feb 3, 2023

Conversation

srkreddy1238
Copy link
Contributor

@srkreddy1238 srkreddy1238 commented Jan 24, 2023

This util generates native CLML code given a DNN model. It does import via tvmc, extracts clml_modules, get the json source and finally generates clml_models.cc that holds source for various sub graphs. cpp_clml tool has additional infrastructure to compile it as a standalone binary that runs these models.

This PR adds symbol name to the generated json graph. Also, extends const_loader interface to get constant params.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 24, 2023

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.

  • No users to tag found in teams: clml, codegen See #10317 for details

Generated by tvm-bot

@TejashShah
Copy link

@srkreddy1238 srkreddy1238 force-pushed the clml_codegen branch 2 times, most recently from 9df8ec8 to c14d2e3 Compare January 25, 2023 05:30
This util generates native CLML code given a DNN model.
It does import via tvmc, extracts clml_modules, get the json source and
finally generates clml_models.cc that holds source for various sub graphs.
cpp_clml tool has additional infrastructure to compile it as a standalong
binary that runs these models.

This PR adds symbol name to the generates json grpah.
Also, extends const_loader interface to get constant params.
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Thank you for the nice application! Quickly took a look at the PR. Some of the comments.

apps/cpp_clml/CMakeLists.txt Outdated Show resolved Hide resolved
apps/cpp_clml/CMakeLists.txt Outdated Show resolved Hide resolved
apps/cpp_clml/CMakeLists.txt Outdated Show resolved Hide resolved
apps/cpp_clml/main.cc Outdated Show resolved Hide resolved
python/tvm/relay/op/contrib/clml.py Show resolved Hide resolved
python/tvm/relay/op/contrib/clml.py Show resolved Hide resolved
apps/cpp_clml/CMakeLists.txt Outdated Show resolved Hide resolved
apps/cpp_clml/clml_runner.cc Outdated Show resolved Hide resolved
@echuraev
Copy link
Contributor

Just another thought to discuss. Can we reuse somehow clml codegen from TVM? In this case is won't be necessary to support a script which generates clml code in this PR and when new operations will be added to clml runtime then they should be automatically supported by this application.

@srkreddy1238
Copy link
Contributor Author

@echuraev thanks for a quick review.

CMAKE options are taken from CLML SDK release. Let me relook for 32bit support and other options.

I thought about clml runtime reuse. This require separating current clml_runtime into two parts one with TVM code (all json runtime and tvm dependencies) and other with only CLML SDK interface (no tvm dependencies here). Then we can simplify clml_runner.cc to use the above interface which is common. Too much of redesign now, probably I will take another pass for this later.

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

In general LGTM. Several minor comments

apps/cpp_clml/CMakeLists.txt Show resolved Hide resolved
apps/cpp_clml/CMakeLists.txt Show resolved Hide resolved
apps/cpp_clml/clml_runner.h Outdated Show resolved Hide resolved
apps/cpp_clml/main.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Could you please replace all NULL with nullptr?

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@echuraev echuraev merged commit d35a8ab into apache:main Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants