[GitHub] [tvm] areusch commented on a diff in pull request #11770: [BYOC] Handle constants in IRModule-at-a-time external codegen

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

areusch commented on code in PR #11770:
URL: https://github.com/apache/tvm/pull/11770#discussion_r902764117


##########
include/tvm/ir/module.h:
##########
@@ -507,10 +509,32 @@ constexpr const char* kRuntime = "runtime";
 constexpr const char* kWorkspaceMemoryPools = "workspace_memory_pools";
 
 /*
- * \brief Module attribute for tir constants
+ * \brief All the runtime::NDArrays extracted from PrimFunc tir::AllocateConst nodes. The
+ * node will record the index into this array. See also kConstNameToNDArray below, which is
+ * the analog for Realy Functions.
+ *
+ * Type: Array<runtime::NDArray>
  */
 constexpr const char* kConstantsArray = "Constants";

Review Comment:
   if you have time, want to fix the naming here? (e.g. either kConstants and "constants" or kConstantsArray and "constants_array")



##########
python/tvm/relay/build_module.py:
##########
@@ -707,24 +708,24 @@ def create_executor(kind="debug", mod=None, device=None, target="llvm", params=N
     -------
     executor : :py:class:`~tvm.relay.backend.interpreter.Executor`
     """
+    raw_targets = Target.canon_multi_target(target)

Review Comment:
   so we require canon_multi_target, but we then immediate re-canonicalize. should we either assert it's canonicalized rather than force-converting it, or update docstring? perhaps here and in the executors, you really want a CanonicalizedTarget type?



##########
src/relay/backend/contrib/codegen_json/codegen_json.h:
##########
@@ -245,11 +258,14 @@ class JSONSerializer : public MemoizedExprTranslator<std::vector<JSONGraphNodeEn
     return memo_[GetRef<Expr>(vn)];
   }
 
-  std::vector<JSONGraphNodeEntry> VisitExpr_(const ConstantNode* cn) {
-    std::string name = symbol_ + "_const_" + std::to_string(params_.size());
-    params_.push_back(name);
-    auto node = std::make_shared<JSONGraphNode>(name, "const" /* op_type_ */);
-    return AddNode(node, GetRef<Expr>(cn));
+  std::vector<JSONGraphNodeEntry> VisitExpr_(const ConstantNode* constant_node) {
+    std::string name = symbol_ + "_const_" + std::to_string(const_names_.size());
+    VLOG(1) << "Will require parameter '" << name << "' to be available at runtime";

Review Comment:
   could we say more about "available"? does that mean the user needs to supply it to set_input()?



##########
src/relay/backend/contrib/cutlass/codegen.cc:
##########
@@ -43,6 +43,16 @@ namespace cutlass {
 
 namespace {
 
+/*! \brief Return the "cutlass" Target instance to use to guide compilation. */
+Target GetCutlassTarget() {
+  Target target = Target::Current(/*allow_not_defined=*/true);
+  if (!target.defined() || target->kind->name != "cutlass") {
+    // Use the default CUTLASS compilation options.
+    target = Target("cutlass");

Review Comment:
   how come target->kind->name != "cutlass" makes this happen? seems like it should icheck-fail?



##########
python/tvm/relay/build_module.py:
##########
@@ -607,16 +607,16 @@ class AotExecutor(_interpreter.Executor):
     device : :py:class:`Device`
         The runtime device to run the code on.
 
-    target : :py:class:`Target`
-        The target option to build the function.
+    raw_targets : Array[tvm.target.Target]
+        The available targets.
     """
 
-    def __init__(self, mod, device, target):
+    def __init__(self, mod, device, raw_targets):
         assert mod is not None
         self.mod = mod
         self.device = device
-        self.target = target
-        assert target.attrs.get("executor", "graph") == "aot"
+        self.raw_targets = raw_targets
+        assert raw_targets[0].attrs.get("executor", "graph") == "aot"

Review Comment:
   shall we add a corresponding assert for graph?
   
   what's the expected form of this array of Targets (seems the host should be the first one)? how should we know what to supply here (and for GraphExecutor)?



##########
src/relay/backend/aot_executor_codegen.cc:
##########
@@ -1328,17 +1333,11 @@ class AOTExecutorCodegenModule : public runtime::ModuleNode {
   runtime::NDArray get_param_by_name(String key) {
     auto it = this->output_.params.find(key);
     CHECK(it != this->output_.params.end()) << "no such parameter " << key;
-    return (*it).second.second;
+    return (*it).second;

Review Comment:
   huh?



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

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org