[GitHub] [incubator-tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module

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

areusch commented on a change in pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#discussion_r528914198



##########
File path: python/tvm/micro/build.py
##########
@@ -162,7 +158,12 @@ def build_static_runtime(
 
         libs.append(compiler.library(lib_build_dir, lib_srcs, lib_opts))
 
-    libs.append(compiler.library(mod_build_dir, [mod_src_path], generated_lib_opts))
+    libs.append(

Review comment:
       out of curiosity, does this place the generated source file anywhere that's easy to find? it's kind of useful for debugging to be able to inspect it

##########
File path: include/tvm/ir/module.h
##########
@@ -56,11 +56,14 @@ class IRModuleNode : public Object {
   Map<GlobalTypeVar, TypeData> type_definitions;
   /*! \brief The source map for the module. */
   parser::SourceMap source_map;
+  /*! \brief The names of ext. functions for func registry */

Review comment:
       write out "external" here so the comment is clear

##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -215,28 +214,37 @@ 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> 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_};
+    return std::make_tuple(sid, builder.const_vars_, builder.JIT(out));
   }
 
   runtime::Module CreateCSourceModule(const ObjectRef& ref) override {
+    ICHECK(ref->IsInstance<FunctionNode>());
+    auto res = GenCFunc(Downcast<Function>(ref));
+    String sym = std::get<0>(res);
+    Array<String> variables = std::get<1>(res);
+
     // Create headers
-    code_stream_ << "#include <cstring>\n";
-    code_stream_ << "#include <vector>\n";
-    code_stream_ << "#include <tvm/runtime/c_runtime_api.h>\n";
-    code_stream_ << "#include <tvm/runtime/container.h>\n";
-    code_stream_ << "#include <tvm/runtime/packed_func.h>\n";
-    code_stream_ << "#include <dlpack/dlpack.h>\n";
-    code_stream_ << "using namespace tvm::runtime;\n";
+    code_stream_ << "#include \"tvm/runtime/c_runtime_api.h\"\n";
+    code_stream_ << "#include \"tvm/runtime/c_backend_api.h\"\n";
+    code_stream_ << "#include <tvm/runtime/crt/module.h>\n";
+    code_stream_ << "#include <stdio.h>\n";
+    code_stream_ << "#include <stdlib.h>\n";
+    code_stream_ << "#include <string.h>\n";
+    if (!variables.empty()) {
+      // These are only needed to handle metadata copying
+      code_stream_ << "#include <tvm/runtime/container.h>\n";

Review comment:
       just wondering--I know this external compiler uses c++ but I don't think we should require it for µTVM. it's fine to be a demo, though.

##########
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##########
@@ -215,28 +214,37 @@ 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> 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_};
+    return std::make_tuple(sid, builder.const_vars_, builder.JIT(out));
   }
 
   runtime::Module CreateCSourceModule(const ObjectRef& ref) override {
+    ICHECK(ref->IsInstance<FunctionNode>());
+    auto res = GenCFunc(Downcast<Function>(ref));
+    String sym = std::get<0>(res);
+    Array<String> variables = std::get<1>(res);
+
     // Create headers
-    code_stream_ << "#include <cstring>\n";
-    code_stream_ << "#include <vector>\n";
-    code_stream_ << "#include <tvm/runtime/c_runtime_api.h>\n";
-    code_stream_ << "#include <tvm/runtime/container.h>\n";
-    code_stream_ << "#include <tvm/runtime/packed_func.h>\n";
-    code_stream_ << "#include <dlpack/dlpack.h>\n";
-    code_stream_ << "using namespace tvm::runtime;\n";
+    code_stream_ << "#include \"tvm/runtime/c_runtime_api.h\"\n";

Review comment:
       I think c standard libraries should go before project-specific libraries.




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