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

[runtime] Add Metadata classes for AOTExecutor #10282

Merged
merged 5 commits into from
Feb 22, 2022

Conversation

areusch
Copy link
Contributor

@areusch areusch commented Feb 17, 2022

This PR adds Metadata classes to hold model metadata needed to implement a Module-based Model Runtime interface for AOT. These Metadata implementations are distinct from others in the codebase in that all of the backing data can exist in C as const struct (meaning that on small targets, it can live along with the code in e.g. flash memory).

TODO: add test to verify that metadata.h compiles from a C compiler.

cc @Mousius @manupa-arm @kparzysz-quic @masahi @mehrdadh

 * These were autogenerated in the original PR, but checking them in
   as plain code until we can revisit the auto-generator approach.
@github-actions github-actions bot requested a review from masahi February 18, 2022 20:23
@masahi
Copy link
Member

masahi commented Feb 21, 2022

I'm going to merge this tomorrow if there is no comment, to unblock @areusch development. This PR is hard to review without seeing how it is used - We are hoping to have an active discussion at #10283 which builds on this PR.

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.

@areusch I did have a look. My concerns are mostly around docs and I could not follow the need for all these polymorphic objects (maybe its due to missing docs).

Happy to take them in a follow up.

}
};

class InMemoryMetadataNode : public ::tvm::target::metadata::VisitableMetadataNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide more explanation for the variants of InMemory* ? Also the reasons for needing a seperate object for that as opposed to having a constructor for MetadataNode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also added docs

Copy link
Contributor Author

@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.

@manupa-arm I added a bunch of docs, PTAL when you get a minute!

}
};

class InMemoryMetadataNode : public ::tvm::target::metadata::VisitableMetadataNode {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also added docs

@github-actions github-actions bot requested review from manupak and masahi February 22, 2022 05:14
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!

@tmoreau89 tmoreau89 merged commit 33082e0 into apache:main Feb 22, 2022
@tmoreau89
Copy link
Contributor

Thank you @manupa-arm and @areusch ! This PR has been merged.

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
* Add new Metadata classes and base implementation.

 * These were autogenerated in the original PR, but checking them in
   as plain code until we can revisit the auto-generator approach.

* address masa comments

* Add documentation per Manupa's comments, and move kMetadataVersion namespace.

* remove get_name function, used for debugging

* clang-format
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