[GitHub] [tvm] Mousius commented on a change in pull request #8014: [AOT] Name mangling in AOT

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

Mousius commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r630137920



##########
File path: include/tvm/runtime/module.h
##########
@@ -231,7 +231,7 @@ constexpr const char* tvm_param_prefix = "__tvm_param__";
 /*! \brief A PackedFunc that looks up linked parameters by storage_id. */
 constexpr const char* tvm_lookup_linked_param = "_lookup_linked_param";
 /*! \brief The main AOT executor function */
-constexpr const char* tvm_run_func_prefix = "tvm__run_func";
+constexpr const char* tvm_run_func_prefix = "run_func";

Review comment:
       This is now used as a suffix?

##########
File path: python/tvm/relay/build_module.py
##########
@@ -145,7 +150,10 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
         old_autotvm_silent = autotvm.GLOBAL_SCOPE.silent
         autotvm.GLOBAL_SCOPE.silent = use_auto_scheduler
 
-        self._build(mod, target, target_host, executor)
+        if not mod_name:

Review comment:
       This can be defaulted in the arguments.

##########
File path: include/tvm/tir/builtin.h
##########
@@ -346,6 +346,10 @@ TVM_DLL const Op& tvm_stack_make_array();
  */
 TVM_DLL const Op& tvm_call_packed();
 
+// This achieve the same of a packed call, but with an extern call
+// directly to the operator
+TVM_DLL const Op& tvm_call_unpacked();

Review comment:
       I don't think this is meant for this PR?

##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -35,6 +35,13 @@
 from tvm.micro import export_model_library_format
 
 
+def mangle_name(mod_name, name):
+    if mod_name:
+        return f"{mod_name}_{name}"

Review comment:
       As above, we should use `f"_tvm_{mod_name}_{name}"` here

##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
 
     mod_indices = {"lib": 0, "src": 0}
     host_codegen_dir = os.path.join(codegen_dir, "host")
+    lib_name = f"{model_name}_lib" if model_name else "lib"

Review comment:
       I'm not sure about this one, either we want to have a consistent `src/libX.c` or if we're changing it to better integrate with apps it'd be better to go with `tvm_{model_name}X.c` 

##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -55,28 +62,16 @@ def subprocess_with_stdout_and_log(cmd, cwd, logfile, stdout):
                     print(text, end="")
 
 
-def create_main(test_name, input_list, output_list, output_path):
-    file_path = pathlib.Path(f"{output_path}/" + test_name).resolve()
-    # create header file
-    raw_path = file_path.with_suffix(".c").resolve()
-    with open(raw_path, "w") as main_file:
-        main_file.write("#include <stdio.h>\n")
-        main_file.write("#include <math.h>\n")
-        main_file.write('#include "tvm/runtime/crt/internal/aot_executor/aot_executor.h"\n')
-        main_file.write('#include "tvm/runtime/crt/stack_allocator.h"\n')
-        main_file.write("#define WORKSPACE_SIZE (16384*1024)\n")
-        main_file.write("static uint8_t g_aot_memory[WORKSPACE_SIZE];\n")
-
-        for i in range(0, len(input_list)):
-            main_file.write('#include "input_data%i.h"\n' % i)
-        for i in range(0, len(output_list)):
-            main_file.write('#include "expected_output_data%i.h"\n' % i)
-            main_file.write('#include "output_data%i.h"\n' % i)
-
-        main_file.write("extern tvm_model_t network;\n")
-        main_file.write("tvm_workspace_t app_workspace;\n")
-        main_file.write(
-            """
+def emit_main_network_definition(main_file, mod_name):
+    main_file.write(f'extern tvm_model_t {mangle_name(mod_name,"network")};\n')

Review comment:
       We need to figure out which symbol is publicly exposed as `extern` to a `_tvm` in a users application is asking them to delve into the internals of the model.

##########
File path: src/runtime/meta_data.h
##########
@@ -41,6 +41,16 @@
 namespace tvm {
 namespace runtime {
 
+inline String get_name_mangled(const String& module_name, const String& name) {
+  std::stringstream ss;
+  if (!module_name.empty()) {
+    ss << module_name << "_" << name;

Review comment:
       Similar to @manupa-arm's comments on the RFC, to avoid namespace pollution we should prefix all of these with `_tvm_`




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