You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/10/16 12:36:45 UTC
[GitHub] [incubator-tvm] mbaret opened a new pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
mbaret opened a new pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697
Currently, all codegens using BYOC must make use of the default ConstantUpdater pass. However, certain codegens, like Ethos-N, don't want to store any constants in metadata module. This provides an interface (via a global) to register a custom constant updating method and assigns a 'null' updater for the Ethos-N codegen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508814094
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
Essentially just so that I can write a test case, because Ethos-N registers a 'null' constant updater. This also has the benefit of removing the implicit coupling between the CCompiler and the default ConstantUpdater (currently they must both adopt the same naming and visiting convention).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r514351237
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
Agreed, but there's about 20 shared lines in this change between the two, not just those two. That's why I suggested creating a shared utility function rather than moving it all into ConstantUpdater.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r512046483
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I think we probably don't need to though. As you can see, currently it is just one/two lines of code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r512018505
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
Could you elaborate on this? I'm not sure what's meant my moving it to the updater.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510442186
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
@manupa-arm I think one of the problems we are trying to avoid is adding more and more explicit registries to the external codegens. But it doesn't seems necessary to me to register a constant updater for each codegen because currently probably only ethos-n needs an empty updater. I think we can just fallback to generic one if there is no need for a special updater.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#issuecomment-718866169
LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r512042436
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
Do you think it might be cleaner to introduce a new function in backend/utils called something like UpdateConstants which replicates the logic that currently lives directly in graph runtime/vm? That way we can keep the generic ConstantUpdater clean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r514361493
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I am fine with that as well
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510731290
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
If you agree to what I say, I think we should slowly start showing this style of design in ccompiler and promote it to be used by external codegen developers to make it future proof and deprecate the generic one living outside the codegen once we have every codegen comply to it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#issuecomment-717596107
this is because pytest will run all testcases together, remove a global func exist in c++ API https://github.com/apache/incubator-tvm/blob/main/src/runtime/registry.cc#L77 we could expose that as a PackedFunc in tvm._ffi.remove_global_func(name)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510286540
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
How about just add a sentence in the file description to explain that the constant updater directly uses the default/generic one so that we don't really need to have the exactly same one here? Since C compiler is not really for production use, I think we should keep is simple and clean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r511004606
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
@manupa-arm Thanks for clarification. I am not arguing the necessity of constant updater for codegens. My point is that we can probably just add to the ones that needs it. To keep consistency, you can also register it for each of the other codegens but we don't really need to copy the code everywhere if there can fully reuse the generic one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510724481
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
I do agree we need to avoid more and more explicit registries but this registry is more of a requirement to enable codegens based on other languages to be linked to TVM -- the same argument as we do relay.ext.<codegen_name> registry.
The problem I see with the current design of constant updating in this manner is if you look at the VisitExpr of ConstantNode, the correct lookup of constant is entirely dependent of the constant_idx that is generated by a generic constant updater living outside the codegen. This locks the down the design of external codegens to traverse in the same way GenericConstant updater is traversing and also blocks any modification to constant if such is required by the external codegen (e.g., compression, adding a header, etc.).
To highlight the further the issue of locking down the traversal will become more damaging when the graph becomes less-linear (the ones that could be generated via NASs in future). In which case we might have a degree of freedom of scheduling (i.e. we might want to schedule one operator before the other for various reasons -- such as caching, memory, performance that depends on the ext backend).
Although, I appreciate that its makes the life quite easy of a tvm developer to have the generic constant updater but however I think it should live in the codegen itself. It also improves readabilty as well -- because now we can figure out the why the constants are indexed in the same way and how it retrieves the exact constant due to the fact the codegen post it to the params in the first place -- rather than having a generic constant updater living outside the codegen sources.
Maybe its outside of the scope for this PR -- but this PR exposed a symptom of the real problem, IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r514141313
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I'm not exactly sure what you mean - these two lines specifically get the symbol from the name_node and the name of the constant updater global func. You need the symbol to initialise the ConstantUpdater, and the ConstantUpdater doesn't require global func name (because it's the default fallback option).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508746996
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
why is this needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r511857139
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
So the reason I added it for ccompiler is really just so that I could write a test for this code pathway. The Ethos-N updater doesn't really do anything so it's not great for testing. I could just temporarily register a new constant updater for ccompiler in the test instead, would that be better?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r512040382
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I meant that we could check the extern updaters in the constant updater itself, i.e. if there is one we use that one, otherwise we use the generic one. Then, we don't have to do have this in both graph_runtime_codegen.cc and compiler.cc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r514336095
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I am saying that we probably don't want to duplicate the code in both compiler.cc and build_module.cc. Instead, we can move it into ConstantUpdater so that difference places can share the same code. I am not saying these two lines are not needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510290218
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
@zhiics , What about the other way ? We make every codegen register a constant updater -- as you say if most of the codegen are based on JSONRuntime maybe that can implement the generic one and which could be overridden if the codegen requires. This way it creates a nice division of responsibilities -- constant creation and consumption is at more or less the same set of sources.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r512038605
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
Yeah, that would be better. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508817318
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
I think this doesn't couple to the updater though because it does nothing as what the constant updater does.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508817318
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
I think this doesn't couple to the updater though because it does nothing different as what the constant updater does.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508822425
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
The concern is that the ConstantUpdater has no direct coupling with the external codegens, and yet it is generic. That is to say, the C Compiler codegen _must_ visit and name its constants in the same way that the ConstantUpdater chooses to, or you get a runtime failure. However, this relationship isn't enforced by any mechanism and relies on the codegen implementer to understand they must follow the convention. A simple example where this may happen is if a codegen only wants to save some constants and not others - the Ethos-N codegen being an extreme example where it wants to save none of them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#issuecomment-717302876
It seems the custom updater I registered in the test persists between tests in CI and causes failures. Is there a way to unregister a function? @tqchen
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r510286540
##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -290,8 +290,37 @@ runtime::Module CCompiler(const ObjectRef& ref) {
return csource.CreateCSourceModule(ref);
}
+/*!
+ * \brief A visitor to add the constants used as params for MetadataModule.
+ */
+struct CCompilerConstantUpdater : public ExprVisitor {
Review comment:
How about just add a sentence in the file description to explain that the constant updater directly uses the default/generic one so that we don't really need to have the exactly same one here? Since C compiler is not really for production use, I think we should keep is simple.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
mbaret commented on pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#issuecomment-710020641
cc @manupa-arm @zhiics @comaniac
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #6697: [BYOC] Allow custom codegens to register their own constant updater
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6697:
URL: https://github.com/apache/incubator-tvm/pull/6697#discussion_r508747443
##########
File path: src/relay/backend/graph_runtime_codegen.cc
##########
@@ -371,10 +371,28 @@ class GraphRuntimeCodegen : public backend::MemoizedExprTranslator<std::vector<G
// Step into the functions that are handled by external codegen to
// collect metadata.
+ auto codegen = func->GetAttr<String>(attr::kCompiler);
+ CHECK(codegen.defined()) << "No external codegen is set";
+ std::string codegen_name = codegen.value();
const auto name_node = func->GetAttr<String>(tvm::attr::kGlobalSymbol);
- std::string symobl = std::string(name_node.value());
- ConstantUpdater const_visit(symobl, ¶ms_);
- const_visit(func);
+ std::string symbol = std::string(name_node.value());
+ std::string const_update_name = "relay.ext." + codegen_name + ".constant_updater";
Review comment:
I think we can move this to the updater instead of having changing here and vm compiler
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org