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 2021/05/06 10:51:35 UTC
[GitHub] [tvm] giuseros opened a new pull request #7988: AOT] Remove lookup parameter function in AOT
giuseros opened a new pull request #7988:
URL: https://github.com/apache/tvm/pull/7988
This PR aims at removing the function call to extract the parameters
within the AOT main function by introducing a `tir::lookup_param` builtin.
This has different benefits:
- In AOT we now only use the `v_handle` field of the `TVMValue` struct
- We save CPU cycles by not calling an intermediate function to extract
local parameters
- We reduce code size, since we don't need to pack a call to extract
parameters and we don't need to produce the `lookup_param` function
anymore within the compilation unit
Change-Id: I36c2f0724a79606424a4374f4f5cd669bb2a8a55
--
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] [tvm] areusch commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627594551
##########
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:
right now GraphExecutor calls `lookup_linked_param` to get a reference to each parameter, then stores that pointer locally in GraphExecutor. the stored pointer is what's actually used in computation.
i agree we can focus in the short term on making AOT work. my question is basically: if we go the step further and push those parameters into operator functions, we'll lose the ability to override them without adding some additional argument to each operator function. so just wanted to ask whether this was a direction you wanted to go later on?
otherwise, I don't think we need an RFC for this change standalone.
--
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] [tvm] areusch commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627639490
##########
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:
slightly different: right now the user calls a higher-level API than is provided by AOT. In this API, the user constructs a GraphExecutor, an opaque object which manages the graph-level tensors for them. No such corresponding thing exists in AOT right now.
As part of initialization, GraphExecutor attempts to find a PackedFunc named `lookup_linked_params` inside the module. If it exists, it invokes this function for each parameter SID and initializes a local parameter table with a DLTensor pointing at the returned `data`. The user can still override this local parameter table with a DLTensor of their choosing.
Over RPC, the user runs GraphExecutor on the RPC client, and this all works over RPC. This PR doesn't break that flow, though I agree that using AOT over RPC is impossible right now. I don't expect we will try to tackle that til we implement this higher-level `AOTExecutor`.
I agree implementing this higher-level e.g. `AOTExecutor` API isn't a priority for embedded work, but I think we need to ensure that others could add one for non-embedded use cases. It's the standard thing a TVM user expects right now. We can do that by effectively wrapping the generated AOT code in an `AOTExecutor` implementation and invoking `tvm_<model_name>_run_func` from the corresponding Executor `Run` function.
So my comment here is just making sure we don't engineer out our ability to override parameters when we add `AOTExecutor`. I think we can merge this PR now to unblock embedded work, but later introduce a compiler option to fetch each parameter from an e.g. model-level parameter lookup table (which can live in `.text`, but be overridden to one in RAM). This lookup table would provide enough indirection to support the override use case.
The reason I ask this now is that a natural next step after this PR would be: in a follow-on PR, push the `tir::lookup_linked_param` into the operator functions. Given @Mousius RFC about improving function call signatures, just wondering if this may also happen. Adding the lookup table is a minor change with this PR alone, but may be slightly more complicated if we then proceed with this change.
--
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] [tvm] giuseros commented on a change in pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r634769442
##########
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 am not mangling here because those parameters are declared as `static`, hence only visible internally in the compilation unit (and not in the library). To be clear, there is no risk of name clashing in this case (even if some other compilation unit was using the exact same names). I could mangle them, but it means that the code generator needs to know the `mod_name`, so I need to hack the `Codegen` method etc.. So it seems a lot of effort for something that is already private and doesn't add any clashing risk.
--
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] [tvm] giuseros commented on a change in pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r634769442
##########
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 am not mangling here because those parameters are declared as `static`, hence only visible internally in the compilation unit (and not in the library). To be clear, there is no risk of name clashing in this case (even if some other compilation unit was using the exact same names). I could mangle them, but it means that the code generator needs to know the `mod_name`, so I need to change the `Codegen` method etc.. So it seems a lot of effort for something that is already private and doesn't add any clashing risk.
--
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] [tvm] areusch commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by 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
[GitHub] [tvm] giuseros commented on pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#issuecomment-833429257
cc: @areusch @Mousius @manupa-arm
--
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] [tvm] giuseros commented on pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#issuecomment-843595753
Hi @areusch sorry for the delayed reply. Somehow my github notifications were not working properly.
--
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] [tvm] giuseros commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627565863
##########
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:
So, my bottom line is: let's try to clean AOT the best as we can now, and we can start thinking to add new functionalities in later. Also (IMHO), among the functionalities we may want to add, I am not sure that changing parameters at runtime is the top priority at the moment.
--
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] [tvm] giuseros commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627615180
##########
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:
Maybe I get your point, let me try to rephrase to see if I understood :)
So you are saying, right now, the user could not specify `--link-params` and she will provide a `lookup_linkparams` function to supply the parameters, and that will work also in e.g., RPC
With this change it will be more difficult, because if the user does not specify `--link-params` she will need to provide names directly, which is still doable in a static compilation scenario, but less doable in a dynamic environment like RPC
If I understood your point (please, correct if I am wrong), you are asking if the "dynamic-parameters" is a direction we want to explore. The reply is "not for now". We don't think this is a priority for now for our embedded work.
So, yes, I would consider this a stand-alone change, only aimed at improving performance/code size.
--
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] [tvm] giuseros commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r627561948
##########
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:
Hi Andrew, thanks for the comment, but I am not sure I follow. When using `--link-params`, how do we change parameters at runtime in AOT as of now? AFAIK the reason for that function to exist was to let the graph executor to get the params during the execution. AOT does not need this functionality, so we can remove the function, exactly how we removed the function registry. In the future, we may want to add a feature to change those parameters at runtime. I can do an RFC, but this change seems very small and straightforward. Just to be clear, I am not adding/removing any functionality, I am only making aot faster and smaller.
--
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] [tvm] areusch commented on a change in pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r631355963
##########
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:
along the lines of @mjs comment that `__` are reserved for c++ compiler naming, shall we rename this to simply `tvm_param_`? and, we should consider naming this after the model as well. finally, it would be great to retain `tvm_param_prefix` here
--
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] [tvm] areusch commented on a change in pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r631357341
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -398,12 +403,14 @@ runtime::Module BuildCHost(IRModule mod, Target target) {
cg.AddFunction(f);
}
- if (could_have_linked_params) {
+ if (could_have_linked_params && !aot_executor_fn.defined()) {
ICHECK(found_linked_params) << "-link-params given but none found";
+ cg.DeclareParameters(linked_params);
cg.LinkParameters(linked_params);
}
if (aot_executor_fn.defined()) {
+ cg.DeclareParameters(linked_params);
Review comment:
for consistency with the above, worth gating on `could_have_linked_params`?
--
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] [tvm] giuseros commented on a change in pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r634769442
##########
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 am not mangling here, because those parameters are static, hence only visible internally in the compilation unit (and not in the library). I can mangle them, but it means that the code generator needs to know the `mod_name`, so I need to hack the `Codegen` method etc.. I think @mshawcroft was referring to names exposed in the library (operators name, runner name, etc..) as opposed to local names to each compilation unit
--
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] [tvm] giuseros commented on a change in pull request #7988: AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #7988:
URL: https://github.com/apache/tvm/pull/7988#discussion_r628181929
##########
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 see your fear, if we push into the operators, then the `AOTExecutor` becomes more complicated. Fear it not, that's a very small contained PR, I don't intend to push the parameter lookup inside the operators. Even more so, because I think that this would make BYOC not working,
--
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] [tvm] areusch merged pull request #7988: [AOT] Remove lookup parameter function in AOT
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #7988:
URL: https://github.com/apache/tvm/pull/7988
--
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