-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[ETHOSN] Add support for experimental compiler option #13410
Conversation
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.
Generated by tvm-bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lhutton1. I have added a few questions/comments.
@@ -713,9 +713,17 @@ runtime::ethosn::OrderedCompiledNetwork EthosnCompiler::CompileEthosnFunc(const | |||
auto network_with_ids = ConstructNetwork(mod, gvar, func); | |||
// Now set the required build flags | |||
sl::CompilationOptions options = CreateOptions(); | |||
// Finally compile the network | |||
// Set the experimental compiler if enabled, for now this is not part of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting and unsetting the variable from here means that its being set multiple times during the compilation. Is there a better place from where it gets set/unset only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thinking was to reduce the scope of the environment variable to the support library, since its not prefixed in any way there's always the (small!) chance the same variable can be used elsewhere. We could move the scope to the NPU codegen instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option could be to look for this option from just the tvmc frontend functions and set/unset it from python for the entire invocation of the tvmc. Maybe that leaves the EthosN code cleaner. Are there any downsides of doing that though? 🤔
43cb690
to
c705665
Compare
The support library currently supports enabling the experimental cascading compiler option via an environment variable `FORCE_EXPERIMENTAL_COMPILER`. This commit exposes the ability to enable this option through TVMC. Change-Id: Ie5667a300f35f99bc8f92d780a56894ef9bbe3ad
* Remove unused code * Add negative test case Change-Id: I5e12d070554954e320b4d15c02d5e2ada179fce5
c705665
to
fc47f98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lhutton1 for addressing the comments.
* [ETHOSN] Add support for experimental compiler option The support library currently supports enabling the experimental cascading compiler option via an environment variable `FORCE_EXPERIMENTAL_COMPILER`. This commit exposes the ability to enable this option through TVMC.
* [ETHOSN] Add support for experimental compiler option The support library currently supports enabling the experimental cascading compiler option via an environment variable `FORCE_EXPERIMENTAL_COMPILER`. This commit exposes the ability to enable this option through TVMC.
The support library currently supports enabling the experimental cascading compiler option via an environment variable
FORCE_EXPERIMENTAL_COMPILER
. This commit exposes the ability to enable this option through TVMC.