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:
[email protected]
