[AOT] Sanitize input/output name in runtime #13046
Conversation
mehrdadh
changed the title
mehrdadh
force-pushed
the
aot/fix_name_mangling
branch
from
4bfa4af to
7be8741
Compare
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
| @@ -107,7 +108,7 @@ PackedFunc AotExecutor::GetFunction(const std::string& name, | |||
| } else if (name == "set_input_zero_copy") { | |||
| return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) { | |||
| if (String::CanConvertFrom(args[0])) { | |||
| int in_idx = this->GetInputIndex(args[0].operator String()); | |||
| int in_idx = this->GetInputIndex(tvm::runtime::SanitizeName(args[0].operator String())); | |||
There was a problem hiding this comment.
Does it also make sense to add a python test that passes in names like input:0 to test that this functionality doesn't regress for the various AoT commands?
There was a problem hiding this comment.
yeah it makes sense. I will add a test.
There was a problem hiding this comment.
I added a test which exams both mangled name and the original name
| inputs = {name: input_data} | ||
| runner.set_input(**inputs) | ||
| runner.run() | ||
| assert (runner.get_output(0).asnumpy() == expected_output).all() |
There was a problem hiding this comment.
also add checks of get_input and get_input_index since they are separate code paths in aot_executor.cc?
include/tvm/runtime/name_mangling.h
Outdated
|
|
||
| /*! | ||
| * \file tvm/runtime/name_mangling.h | ||
| * \brief Utility functions for name mangling which are used |
There was a problem hiding this comment.
Given the name transforms are fairly standalone, can we move them all rather than having some here and some in the original file?
There was a problem hiding this comment.
I moved this one since it is shared between runtime library and tvm library. I think it's better to keep the rest in src/backend since they are not used in runtime currently. wdyt?
There was a problem hiding this comment.
I don't see much harm moving them all across, they're small, self contained and it's likely if you need one you need others. Either way, it'd be good not to have name_mangling.h and name_transforms.h as it confuses people, if you don't want to move it together could we at least name them consistently? "Mangle" is also an unnecessarily aggressive word, we should probably not be using it in our codebase.
There was a problem hiding this comment.
Makes sense, I'll rename it to name_transforms
|
@Mousius PTAL. thanks! |
|
@tvm-bot rerun |
This PR adds name sanitization to input/output at AOT runtime module. This means when we use set_input/set_output with AOT, even if we use the original name of those input/outputs which were sanitized and changed in the codegen, it will map them to the correct input/output. For example if your model has an input with name `input:0`, the AOT codegen would change this to `input_0` but at runtime if we try to set this input it does not exist. Now, we use the same sanitization at runtime
This PR adds name sanitization to input/output at AOT runtime module. This means when we use set_input/set_output with AOT, even if we use the original name of those input/outputs which were sanitized and changed in the codegen, it will map them to the correct input/output. For example if your model has an input with name `input:0`, the AOT codegen would change this to `input_0` but at runtime if we try to set this input it does not exist. Now, we use the same sanitization at runtime
This PR adds name sanitization to input/output at AOT runtime module. This means when we use set_input/set_output with AOT, even if we use the original name of those input/outputs which were sanitized and changed in the codegen, it will map them to the correct input/output.
For example if your model has an input with name
input:0, the AOT codegen would change this toinput_0but at runtime if we try to set this input it does not exist. Now, we use the same sanitization at runtimecc @areusch