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

2020-11-24 Thread GitBox 


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



##
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##
@@ -215,28 +214,37 @@ class CodegenC : public 
MemoizedExprTranslator>, public Code
 
 class CSourceCodegen : public CSourceModuleCodegenBase {
  public:
-  std::pair> GenCFunc(const Function& func) {
+  std::tuple, 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());
+auto res = GenCFunc(Downcast(ref));
+String sym = std::get<0>(res);
+Array variables = std::get<1>(res);
+
 // Create headers
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "using namespace tvm::runtime;\n";
+code_stream_ << "#include \"tvm/runtime/c_runtime_api.h\"\n";

Review comment:
   Done





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:
us...@infra.apache.org

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

2020-11-24 Thread GitBox 


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



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

Review comment:
   Done





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:
us...@infra.apache.org

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

2020-11-24 Thread GitBox 


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



##
File path: src/relay/backend/contrib/codegen_c/codegen_c.h
##
@@ -235,14 +289,14 @@ class CodegenCBase {
 continue;
   }
   this->PrintIndents();
-  code_stream_ << "std::memcpy(out" << i << ", " << outs[i].name << ", 4 * 
" << outs[i].size
+  code_stream_ << "memcpy(out" << i << ", " << outs[i].name << ", 4 * " << 
outs[i].size

Review comment:
   I dont think we need the namespace. Do we ?





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:
us...@infra.apache.org

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

2020-11-24 Thread GitBox 


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



##
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##
@@ -215,28 +214,37 @@ class CodegenC : public 
MemoizedExprTranslator>, public Code
 
 class CSourceCodegen : public CSourceModuleCodegenBase {
  public:
-  std::pair> GenCFunc(const Function& func) {
+  std::tuple, 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());
+auto res = GenCFunc(Downcast(ref));
+String sym = std::get<0>(res);
+Array variables = std::get<1>(res);
+
 // Create headers
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \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 \n";
+code_stream_ << "#include \n";
+code_stream_ << "#include \n";
+code_stream_ << "#include \n";
+if (!variables.empty()) {
+  // These are only needed to handle metadata copying
+  code_stream_ << "#include \n";

Review comment:
   Yea, this part is not expected to run with uTVM as it involves copying 
Array of NDArrays which is something we dont want in the context of uTVM, hence 
the gating as this is a demo. 





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:
us...@infra.apache.org

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

2020-11-24 Thread GitBox 


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



##
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:
   No, the export_library will create a temporary directory to save source 
files and it will not live beyond the scope of the function. So it did not 
occur to me saving the sources is a feature (I thought it was a side-effect of 
this flow). Thus, I ll just say what I generally do: use pdb and do get_source 
to see the c-source created. Anyhow, do we think this would be a user-facing 
feature ?
   
   if so we can do two things : 
   A1 : Traverse module hierarchy and dump the sources here independently of 
export library, here.
   A2 :  More generally  we can introduce an attr to target_kind "c" in the 
lines of something like "-dbgdir=" and make the export_library to use 
that directory the dump the intermediary sources.
   
   I think A2 would be more generic, but would be beyond the scope of this PR. 
   Thus if people agree, I could just do A1 here for now with a TODO for A2 at 
some point. 
   What do you think ?





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:
us...@infra.apache.org