[GitHub] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r556740015
##
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##
@@ -229,25 +228,31 @@ class CSourceCodegen : public CSourceModuleCodegenBase {
String func_name = 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_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "using namespace tvm::runtime;\n";
+code_stream_ << "#include \n";
+code_stream_ << "#include \n";
+if (!variables.empty()) {
+ // These are only needed to handle metadata copying
+ code_stream_ << "#include \n";
+ code_stream_ << "#include \n";
+ code_stream_ << "#include \n";
+ code_stream_ << "using namespace tvm::runtime;\n";
Review comment:
hrm. should we then add code like:
```
#ifdef __cplusplus
using std::malloc;
#endif
```
you're right this is a demo codegen, perhaps we don't need to resolve this.
it's just confusing to create c++ code in a codegen named codegen_c.
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950: URL: https://github.com/apache/tvm/pull/6950#discussion_r556738065 ## File path: tests/micro/qemu/test_zephyr.py ## @@ -198,5 +200,143 @@ def test_relay(platform): tvm.testing.assert_allclose(result, x_in * x_in + 1) +class CcompilerAnnotator(ExprMutator): Review comment: okay and actually since this involves a separate codegen path, it makes sense to keep this here. 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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r556736523
##
File path: tests/micro/qemu/test_zephyr.py
##
@@ -198,5 +200,143 @@ def test_relay(platform):
tvm.testing.assert_allclose(result, x_in * x_in + 1)
+class CcompilerAnnotator(ExprMutator):
+"""
+This is used to create external functions for ccompiler.
+A simple annotator that creates the following program:
+ |
+ -- begin --
+ |
+ add
+ |
+subtract
+ |
+multiply
+ |
+ -- end --
+ |
+"""
+
+def __init__(self):
+super(CcompilerAnnotator, self).__init__()
+self.in_compiler = 0
+
+def visit_call(self, call):
+if call.op.name == "add": # Annotate begin at args
+if self.in_compiler == 1:
+lhs = compiler_begin(super().visit(call.args[0]), "ccompiler")
+rhs = compiler_begin(super().visit(call.args[1]), "ccompiler")
+op = relay.add(lhs, rhs)
+self.in_compiler = 2
+return op
+elif call.op.name == "subtract":
+if self.in_compiler == 1:
+lhs = super().visit(call.args[0])
+rhs = super().visit(call.args[1])
+if isinstance(lhs, relay.expr.Var):
+lhs = compiler_begin(lhs, "ccompiler")
+if isinstance(rhs, relay.expr.Var):
+rhs = compiler_begin(rhs, "ccompiler")
+return relay.subtract(lhs, rhs)
+elif call.op.name == "multiply": # Annotate end at output
+self.in_compiler = 1
+lhs = super().visit(call.args[0])
+rhs = super().visit(call.args[1])
+if isinstance(lhs, relay.expr.Var):
+lhs = compiler_begin(lhs, "ccompiler")
+if isinstance(rhs, relay.expr.Var):
+rhs = compiler_begin(rhs, "ccompiler")
+op = relay.multiply(lhs, rhs)
+if self.in_compiler == 2:
+op = compiler_end(op, "ccompiler")
+self.in_compiler = 0
+return op
+return super().visit_call(call)
+
+
+def check_result(relay_mod, model, zephyr_board, map_inputs, out_shape,
result):
+"""Helper function to verify results"""
+tol = 1e-5
+target = tvm.target.target.micro(model)
+with tvm.transform.PassContext(opt_level=3,
config={"tir.disable_vectorize": True}):
+graph, mod, params = tvm.relay.build(relay_mod, target=target)
+
+with _make_session(model, target, zephyr_board, mod) as session:
Review comment:
ah that's correct, thanks for jogging my memory. great, so this test
should do everything you need for now.
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r556688725
##
File path: tests/micro/qemu/test_zephyr.py
##
@@ -198,5 +200,143 @@ def test_relay(platform):
tvm.testing.assert_allclose(result, x_in * x_in + 1)
+class CcompilerAnnotator(ExprMutator):
Review comment:
do you prefer this test here or in `tests/python/unittest/test_crt.py`?
this test is designed to compile against Zephyr and run either in qemu or on
bare metal, whereas the other simply tests the C runtime.
##
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##
@@ -229,25 +228,31 @@ class CSourceCodegen : public CSourceModuleCodegenBase {
String func_name = 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_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "using namespace tvm::runtime;\n";
+code_stream_ << "#include \n";
+code_stream_ << "#include \n";
+if (!variables.empty()) {
+ // These are only needed to handle metadata copying
+ code_stream_ << "#include \n";
+ code_stream_ << "#include \n";
+ code_stream_ << "#include \n";
+ code_stream_ << "using namespace tvm::runtime;\n";
Review comment:
how come we dropped `std::malloc` yet have a using decl here?
##
File path: tests/micro/qemu/test_zephyr.py
##
@@ -198,5 +200,143 @@ def test_relay(platform):
tvm.testing.assert_allclose(result, x_in * x_in + 1)
+class CcompilerAnnotator(ExprMutator):
+"""
+This is used to create external functions for ccompiler.
+A simple annotator that creates the following program:
+ |
+ -- begin --
+ |
+ add
+ |
+subtract
+ |
+multiply
+ |
+ -- end --
+ |
+"""
+
+def __init__(self):
+super(CcompilerAnnotator, self).__init__()
+self.in_compiler = 0
+
+def visit_call(self, call):
+if call.op.name == "add": # Annotate begin at args
+if self.in_compiler == 1:
+lhs = compiler_begin(super().visit(call.args[0]), "ccompiler")
+rhs = compiler_begin(super().visit(call.args[1]), "ccompiler")
+op = relay.add(lhs, rhs)
+self.in_compiler = 2
+return op
+elif call.op.name == "subtract":
+if self.in_compiler == 1:
+lhs = super().visit(call.args[0])
+rhs = super().visit(call.args[1])
+if isinstance(lhs, relay.expr.Var):
+lhs = compiler_begin(lhs, "ccompiler")
+if isinstance(rhs, relay.expr.Var):
+rhs = compiler_begin(rhs, "ccompiler")
+return relay.subtract(lhs, rhs)
+elif call.op.name == "multiply": # Annotate end at output
+self.in_compiler = 1
+lhs = super().visit(call.args[0])
+rhs = super().visit(call.args[1])
+if isinstance(lhs, relay.expr.Var):
+lhs = compiler_begin(lhs, "ccompiler")
+if isinstance(rhs, relay.expr.Var):
+rhs = compiler_begin(rhs, "ccompiler")
+op = relay.multiply(lhs, rhs)
+if self.in_compiler == 2:
+op = compiler_end(op, "ccompiler")
+self.in_compiler = 0
+return op
+return super().visit_call(call)
+
+
+def check_result(relay_mod, model, zephyr_board, map_inputs, out_shape,
result):
+"""Helper function to verify results"""
+tol = 1e-5
+target = tvm.target.target.micro(model)
+with tvm.transform.PassContext(opt_level=3,
config={"tir.disable_vectorize": True}):
+graph, mod, params = tvm.relay.build(relay_mod, target=target)
+
+with _make_session(model, target, zephyr_board, mod) as session:
Review comment:
I think technically right now this invokes the C++ compiler (due to
limitation in `export_library`). at some point in the future, i think the true
test you want would be one which invokes the C compiler--does that match your
understanding?
##
File path: src/relay/backend/contrib/codegen_c/codegen.cc
##
@@ -229,25 +228,31 @@ class CSourceCodegen : public CSourceModuleCodegenBase {
String func_name = 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_ << "#include \n";
-code_stream_ << "#include \n";
-code_stream_ << "#include \n";
-
[GitHub] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r532803023
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
cool--that makes sense to me.
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r532793299
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
ok, looking forward to it. sorry what I mean is: do you intend to output
the same thing, even if it's just from a different module?
enumeration of the functions a TVMModule contains is a standing problem with
the PackedFunc module infra. cc @tqchen @jroesch
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r532779368
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
@manupa-arm that also seems reasonable to me. one question though--do
you still intend to keep FuncRegistry codegen as-is (i.e. would you still
generate the same global symbols)? it would be nice to avoid worrying about
alignment issues for global symbols if we are generating C.
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r530632362
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
I think the issue with metadata module is that it assumes it is just
generating the data for storage rather than immediate consumption on the target
device. it requires a load process that ultimately copies the data into RAM. we
want to avoid this at all cost on µTVM, because RAM is most precious. so I
think we either would need to modify metadatamodule to enforce alignment or
create something else.
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r530590667
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
i think so. I think that is the right way to go...I don't have bandwidth
for that til after TVM conf though :/
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r530590667
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
I think that is the right way to go...I don't have bandwidth for that
til after TVM conf though :/
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r530584966
##
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 external functions for func registry */
+ Array ext_func_names;
Review comment:
@zhiics I think i see what's happening here: with the c++ runtime on
full OS, codegen could generate symbols from a variety of modules and rely on
runtime lookup via dlsym. with C runtime which has no dlsym, we use a
FuncRegistry which is currently generated inside the host module. this has led
to the need for "model-level" metadata being passed to the codegen in a couple
of cases. probably the right thing to do is move FuncRegistry to its own module
and pass this metadata there?
##
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:
document the return type
##
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:
cool, I don't mind this approach
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950: URL: https://github.com/apache/tvm/pull/6950#discussion_r529785970 ## 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: I agree that we should discuss with an RFC. however, I don't want to break support for GDB debugging with source now. regarding the target_kind attribute--we do need to be a bit careful with those, because they serve as keys to the autotuning logs. I don't know that a debug source directory should be part of identifying an autotuning result. it seems like perhaps `export_library` could just allow you to specify the tempdir as a kwarg? 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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950:
URL: https://github.com/apache/tvm/pull/6950#discussion_r529781778
##
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 think this explicitly avoids use of compiler builtins, which are
sometimes perhaps broken?
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] [tvm] areusch commented on a change in pull request #6950: [uTVM] Initial BYOC support with c-source module
areusch commented on a change in pull request #6950: URL: https://github.com/apache/tvm/pull/6950#discussion_r529707962 ## 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: I think in the future we should consider generating full projects (I.e. zephyr projects) at which point we should have a defined way to describe module and library sources to the generators. saving generated code to the workspace tree means that (if the Workspace is operating with debug=True, you get source included in your debugger. that can come in handy when the thing causing a CHECK-fail is memory overflow from a TVMBackendAllocWorkspace inside an operator function. So I think we want A1 but we also want to compile the library from the user-accessible source files. 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
