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