You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/11/23 13:35:01 UTC
[GitHub] [incubator-tvm] manupa-arm opened a new pull request #6950: [uTVM] Initial BYOC support with c-source module
manupa-arm opened a new pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950
* Current flow uses saves the module as a c source and compiles it seperately. This blocks multi-module compilation.
* This patch upgrades it to use export_library which supports external modules.
* Added BYOC support for uTVM and testing
* A new member for IRModule to carry external function names as they are required to create the function registry when creating a system-lib.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#discussion_r528963561
##########
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:
Did we use the std namespace somewhere?
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
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<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:
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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
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<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:
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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#issuecomment-732342167
I'm not familiar with uTVM. Out of curiosity, how uTVM uses BYOC codegen C? I ask because BYOC codegen C is just for demonstration purpose so its functionality is not complete.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] manupa-arm edited a comment on pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
manupa-arm edited a comment on pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#issuecomment-732372088
@comaniac , Likewise this is also for demonstration purpose but in the uTVM contex. i.e., if we can annotate and offload a set of operations and produce a c-source that can do the required computations and return the DLTensors, this is to show it works in the uTVM context.
Thus, we do not intend to use "ccompiler" as the solution rather to show a external codegen that could create c-source the place of the "ccompiler", the changes done here should enable the compilation of a such a source. Moreover, we do not prefer the metadata packing of Imports (using PackImports* methods) as they will re-create the artifacts in the volatile memory. However, having a c-source gives much better control as to where the artifacts should be placed through a standard linker script.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
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=<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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#issuecomment-732372088
@comaniac , Likewise this is also for demonstration purpose but in the uTVM contex. i.e., if we can annotate and offload a set of operations and produce a c-source that can do the required computations and return the DLTensors, this is to show it works in the uTVM context.
Thus, we do not intend to use "ccompiler" as the solution rather to show a external codegen that could create c-source the place of the "ccompiler", the changes done here should enable the compilation of a such a source. Moreover, we do not prefer the metadata Packing of Imports (either using PackImports* methods) as they will re-create the artifacts in the volatile memory. However, having a c-source gives much better control as to where the artifacts should be placed through a standard linker script.
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by 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
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
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<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:
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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#issuecomment-732326410
cc: @tqchen @areusch @zhiics @comaniac
----------------------------------------------------------------
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
[GitHub] [incubator-tvm] manupa-arm edited a comment on pull request #6950: [uTVM] Initial BYOC support with c-source module
Posted by GitBox <gi...@apache.org>.
manupa-arm edited a comment on pull request #6950:
URL: https://github.com/apache/incubator-tvm/pull/6950#issuecomment-732372088
@comaniac , Likewise this is also for demonstration purpose but in the uTVM context. i.e., if we can annotate and offload a set of operations and produce a c-source that can do the required computations and return the DLTensors, this is to show it works in the uTVM context.
Thus, we do not intend to use "ccompiler" as the solution rather to show a external codegen that could create c-source the place of the "ccompiler", the changes done here should enable the compilation of a such a source. Moreover, we do not prefer the metadata packing of Imports (using PackImports* methods) as they will re-create the artifacts in the volatile memory. However, having a c-source gives much better control as to where the artifacts should be placed through a standard linker script.
----------------------------------------------------------------
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