[GitHub] [tvm] areusch commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT

[GitBox <gi...@apache.org>]

areusch commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627551583



##########
File path: src/target/source/codegen_c.cc
##########
@@ -662,6 +662,11 @@ void CodeGenC::VisitExpr_(const CallNode* op, std::ostream& os) {  // NOLINT(*)
       os << " != ";
       this->PrintExpr(op->args[0], os);
       os << ")";
+    } else if (op->op.same_as(builtin::lookup_param())) {
+      ICHECK_EQ(op->args.size(), 1);
+      const StringImmNode* str = op->args[0].as<StringImmNode>();
+      ICHECK(str != nullptr);
+      os << "__tvm_param__" << str->value;

Review comment:
       i think my main question with this is: how can we support a user overriding a param at runtime? or, switching to a different "set" of parameters? we don't have to solve that with this PR, but it seems like we need to retain some level of indirection. given this is a fairly infrequent, non-deployment use case, it seems like we could maintain a single global pointer to a "parameter struct," and that struct would contain pointers to various params used by the model. user code could then override that.
   
   so my main question here is: having such a struct implies a function argument or global pointer state. does that agree with the direction you want to take this? perhaps if this PR is part of something larger than just "remove an unnecessary function call," could you sketch a short (like 2-3 paragraph) RFC describing what you want to do?




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