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/05/10 23:09:56 UTC
[GitHub] [tvm] giuseros opened a new pull request #8014: [AOT] Name mangling in AOT
giuseros opened a new pull request #8014:
URL: https://github.com/apache/tvm/pull/8014
Mini-RFC is here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot
With this change we'll mangle the name of global symbols so that we can bundle
together multiple models in the same application.
The relay.build interface has been left unchanged, which means I am
resuing mod_name as a prefix for all functions. If mod_name is None then
a `_tvm` prefix is used.
I had to add two different compilation functions:
- `_CompileEngineLowerWithModuleName` to mangle all the operators with the mod_name
- `PartitionGraphWithModName` to mangle all the operators produced by BYOC
I could have changed signature of both, but that would have meant a very
invasive refactoring.
I refactored the aot test utils and added some tests for multiple
models.
Change-Id: I310af75c24e422861aeaceb3c3cd4cd602071df5
--
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] [tvm] areusch merged pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8014:
URL: https://github.com/apache/tvm/pull/8014
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r630137920
##########
File path: include/tvm/runtime/module.h
##########
@@ -231,7 +231,7 @@ constexpr const char* tvm_param_prefix = "__tvm_param__";
/*! \brief A PackedFunc that looks up linked parameters by storage_id. */
constexpr const char* tvm_lookup_linked_param = "_lookup_linked_param";
/*! \brief The main AOT executor function */
-constexpr const char* tvm_run_func_prefix = "tvm__run_func";
+constexpr const char* tvm_run_func_prefix = "run_func";
Review comment:
This is now used as a suffix?
##########
File path: python/tvm/relay/build_module.py
##########
@@ -145,7 +150,10 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
old_autotvm_silent = autotvm.GLOBAL_SCOPE.silent
autotvm.GLOBAL_SCOPE.silent = use_auto_scheduler
- self._build(mod, target, target_host, executor)
+ if not mod_name:
Review comment:
This can be defaulted in the arguments.
##########
File path: include/tvm/tir/builtin.h
##########
@@ -346,6 +346,10 @@ TVM_DLL const Op& tvm_stack_make_array();
*/
TVM_DLL const Op& tvm_call_packed();
+// This achieve the same of a packed call, but with an extern call
+// directly to the operator
+TVM_DLL const Op& tvm_call_unpacked();
Review comment:
I don't think this is meant for this PR?
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -35,6 +35,13 @@
from tvm.micro import export_model_library_format
+def mangle_name(mod_name, name):
+ if mod_name:
+ return f"{mod_name}_{name}"
Review comment:
As above, we should use `f"_tvm_{mod_name}_{name}"` here
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{model_name}_lib" if model_name else "lib"
Review comment:
I'm not sure about this one, either we want to have a consistent `src/libX.c` or if we're changing it to better integrate with apps it'd be better to go with `tvm_{model_name}X.c`
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -55,28 +62,16 @@ def subprocess_with_stdout_and_log(cmd, cwd, logfile, stdout):
print(text, end="")
-def create_main(test_name, input_list, output_list, output_path):
- file_path = pathlib.Path(f"{output_path}/" + test_name).resolve()
- # create header file
- raw_path = file_path.with_suffix(".c").resolve()
- with open(raw_path, "w") as main_file:
- main_file.write("#include <stdio.h>\n")
- main_file.write("#include <math.h>\n")
- main_file.write('#include "tvm/runtime/crt/internal/aot_executor/aot_executor.h"\n')
- main_file.write('#include "tvm/runtime/crt/stack_allocator.h"\n')
- main_file.write("#define WORKSPACE_SIZE (16384*1024)\n")
- main_file.write("static uint8_t g_aot_memory[WORKSPACE_SIZE];\n")
-
- for i in range(0, len(input_list)):
- main_file.write('#include "input_data%i.h"\n' % i)
- for i in range(0, len(output_list)):
- main_file.write('#include "expected_output_data%i.h"\n' % i)
- main_file.write('#include "output_data%i.h"\n' % i)
-
- main_file.write("extern tvm_model_t network;\n")
- main_file.write("tvm_workspace_t app_workspace;\n")
- main_file.write(
- """
+def emit_main_network_definition(main_file, mod_name):
+ main_file.write(f'extern tvm_model_t {mangle_name(mod_name,"network")};\n')
Review comment:
We need to figure out which symbol is publicly exposed as `extern` to a `_tvm` in a users application is asking them to delve into the internals of the model.
##########
File path: src/runtime/meta_data.h
##########
@@ -41,6 +41,16 @@
namespace tvm {
namespace runtime {
+inline String get_name_mangled(const String& module_name, const String& name) {
+ std::stringstream ss;
+ if (!module_name.empty()) {
+ ss << module_name << "_" << name;
Review comment:
Similar to @manupa-arm's comments on the RFC, to avoid namespace pollution we should prefix all of these with `_tvm_`
--
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] [tvm] areusch commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-865437382
apologies, i think i missed this and it now needs a rebase or merge.
i discussed this with @jroesch a bit as well, who is now working in earnest to merge #7518 in the next two weeks. following that PR, we'll arrive at a state where we can begin to think of the entire compiler as a series of IRModule passes. our discussion was that, at that point, it probably makes sense to move things like name mangling and e.g. the type signature changes proposed by @Mousius to one end of the compiler stack (i.e. at the relay level or at the end of the TIR passes). this way, the middle part of the compiler remains as generic as possible.
we'd like to propose the following:
- let us sync and merge this PR
- we'll land #7518 on top of this in the next two weeks (e.g. aiming for before July 4 US holiday)
- following that PR, we'll likely reorganize this and move it to the end of the TIR passes
to that end, we'd like to briefly prioritize #7518 next week over AOT changes to reduce the churn and let it land, since it's a fairly invasive PR.
if that's ok with you guys, i'll hand this off to @jroesch to merge while i'm ooo next few days.
--
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] [tvm] manupa-arm commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r634900775
##########
File path: include/tvm/runtime/module.h
##########
@@ -231,7 +231,7 @@ constexpr const char* tvm_param_prefix = "__tvm_param__";
/*! \brief A PackedFunc that looks up linked parameters by storage_id. */
constexpr const char* tvm_lookup_linked_param = "_lookup_linked_param";
/*! \brief The main AOT executor function */
-constexpr const char* tvm_run_func_prefix = "tvm__run_func";
+constexpr const char* tvm_run_func_suffix = "run_func";
Review comment:
I think we should drop "func" because it adds little value as a name of the function.
cc : @areusch
##########
File path: src/relay/backend/aot_executor_codegen.cc
##########
@@ -374,13 +374,14 @@ class AOTExecutorCodegen : public ExprVisitor {
}
auto pf0 = GetPackedFunc("relay.backend._make_CCacheKey");
- auto pf1 = GetPackedFunc("relay.backend._CompileEngineLower");
+ auto pf1 = GetPackedFunc("relay.backend._CompileEngineLowerWithModuleName");
Review comment:
Why do we need a separate function ? Isnt it possible to improve _CompileEngineLower to accept module name generally ?
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -612,11 +613,20 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
class CompileEngineImpl : public CompileEngineNode {
public:
// Lower the function.
- CachedFunc Lower(const CCacheKey& key) { return LowerInternal(key)->cached_func; }
+ CachedFunc Lower(const CCacheKey& key) {
Review comment:
I think we can have just one Lower function -- is there a reason why mangling the name is bad ?
cc : @areusch
##########
File path: python/tvm/relay/build_module.py
##########
@@ -145,7 +150,10 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
old_autotvm_silent = autotvm.GLOBAL_SCOPE.silent
autotvm.GLOBAL_SCOPE.silent = use_auto_scheduler
- self._build(mod, target, target_host, executor)
+ if not mod_name:
Review comment:
Why cant the default be the empty string itself?
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +58,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{module_name}_lib" if module_name else "lib"
Review comment:
Maybe we can make the default a empty string and append it always ?
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -726,6 +726,18 @@ def PartitionGraph():
return _ffi_api.PartitionGraph()
+def MangleGraph(mod_name):
Review comment:
nit : I think this is name mangling the functions, right ? We might need a name that indicates it is name mangling the function names. What do others think ?
cc : @areusch @Mousius
##########
File path: python/tvm/relay/build_module.py
##########
@@ -70,6 +70,20 @@ def _convert_param_map(params):
return inputs
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
Review comment:
Why do we need this to be true if mod_name is None ?
##########
File path: tests/python/relay/aot/test_crt_aot.py
##########
@@ -348,7 +348,9 @@ def test_byoc_utvm(use_calculated_workspaces):
mod = tvm.IRModule()
ann = CcompilerAnnotator()
mod["main"] = ann.visit(f)
+
mod = tvm.relay.transform.PartitionGraph()(mod)
+ mod = tvm.relay.transform.MangleGraph("my_mod")(mod)
Review comment:
Is there a use case, we would not want to name mangle ? Im think name mangled function names are strictly superior. If thats the case, I dont like the idea having to run another pass for all BYOC codegens.
Seeing the previous conversation with @areusch , I agree that we dont need another PartitionGraphWithModName but also not sure we need to call a seperate pass explicitly either. My suggestion is we should name mangle by default otherwise the compilation pathway feels bit fragile.
At a minimum, we should move this pass as a sub-pass of PartitionGraph and call it by default.
This comment also has a synergy with why we need LowerWithModName in compile engine.
I think Im missing whats the value of not performing name mangling when user do provide with a mod_name.
##########
File path: src/relay/backend/compile_engine.h
##########
@@ -202,6 +202,13 @@ class CompileEngineNode : public Object {
* \return The result.
*/
virtual CachedFunc Lower(const CCacheKey& key) = 0;
+ /*!
+ * \brief Get lowered result and mangle functions names with a specific module name.
+ * \param key The key to the cached function.
+ * \param mod_name The module name to mangle the functions
+ * \return The result.
+ */
+ virtual CachedFunc LowerWithModuleName(const CCacheKey& key, const String mod_name) = 0;
Review comment:
Same as above, Im missing the requirement to have a whole new function.
##########
File path: src/relay/backend/compile_engine.cc
##########
@@ -612,11 +613,20 @@ class MakeShapeFunc : public backend::MemoizedExprTranslator<Array<te::Tensor>>
class CompileEngineImpl : public CompileEngineNode {
public:
// Lower the function.
- CachedFunc Lower(const CCacheKey& key) { return LowerInternal(key)->cached_func; }
+ CachedFunc Lower(const CCacheKey& key) {
+ auto mangle_fn = [](String name) { return name; };
+ return LowerInternal(key, mangle_fn)->cached_func;
+ }
+
+ CachedFunc LowerWithModuleName(const CCacheKey& key, const String mod_name) {
Review comment:
Same comment as above
--
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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-838480901
I marked the naming comments "resolved". The meaning is that we will discuss those on the RFC here: https://discuss.tvm.apache.org/t/mini-rfc-name-mangling-in-aot/
--
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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-847034194
Hi @manupa-arm ,
Thanks for your reply. I pushed a new set of changes trying to address your comments:
* Now I am using name-mangling as default in all TVM
* The `MangleGraph` pass is now the default in the partitioning pipeline
* The mangled mod-name is formed in python and passed down in C++ where we only concatenate to the function names
I guess that, in addition to make the tests pass, we need to agree on naming:
* `MangleGraph`. Is `MangleExternalFunction` better? Because I am only mangling the functions that will be offloaded to the external compiler
* `run_func`: is it ok for everyone if I use only `run`? My problem with only `run` is that it seems a bit too generic. Maybe `run_model`? 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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-839689938
I made the name-mangling logic completely decoupled by the non-name-mangling logic. This means that name mangling is enabled only in AOT and in `PartitionGraphWithModName`, the rest of TVM won't be using this feature by default.
--
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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-866166331
Hi @jroesch , @areusch ,
After further thinking we were wondering if we could merge both this PR and #8096 before merging #7518 . This is because #8096 would solve quite a few issues that we have with AoT at the moment.
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631679298
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -35,6 +35,13 @@
from tvm.micro import export_model_library_format
+def mangle_name(mod_name, name):
Review comment:
It's not used anywhere except for those tests. I would move this to be a util function only when (and if) we need it.
--
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] [tvm] areusch merged pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8014:
URL: https://github.com/apache/tvm/pull/8014
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631905572
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -386,7 +393,7 @@ runtime::Module BuildCHost(IRModule mod, Target target) {
// symbols are available to tvm_run_func
auto fun_name = std::string(kv.first->name_hint);
const bool is_aot_executor_fn =
- (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_prefix, 0) == 0);
+ (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_suffix) != std::string::npos);
Review comment:
yes, although would suggest to include "aot_" in the attr name and create a constant
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r630167126
##########
File path: python/tvm/relay/build_module.py
##########
@@ -145,7 +150,10 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
old_autotvm_silent = autotvm.GLOBAL_SCOPE.silent
autotvm.GLOBAL_SCOPE.silent = use_auto_scheduler
- self._build(mod, target, target_host, executor)
+ if not mod_name:
Review comment:
I can't use the default, unfortunately. If I specify `None` the object system will have issues to convert `None` to an empty string. That's why I need to construct the empty string manually.
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r638801855
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -480,11 +480,77 @@ IRModule FlattenTupleOutputs(IRModule module) {
return module;
}
+class Mangler : public MixedModeMutator {
Review comment:
The point is that now the pass is integrated inside `the relay._transform.PartitionGraph` pass, so there is now way to test this in isolation. This will be tested by every BYOC test
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r633566451
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
+ auto mangle_fn = [](String name) { return name; };
+ return PartitionGraphCommon(mangle_fn);
+}
+
+Pass PartitionGraphWithModName(String mod_name) {
Review comment:
Adding the `PartitionGraphWithModName` is a very simple change, but maybe is a tiny less elegant than a `MangleGraph` pass (which is a bit more code to write, to do a simple thing).I implemented (and used) the `MangleGraph` pass, so that you can see the difference. I would be still go for a `PartitionGraphWithModName` but I don't have a strong opinion 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:
users@infra.apache.org
[GitHub] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-837474844
cc: @manupa-arm @areusch @Mousius
--
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] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r638360064
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -228,6 +287,65 @@ def compile_and_run(
assert ret == 0
+def compile_and_run_multiple_models(mod_map, input_list_map, output_list_map, param_map):
+ """
+ This method verifies the generated source
+ """
+ target = "c -runtime=c --link-params --executor=aot"
+ tmp_path = utils.tempdir()
+ tmp_dir = tmp_path.temp_dir
+ tmp_dir = "."
Review comment:
remove this or the line above
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -178,10 +232,11 @@ def compile_and_run(
cflags += "-DTVM_CRT_STACK_ALLOCATOR_ENABLE_LIFO_CHECK "
with tvm.transform.PassContext(opt_level=3, config={"tir.disable_vectorize": True}):
- lib = tvm.relay.build(mod, target, target_host=target, params=params)
+ lib = tvm.relay.build(mod, target, target_host=target, params=params, mod_name=mod_name)
tmp_path = utils.tempdir()
tmp_dir = tmp_path.temp_dir
+ # tmp_dir = "."
Review comment:
remove
##########
File path: python/tvm/relay/transform/transform.py
##########
@@ -723,7 +724,20 @@ def PartitionGraph():
ret: tvm.transform.Pass
The registered pass that partitions the Relay program.
"""
- return _ffi_api.PartitionGraph()
+ mod_name = mangle_module_name(mod_name)
+ return _ffi_api.PartitionGraph(mod_name)
+
+
+def MangleGraph(mod_name):
+ """Mangle the functions that are supposed to be externally compiled
Review comment:
per numpydoc style, prefer one-liner for the first line in the docstring
##########
File path: tests/python/contrib/test_vitis_ai/test_vitis_ai_codegen.py
##########
@@ -305,6 +304,7 @@ def expected():
call0 = gv0(data, weight, bn_gamma, bn_beta, bn_mmean, bn_mvar)
mod["main"] = relay.Function([data, weight, bn_gamma, bn_beta, bn_mmean, bn_mvar], call0)
mod = relay.transform.InferType()(mod)
+ print(mod)
Review comment:
rm
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
Review comment:
can prob not break this line
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -480,11 +480,77 @@ IRModule FlattenTupleOutputs(IRModule module) {
return module;
}
+class Mangler : public MixedModeMutator {
Review comment:
this one seems pretty easy to unit test--possible to write one?
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
Review comment:
can you catch a more narrow exception?
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
+
+
+def mangle_module_name(mod_name):
Review comment:
can you write unit tests for this function?
##########
File path: cmake/modules/contrib/VitisAI.cmake
##########
@@ -19,9 +19,9 @@ if(USE_VITIS_AI)
set(PYXIR_SHARED_LIB libpyxir.so)
find_package(PythonInterp 3.6 REQUIRED)
if(NOT PYTHON)
- find_program(PYTHON NAMES python3 python3.6)
+ find_program(PYTHON NAMES python3)
endif()
- execute_process(COMMAND "${PYTHON_EXECUTABLE}" "-c"
+ execute_process(COMMAND "python3" "-c"
Review comment:
why this change?
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
Review comment:
the empty string is valid?
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631674824
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -386,7 +393,7 @@ runtime::Module BuildCHost(IRModule mod, Target target) {
// symbols are available to tvm_run_func
auto fun_name = std::string(kv.first->name_hint);
const bool is_aot_executor_fn =
- (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_prefix, 0) == 0);
+ (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_suffix) != std::string::npos);
Review comment:
Do you mean adding an attribute to the TIR function, e.g., `runner_func` and then this would become
`const bool is_aot_executor_fn = (kv.first->GetAttr("runner_func") == true)`?
--
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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-866034715
Hi @areusch , @jroesch ,
I would say, let's get this merged and once #7518 lands, we can sync-up if we want to have the mangling as a pass or as a feature into the compiler. I wouldn't have any strong opinions (and honestly I like the idea of having it as a pass) but maybe @manupa-arm or @Mousius might.
Thanks,
Giuseppe
--
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] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631909163
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{model_name}_lib" if model_name else "lib"
+
for dso_mod in dso_modules:
if dso_mod.type_key == "c":
index = mod_indices["src"]
mod_indices["src"] += 1
parent_dir = os.path.join(host_codegen_dir, "src")
- file_name = os.path.join(parent_dir, f"lib{index}.c")
+ file_name = os.path.join(parent_dir, f"{lib_name}{index}.c")
Review comment:
yeah i like this idea, but we should bump the [version](https://github.com/apache/tvm/blob/main/python/tvm/micro/model_library_format.py#L225) in the metadata to indicate a format change. since MLF is new and not used much yet, i don't think this is consequential, and would rather get in the practice of doing this now rather than trying to judge some more confusing criteria later on.
--
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] [tvm] areusch commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-868883971
@manupa-arm @Mousius please take a look and explicitly approve if you're ok w/ this change
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] giuseros edited a comment on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros edited a comment on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-847034194
Hi @manupa-arm ,
Thanks for your reply. I pushed a new set of changes trying to address your comments:
* Now I am using name-mangling as default in all TVM, removing duplication for non-mangled behaviour.
* The `MangleGraph` pass is now the default in the partitioning pipeline
* The mangled mod-name is formed in python and passed down in C++ where we only concatenate to the function names
I guess that, in addition to make the tests pass, we need to agree on naming:
* `MangleGraph`. Is `MangleExternalFunction` better? Because I am only mangling the functions that will be offloaded to the external compiler
* `run_func`: is it ok for everyone if I use only `run`? My problem with only `run` is that it seems a bit too generic. Maybe `run_model`? 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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-867705273
A friendly ping @manupa-arm , @jroesch , @areusch !
--
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] [tvm] areusch commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-868881445
@jroesch can you comment on @giuseros question above?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r645725820
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
+
+
+def mangle_module_name(mod_name):
Review comment:
didn't see these, not sure if you had an opinion whether it should be tested?
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r646813963
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
+
+
+def mangle_module_name(mod_name):
Review comment:
Yes, sorry, I forgot to push them. I think I added them 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:
users@infra.apache.org
[GitHub] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r633566451
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
+ auto mangle_fn = [](String name) { return name; };
+ return PartitionGraphCommon(mangle_fn);
+}
+
+Pass PartitionGraphWithModName(String mod_name) {
Review comment:
Adding the `PartitionGraphWithModName` is a very simple change, but maybe is a tiny less elegant than a `MangleGraph` pass (which is a bit more code to write, do to a simple thing).I implemented (and used) the `MangleGraph` pass, so that you can see the difference. I would be still go for a `PartitionGraphWithModName` but I don't have a strong opinion 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:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631378115
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -35,7 +35,7 @@ class UnsupportedInModelLibraryFormatError(Exception):
"""Raised when export_model_library_format does not support the given Module tree."""
-def _populate_codegen_dir(mod, codegen_dir: str):
+def _populate_codegen_dir(mod, codegen_dir: str, model_name: str = None):
Review comment:
can you add model_name to the parameters docstring?
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
Review comment:
I think this comment is confusing because it uses "function signatures" and `COMPILER_ID` which isn't obvious as a placeholder (i think?) unless you also read the following comment. Could you clarify?
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
+ auto mangle_fn = [](String name) { return name; };
+ return PartitionGraphCommon(mangle_fn);
+}
+
+Pass PartitionGraphWithModName(String mod_name) {
Review comment:
it seems like it may be a better design to create a "mangle" pass. curious what your thoughts are on this?
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -61,8 +62,14 @@ void CodeGenCHost::AddFunction(const PrimFunc& f) {
CodeGenC::AddFunction(f);
}
-void CodeGenCHost::LinkParameters(Map<String, LinkedParam> params) {
- PrintFuncPrefix();
+void CodeGenCHost::LinkParameters(Map<String, LinkedParam> params, bool static_linking) {
+ // TODO(giuseros): remove this once we don't need an external function call to
+ // retrieve the parameters.
+ if (static_linking) {
Review comment:
can you add documentation explaining why we need this and what it does?
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{model_name}_lib" if model_name else "lib"
+
for dso_mod in dso_modules:
if dso_mod.type_key == "c":
index = mod_indices["src"]
mod_indices["src"] += 1
parent_dir = os.path.join(host_codegen_dir, "src")
- file_name = os.path.join(parent_dir, f"lib{index}.c")
+ file_name = os.path.join(parent_dir, f"{lib_name}{index}.c")
Review comment:
i think we should rev the model library format version
##########
File path: python/tvm/relay/build_module.py
##########
@@ -115,6 +117,9 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
- If "graph" is specified, then the graph_executor will be used
- If "aot" is specified, then the aot_executor will be used
+ mod_name: Optional[str]
+ The module name we will build
Review comment:
are there restrictions on this? shall we spell them out? do we need to validate the model name to ensure it mangles correctly or otherwise apply some mangling?
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -35,6 +35,13 @@
from tvm.micro import export_model_library_format
+def mangle_name(mod_name, name):
Review comment:
should this become a util function rather than a test function?
##########
File path: src/target/source/codegen_c_host.cc
##########
@@ -386,7 +393,7 @@ runtime::Module BuildCHost(IRModule mod, Target target) {
// symbols are available to tvm_run_func
auto fun_name = std::string(kv.first->name_hint);
const bool is_aot_executor_fn =
- (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_prefix, 0) == 0);
+ (fun_name.rfind(::tvm::runtime::symbol::tvm_run_func_suffix) != std::string::npos);
Review comment:
perhaps it's worth identifying this with an attribute, since it doesn't have a fixed name?
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r630169041
##########
File path: tests/python/relay/aot/aot_test_utils.py
##########
@@ -55,28 +62,16 @@ def subprocess_with_stdout_and_log(cmd, cwd, logfile, stdout):
print(text, end="")
-def create_main(test_name, input_list, output_list, output_path):
- file_path = pathlib.Path(f"{output_path}/" + test_name).resolve()
- # create header file
- raw_path = file_path.with_suffix(".c").resolve()
- with open(raw_path, "w") as main_file:
- main_file.write("#include <stdio.h>\n")
- main_file.write("#include <math.h>\n")
- main_file.write('#include "tvm/runtime/crt/internal/aot_executor/aot_executor.h"\n')
- main_file.write('#include "tvm/runtime/crt/stack_allocator.h"\n')
- main_file.write("#define WORKSPACE_SIZE (16384*1024)\n")
- main_file.write("static uint8_t g_aot_memory[WORKSPACE_SIZE];\n")
-
- for i in range(0, len(input_list)):
- main_file.write('#include "input_data%i.h"\n' % i)
- for i in range(0, len(output_list)):
- main_file.write('#include "expected_output_data%i.h"\n' % i)
- main_file.write('#include "output_data%i.h"\n' % i)
-
- main_file.write("extern tvm_model_t network;\n")
- main_file.write("tvm_workspace_t app_workspace;\n")
- main_file.write(
- """
+def emit_main_network_definition(main_file, mod_name):
+ main_file.write(f'extern tvm_model_t {mangle_name(mod_name,"network")};\n')
Review comment:
I am not addressing this here. In AOT, as of now, we are producing a global struct, and I am simply making that global struct prefixed (and hence I need to prefix also the `extern` definition used by the user to point to that `struct`)
--
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] [tvm] areusch commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-865437382
apologies, i think i missed this and it now needs a rebase or merge.
i discussed this with @jroesch a bit as well, who is now working in earnest to merge #7518 in the next two weeks. following that PR, we'll arrive at a state where we can begin to think of the entire compiler as a series of IRModule passes. our discussion was that, at that point, it probably makes sense to move things like name mangling and e.g. the type signature changes proposed by @Mousius to one end of the compiler stack (i.e. at the relay level or at the end of the TIR passes). this way, the middle part of the compiler remains as generic as possible.
we'd like to propose the following:
- let us sync and merge this PR
- we'll land #7518 on top of this in the next two weeks (e.g. aiming for before July 4 US holiday)
- following that PR, we'll likely reorganize this and move it to the end of the TIR passes
to that end, we'd like to briefly prioritize #7518 next week over AOT changes to reduce the churn and let it land, since it's a fairly invasive PR.
if that's ok with you guys, i'll hand this off to @jroesch to merge while i'm ooo next few days.
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631677060
##########
File path: python/tvm/relay/build_module.py
##########
@@ -115,6 +117,9 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
- If "graph" is specified, then the graph_executor will be used
- If "aot" is specified, then the aot_executor will be used
+ mod_name: Optional[str]
+ The module name we will build
Review comment:
I didn't think about any restrictions. The idea was to use the name provided by the user as it is. You are asking something like : `verify("&%!") == false`? If yes, two points:
1. I would only validate. If the name is not valid, an error should be thrown
2. Any suggestions on what makes a "valid" name?
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r638778418
##########
File path: python/tvm/relay/backend/utils.py
##########
@@ -0,0 +1,39 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Utility backend functions."""
+
+
+def _is_valid_modname(mod_name):
+ """Determine if mod_name is a valid string to use inside
+ function names
+ """
+ if mod_name:
+ try:
+ mod_name.encode("ascii")
+ return True
+ except e:
+ return False
+
+ return True
Review comment:
Yes, if `mod_name` is empty we do `tvmgen_funname`. If it is not empty we do `tvmgen_MODNAME_funname`.
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r630166332
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{model_name}_lib" if model_name else "lib"
Review comment:
The idea here is that if we can specify the name also for the files, I can have those files into the same folder. This makes testing (and hence deployment) way, way easier. About the naming, let's discuss it on the RFC. I don't have any strong opinion, but the idea here was that if we do `{model_name}_function` for the operators we can also do `{model_name}_lib.c` for the file names. If we decide to put `_tvm_ as a constant prefix, then let's put it also in front of the file names (without the `_`)
--
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] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r645727270
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -480,11 +480,77 @@ IRModule FlattenTupleOutputs(IRModule module) {
return module;
}
+class NameMangleExtFuncs : public MixedModeMutator {
+ public:
+ explicit NameMangleExtFuncs(const IRModule& module, std::function<String(String)> mangle_fn)
+ : module_(module), mangle_fn_(mangle_fn) {}
+
+ IRModule Run() {
+ auto glob_funcs = module_->functions;
+
+ // Collect function names to be mangled and create
+ // global mangled variables
+ for (const auto& pair : glob_funcs) {
+ if (auto* fn = pair.second.as<FunctionNode>()) {
+ auto func = GetRef<Function>(fn);
+ if (func->GetAttr<String>(attr::kCompiler).defined()) {
+ auto fn_name_mangled = mangle_fn_(pair.first->name_hint);
+ GlobalVar gvar = GlobalVar(fn_name_mangled);
+ mangled_gvars_[pair.first->name_hint] = gvar;
+ }
+ }
+ }
+
+ // Walk the three and mangle the functions. Then replace compiler functions
Review comment:
nit: s/three/tree/ i 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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-854057639
Hi @manupa-arm , @areusch ,
All the tests have finally passed. Please, let me know if there is anything else I should address.
--
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] [tvm] giuseros commented on pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#issuecomment-854057639
Hi @manupa-arm , @areusch ,
All the tests have finally passed. Please, let me know if there is anything else I should address.
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631680451
##########
File path: python/tvm/micro/model_library_format.py
##########
@@ -55,17 +55,19 @@ def _populate_codegen_dir(mod, codegen_dir: str):
mod_indices = {"lib": 0, "src": 0}
host_codegen_dir = os.path.join(codegen_dir, "host")
+ lib_name = f"{model_name}_lib" if model_name else "lib"
+
for dso_mod in dso_modules:
if dso_mod.type_key == "c":
index = mod_indices["src"]
mod_indices["src"] += 1
parent_dir = os.path.join(host_codegen_dir, "src")
- file_name = os.path.join(parent_dir, f"lib{index}.c")
+ file_name = os.path.join(parent_dir, f"{lib_name}{index}.c")
Review comment:
Mmmm... Not sure I follow :) To be clear, I am adding a prefix to the lib names so that multiple source files can be in the same folder. Otherwise the user would need to compile each library and manually copy/paste the `lib*.c` files in a common folder with different names.
--
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] [tvm] areusch commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r631908084
##########
File path: python/tvm/relay/build_module.py
##########
@@ -115,6 +117,9 @@ def build(self, mod, target=None, target_host=None, params=None, executor="graph
- If "graph" is specified, then the graph_executor will be used
- If "aot" is specified, then the aot_executor will be used
+ mod_name: Optional[str]
+ The module name we will build
Review comment:
cool, i agree with that. mangling is annoying because you have to be 1:1 and doing this may confuse the user. filesystems can handle a lot of strange filenames now--I think C function names are the more restrictive use case of `mod_name` now. Looks like the spec is digits, underscores, letters, and escaped unicode chars. I'd suggest allowing all of that except the escaped unicode. we don't have the restriction on the first char being a digit since we will prefix this.
--
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] [tvm] giuseros commented on a change in pull request #8014: [AOT] Name mangling in AOT
Posted by GitBox <gi...@apache.org>.
giuseros commented on a change in pull request #8014:
URL: https://github.com/apache/tvm/pull/8014#discussion_r633566451
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
+ auto mangle_fn = [](String name) { return name; };
+ return PartitionGraphCommon(mangle_fn);
+}
+
+Pass PartitionGraphWithModName(String mod_name) {
Review comment:
Adding the `PartitionGraphWithModName` is a very simple change, but maybe is a tiny less elegant than a `MangleGraph` pass (which is a bit more code to write, do to a simple thing).I implemented (and used) the `MangleGraph` pass, so that you can see the difference. I would be still go for a `PartitionGraphWithModName` but I don't have a strong opinion here.
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -503,16 +506,35 @@ Pass PartitionGraph() {
return partitioning::RemoveDefaultAnnotations(m);
};
- runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func =
- [=](IRModule m, PassContext pc) { return partitioning::Partitioner(m).Partition(); };
+ runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> part_func = [=](IRModule m,
+ PassContext pc) {
+ return partitioning::Partitioner(m, mangle_fn).Partition();
+ };
auto flatten_tuples_pass = CreateModulePass(flatten_tuples, 0, "FlattenNestedTuples", {});
auto remove_default_pass = CreateModulePass(remove_defaults, 0, "RemoveDefaultAnnotations", {});
auto partition_pass = CreateModulePass(part_func, 0, "PartitionGraph", {});
return Sequential({flatten_tuples_pass, remove_default_pass, partition_pass, InferType()});
}
+Pass PartitionGraph() {
+ // Default version. All the function signatures will be "COMPILER_ID"
+ auto mangle_fn = [](String name) { return name; };
+ return PartitionGraphCommon(mangle_fn);
+}
+
+Pass PartitionGraphWithModName(String mod_name) {
Review comment:
Adding the `PartitionGraphWithModName` is a very simple change, but maybe is a tiny less elegant than a `MangleGraph` pass (which is a bit more code to write, to do a simple thing).I implemented (and used) the `MangleGraph` pass, so that you can see the difference. I would be still go for a `PartitionGraphWithModName` but I don't have a strong opinion 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:
users@infra.apache.org