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 2021/01/13 18:02:14 UTC
[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 <cstring>\n";
- code_stream_ << "#include <vector>\n";
+ code_stream_ << "#include <stdio.h>\n";
+ code_stream_ << "#include <stdlib.h>\n";
+ code_stream_ << "#include <string.h>\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_backend_api.h>\n";
+ code_stream_ << "#include <tvm/runtime/crt/module.h>\n";
+ if (!variables.empty()) {
+ // These are only needed to handle metadata copying
+ 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";
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 <cstring>\n";
- code_stream_ << "#include <vector>\n";
+ code_stream_ << "#include <stdio.h>\n";
+ code_stream_ << "#include <stdlib.h>\n";
+ code_stream_ << "#include <string.h>\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_backend_api.h>\n";
+ code_stream_ << "#include <tvm/runtime/crt/module.h>\n";
+ if (!variables.empty()) {
+ // These are only needed to handle metadata copying
Review comment:
nitpick: delete "only" here--that statement is true only at this moment.
##########
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
Review comment:
can you make this a module-level all-CAPS constant?
##########
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 <cstring>\n";
- code_stream_ << "#include <vector>\n";
+ code_stream_ << "#include <stdio.h>\n";
+ code_stream_ << "#include <stdlib.h>\n";
+ code_stream_ << "#include <string.h>\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_backend_api.h>\n";
+ code_stream_ << "#include <tvm/runtime/crt/module.h>\n";
+ if (!variables.empty()) {
+ // These are only needed to handle metadata copying
+ code_stream_ << "#include <tvm/runtime/container.h>\n";
+ code_stream_ << "#include <tvm/runtime/packed_func.h>\n";
+ code_stream_ << "#include <dlpack/dlpack.h>\n";
Review comment:
@zhiics to clarify this codegen is just meant to generate C code, correct? we just got away with generating C++ code because export_library always writes .cc files. i also see that the BYOC blogpost has `-std=c++14` as a compiler arg, but is that necessary with this simple codegen_c?
it looks like the purpose of TVM_DLL_EXPORT_TYPED_FUNC is to do some limited argument type validation and translate the function call signature from TVMBackendPackedCFunc into a series of primitive pointers. what's confusing is that theoretically we shouldn't need to go through C++ to do this, and you wouldn't expect that to happen since this is called, well, CodegenC.
it seems like the typechecking is actually not buying us much, because it doesn't actually check the element datatype of any DLTensors passed-in. Here, we just `static_cast` and presume the element datatype matches. if that's true, i'd propose (perhaps not in this PR) that we get rid of the dependency on TVM_DLL_EXPORT_TYPED_FUNC here. I think we should aim to have the same C header dependencies as the `c` target codegen in `src/target/source/codegen_c_host.cc`.
----------------------------------------------------------------
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