[GitHub] [tvm] areusch commented on a change in pull request #7002: Created CSourceMetaData module for model metadata

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

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



##########
File path: python/tvm/runtime/module.py
##########
@@ -268,6 +268,11 @@ def export_library(self, file_name, fcompile=None, addons=None, **kwargs):
             If fcompile has attribute object_format, will compile host library
             to that format. Otherwise, will use default format "o".
 
+        workspace_dir : str, optional
+            The name of the workspace dir to create intermediary

Review comment:
       the path to a directory used to create

##########
File path: python/tvm/micro/build.py
##########
@@ -74,8 +74,8 @@ def path(self):
 
 _CRT_DEFAULT_OPTIONS = {
     "cflags": ["-std=c11"] + _COMMON_CFLAGS,
-    "ccflags": ["-std=c++11"] + _COMMON_CFLAGS,
-    "ldflags": ["-std=c++11"],
+    "ccflags": ["-std=gnu++11"] + _COMMON_CFLAGS,

Review comment:
       do we need to change to gnu? would prefer to stick with c++11

##########
File path: python/tvm/runtime/module.py
##########
@@ -268,6 +268,11 @@ def export_library(self, file_name, fcompile=None, addons=None, **kwargs):
             If fcompile has attribute object_format, will compile host library
             to that format. Otherwise, will use default format "o".
 
+        workspace_dir : str, optional
+            The name of the workspace dir to create intermediary
+            artifacts for the process exporting of the library.
+            If this is not provided a temporary dir will be created.
+
         kwargs : dict, optional
             Additional arguments passed to fcompile
         """

Review comment:
       update the docstring to describe the return type

##########
File path: python/tvm/runtime/module.py
##########
@@ -330,25 +341,25 @@ def export_library(self, file_name, fcompile=None, addons=None, **kwargs):
 
         if self.imported_modules:
             if enabled("llvm") and llvm_target_triple:
-                path_obj = temp.relpath("devc." + object_format)
+                path_obj = os.path.join(workspace_dir, f"devc.{object_format}")
                 m = _ffi_api.ModulePackImportsToLLVM(self, is_system_lib, llvm_target_triple)
                 m.save(path_obj)
                 files.append(path_obj)
             else:
-                path_cc = temp.relpath("devc.cc")
+                path_cc = os.path.join(workspace_dir, "devc.cc")
                 with open(path_cc, "w") as f:
                     f.write(_ffi_api.ModulePackImportsToC(self, is_system_lib))
                 files.append(path_cc)
 
-        if has_c_module:
+        if has_c_module and not file_name.endswith(".tar"):

Review comment:
       can you add a comment to explain when .tar would be present here?

##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -215,20 +215,24 @@ class CodegenC : public MemoizedExprTranslator<std::vector<Output>>, public Code
 
 class CSourceCodegen : public CSourceModuleCodegenBase {
  public:
-  std::pair<std::string, Array<String>> GenCFunc(const Function& func) {
+  std::tuple<std::string, Array<String>, String, String> GenCFunc(const Function& func) {
     ICHECK(func.defined()) << "Input error: expect a Relay function.";
 
     // Record the external symbol for runtime lookup.
     auto sid = GetExtSymbol(func);
 
     CodegenC builder(sid);
     auto out = builder.VisitExpr(func->body);
-    code_stream_ << builder.JIT(out);
-
-    return {sid, builder.const_vars_};

Review comment:
       would also prefer to avoid sid here, since it's used in graph_runtime extensively as an int.

##########
File path: src/target/source/source_module.cc
##########
@@ -46,20 +48,25 @@ using runtime::SaveBinaryToFile;
  *        codegens, such as graph runtime codegen and the vm compiler.
  *
  * \param params The metadata for initialization of all modules.
- * \param dso_module The DSO module that contains TVM primitives.
- * \param modules The submodules that will be wrapped, e.g. CSource modules that
- *        contain vendor library calls or customized runtime modules.
- *
+ * \param modules All the modules that needs to be imported inside the metadata module(s).

Review comment:
       ext_modules, correct? can you also add a line for target_module?




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