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/02/26 01:28:41 UTC
[GitHub] [tvm] zxybazh opened a new pull request #7534: [WIP][Target] Add support for target object with host field compatible with previous api
zxybazh opened a new pull request #7534:
URL: https://github.com/apache/tvm/pull/7534
This PR is to support the new target obejct with host field in TVM, migrating functions to support both version. Future deprecation of old target host api is pending dicussion in the [RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844/44) channel. Fixed the problem of adding existing target host to its target.
----------------------------------------------------------------
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 #7534: [WIP][Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r583710655
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -216,7 +216,35 @@ def test_target_host_warning():
attributes fails as expected.
"""
with pytest.raises(ValueError):
- tgt = tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+ tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+
+
+def test_target_host_merge_0():
+ tgt = tvm.target.Target(tvm.target.Target("cuda --host nvidia/jetson-nano"), None)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_53"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 32768
+
+
+def test_target_host_merge_1():
+ tgt = tvm.target.Target("cuda --host llvm")
+ tgt = tvm.target.Target(tgt, tgt.host)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_merge_2():
+ with pytest.raises(ValueError):
+ tvm.target.Target(tvm.target.Target("cuda --host llvm"), tvm.target.Target("llvm"))
+
+
+def test_target_host_merge_3():
+ with pytest.raises(ValueError):
+ tvm.target.Target(tvm.target.Target("cuda --host llvm"), 12.34)
if __name__ == "__main__":
Review comment:
minor nit: I think we can write:
```
if __name__ == "__main__":
sys.exit(pytest.main([__file__] + sys.argv[1:]))
```
and then don't need to list tests by name here
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -216,7 +216,35 @@ def test_target_host_warning():
attributes fails as expected.
"""
with pytest.raises(ValueError):
- tgt = tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+ tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+
+
+def test_target_host_merge_0():
+ tgt = tvm.target.Target(tvm.target.Target("cuda --host nvidia/jetson-nano"), None)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_53"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 32768
+
+
+def test_target_host_merge_1():
+ tgt = tvm.target.Target("cuda --host llvm")
+ tgt = tvm.target.Target(tgt, tgt.host)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_merge_2():
+ with pytest.raises(ValueError):
Review comment:
want to assert on the message, either with match= or by inspecting the context manager?
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r597209297
##########
File path: python/tvm/target/target.py
##########
@@ -494,3 +498,32 @@ def _load_config_dict(config_dict_str):
if not isinstance(key, str):
return None
return config
+
+
+def refresh_host(target, host=None, target_is_key=True):
+ """Helpfer function to return a target and target host after updating each other.
Review comment:
A lot more clarified than mine.
--
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] junrushao1994 edited a comment on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793062861
Notes on the tutorial/tests change:
As we are changing user-facing tutorials, we should stick to what is the most readable to new-comers. My proposal is:
Replacing:
```python
target = "cuda"
target_host = "llvm"
```
with
```python
target = tvm.target.Target("cuda", host="llvm")
```
This way, we are not deprecating the old ways of using target and target_host, but encouraging the new ways of combining them from the very beginning
----------------------------------------------------------------
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] zxybazh commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793281419
> In general my opinion is of course we should maintain back compatible as possible, but we should not keep the non-effective argument (i.e., `target_host`). In short, `target_host` should not become an unused-argument anywhere after this PR. Since this warning has been disabled in TVM pylintrc, manual check should be done for this case.
>
> For API updating, I'd suggest either of the following approaches (can be applied case by case):
>
> 1. Simply remove the `target_host` argument if that API is seldemly used.
> 2. Keep the `target_host` argument, but log out a warning saying this is not effective anymore and will be deprecated.
Thanks Cody for the advice. Of course, if you spot any unused `target_host` hanging out there and we should simply remove that. For simplicity we should definitely remove `target_host` and replace it with `target.host` in most occurance. I did not remove all of them mainly because of two reasons:
1. For heterogeneous targets, there could be multiple targets maintained in a dict, which makes a single target_host more convenient in related context.
2. It takes more time to replace all the occurance, and part of the code would be refactored, e.g., the structure of `SearchTaskNode` and the signature of `target_host` related functions, once we decide to deprecate `target_host`. I want to leave the work later if we decide on deprecation and be more consistent in design at that time.
For API updating, both would be good choice. I would post two of them onto the [RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844/56) and see what others think and decide.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [WIP][Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r583836665
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -216,7 +216,35 @@ def test_target_host_warning():
attributes fails as expected.
"""
with pytest.raises(ValueError):
- tgt = tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+ tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+
+
+def test_target_host_merge_0():
+ tgt = tvm.target.Target(tvm.target.Target("cuda --host nvidia/jetson-nano"), None)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_53"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 32768
+
+
+def test_target_host_merge_1():
+ tgt = tvm.target.Target("cuda --host llvm")
+ tgt = tvm.target.Target(tgt, tgt.host)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_merge_2():
+ with pytest.raises(ValueError):
Review comment:
Noted with thanks!
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r597215602
##########
File path: python/tvm/relay/build_module.py
##########
@@ -205,7 +208,7 @@ def _build_module_no_factory(mod, target=None, target_host=None, params=None, mo
This wrapper is suitable to be used from other programming languages as
the runtime::Module can be freely passed between language boundaries.
"""
- return build(mod, target, target_host, params, mod_name).module
+ return build(mod, target, target_host, params=params, mod_name=mod_name).module
Review comment:
Fixed. I acutally wanted to remove the `target_host` argument 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] zxybazh commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793266194
> @zxybazh Thanks Xiyou for updating it! Let's focus on the encouraged canonical way (i.e. `tvm.target.Target("cuda", host="llvm")`), and do not use other syntactic sugars (e.g. raw string representation)
Fixed in tutorial code to emphasis argument usage.
----------------------------------------------------------------
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] comaniac commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r589878495
##########
File path: python/tvm/auto_scheduler/measure.py
##########
@@ -223,8 +223,8 @@ def recover_measure_input(inp, rebuild_state=False):
task = inp.task
new_task = SearchTask(
workload_key=task.workload_key,
- target=task.target,
- target_host=task.target_host,
+ target=tvm.target.Target(task.target, task.target_host),
+ target_host=None,
Review comment:
We should just remove this argument.
----------------------------------------------------------------
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] junrushao1994 commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r592000534
##########
File path: python/tvm/driver/build_module.py
##########
@@ -409,6 +419,16 @@ def build(inputs, args=None, target=None, target_host=None, name="default_functi
if not target_host:
target_host = "llvm" if tvm.runtime.enabled("llvm") else "stackvm"
+ new_input_mod = {}
+ for tar, mod in target_input_mod.items():
Review comment:
ditto
```suggestion
for tgt, mod in target_input_mod.items():
```
##########
File path: python/tvm/driver/build_module.py
##########
@@ -399,8 +399,18 @@ def build(inputs, args=None, target=None, target_host=None, name="default_functi
if not isinstance(mod, tvm.IRModule):
raise ValueError("inputs must be Schedule, IRModule," "or dict of str to IRModule.")
+ new_input_mod = {}
+ for tar, mod in target_input_mod.items():
Review comment:
`tar` might mean something else
```suggestion
for tgt, mod in target_input_mod.items():
```
##########
File path: python/tvm/relay/build_module.py
##########
@@ -129,6 +129,15 @@ def build(self, mod, target=None, target_host=None, params=None):
old_autotvm_silent = autotvm.GLOBAL_SCOPE.silent
autotvm.GLOBAL_SCOPE.silent = use_auto_scheduler
+ # Assume the target host of all targets in heterogenous target are identical
+ if isinstance(target, dict):
+ for k in target:
+ target[k] = Target(target[k], target_host)
+ target_host = target[k].host
Review comment:
ditto
##########
File path: src/relay/backend/vm/compiler.cc
##########
@@ -256,8 +256,11 @@ class VMFunctionCompiler : ExprFunctor<void(const Expr& expr)> {
target_host_(target_host),
expr_device_map_(std::move(expr_device_map)) {
for (const auto& it : targets) {
+ targets.Set(it.first, Target(targets[it.first], target_host));
+ target_host = targets[it.first]->GetHost().value_or(Target());
targets_[it.first->value] = it.second;
}
Review comment:
It is a bit tricky to mutate the container while iterating it. It is quite error-prone (because tvm::Map does copy-on-write internally). Please consider reconstruct a Map instead. Also, the logic is all over the codebase, so probably we need a helper function.
##########
File path: src/relay/backend/build_module.cc
##########
@@ -235,8 +235,15 @@ class RelayBuildModule : public runtime::ModuleNode {
* \param target_host Host target device
*/
void Build(IRModule mod, const TargetsMap& targets, const tvm::Target& target_host) {
+ // Create protected variable targets_ from ground up
targets_ = targets;
target_host_ = target_host;
+ for (const auto& it : targets) {
+ // Construct a new target with target host filed if available
+ targets_.Set(it.first, Target(it.second, target_host_));
+ target_host_ = targets_[it.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: src/relay/backend/build_module.cc
##########
@@ -481,6 +488,13 @@ class RelayBuildModule : public runtime::ModuleNode {
const runtime::PackedFunc* pf = runtime::Registry::Get("codegen.LLVMModuleCreate");
if (!target_host.defined()) target_host = (pf != nullptr) ? Target("llvm") : Target("stackvm");
+ // Update all the targets in the _targets TargetsMap
+ for (const auto& it : targets_) {
+ // Construct a new target with target host filed if available
+ targets_.Set(it.first, Target(it.second, target_host));
+ target_host = targets_[it.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -65,6 +65,13 @@ def compile(mod, target=None, target_host=None, params=None):
compiler = VMCompiler()
if params:
compiler.set_params(params)
+ if isinstance(target, dict):
+ for k in target:
+ target[k] = tvm.target.Target(target[k], target_host)
+ target_host = target[k].host
Review comment:
I saw the lookup, so it is probably better to use `.item` here
```suggestion
for tgt, mod in target.items():
```
##########
File path: python/tvm/driver/tvmc/autotuner.py
##########
@@ -23,6 +23,8 @@
from urllib.parse import urlparse
+import tvm
Review comment:
Don't need to import the entire package
```suggestion
from tvm.target import Target
```
##########
File path: src/auto_scheduler/feature.cc
##########
@@ -1397,9 +1397,12 @@ void GetPerStoreFeaturesFromFile(const std::string& filename, int max_lines, int
if (find_res == task_cache.end()) {
// rebuild task
Array<te::Tensor> tensors = (*workload_key_to_tensors)(workload_key);
- task = SearchTask(ComputeDAG(tensors), workload_key, cur_inp->task->target,
- cur_inp->task->target_host, cur_inp->task->hardware_params,
- cur_inp->task->layout_rewrite_option, cur_inp->task->task_input_names);
+ Target target = cur_inp->task->target, target_host = cur_inp->task->target_host;
Review comment:
split into two lines
##########
File path: src/relay/backend/vm/compiler.cc
##########
@@ -900,6 +903,10 @@ void VMCompiler::Lower(IRModule mod, const TargetsMap& targets, const tvm::Targe
exec_ = make_object<Executable>();
targets_ = targets;
target_host_ = target_host;
+ for (auto& iter : targets_) {
+ targets_.Set(iter.first, Target(targets_[iter.first], target_host_));
+ target_host_ = targets[iter.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: src/relay/backend/vm/compiler.cc
##########
@@ -1001,8 +1008,14 @@ transform::Sequential MemoryOpt(tvm::Target host_target, TargetsMap targets) {
return transform::Sequential(pass_seqs);
}
-IRModule VMCompiler::OptimizeModule(IRModule mod, const TargetsMap& targets,
- const Target& target_host) {
+IRModule VMCompiler::OptimizeModule(IRModule mod, const TargetsMap& targets_arg,
+ const Target& target_host_arg) {
+ TargetsMap targets = targets_arg;
+ Target target_host = target_host_arg;
+ for (auto& iter : targets) {
+ targets.Set(iter.first, Target(targets[iter.first], target_host));
+ target_host = targets[iter.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: src/target/target.cc
##########
@@ -375,7 +375,7 @@ Target::Target(const Map<String, ObjectRef>& config) {
Target::Target(Target target, Target host) {
ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
- CHECK(!n->host.defined())
+ CHECK((!n->host.defined()) || n->host == host)
Review comment:
```suggestion
CHECK(!n->host.defined() || n->host == host)
```
##########
File path: src/relay/transforms/memory_alloc.cc
##########
@@ -458,6 +462,10 @@ Pass ManifestAlloc(Target target_host, Map<tvm::Integer, tvm::Target> targets) {
TVM_REGISTER_GLOBAL("relay.transform.ManifestAlloc")
.set_body_typed([](Target target_host, Map<tvm::Integer, tvm::Target> targets) {
+ for (auto& iter : targets) {
+ targets.Set(iter.first, Target(targets[iter.first], target_host));
+ target_host = targets[iter.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: src/driver/driver_api.cc
##########
@@ -185,9 +185,11 @@ IRModule lower(te::Schedule sch, const Array<te::Tensor>& args, const std::strin
return mod;
}
-std::pair<IRModule, IRModule> SplitDevHostFuncs(IRModule mod_mixed, const Target& target,
- const Target& target_host,
+std::pair<IRModule, IRModule> SplitDevHostFuncs(IRModule mod_mixed, const Target& target_arg,
+ const Target& target_host_arg,
const transform::PassContext& pass_ctx) {
+ Target target = Target(target_arg, target_host_arg),
+ target_host = target->GetHost().value_or(Target());
Review comment:
split into two statements
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -130,6 +137,15 @@ def lower(self, mod, target=None, target_host=None):
"""
target = self._update_target(target)
target_host = self._update_target_host(target, target_host)
+
+ if isinstance(target, dict):
+ for k in target:
+ target[k] = tvm.target.Target(target[k], target_host)
+ target_host = target[k].host
Review comment:
ditto
##########
File path: python/tvm/relay/build_module.py
##########
@@ -263,14 +272,20 @@ def build(ir_mod, target=None, target_host=None, params=None, mod_name="default"
"instead of deprecated parameter mod (tvm.relay.function.Function)",
DeprecationWarning,
)
-
target = _update_target(target)
-
if isinstance(target_host, (str, Target)):
target_host = Target(target_host)
elif target_host:
raise ValueError("target host must be the type of str, " + "tvm.target.Target, or None")
+ if isinstance(target, dict):
+ for k in target:
+ target[k] = Target(target[k], target_host)
+ target_host = target[k].host
Review comment:
ditto
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -167,6 +183,15 @@ def optimize(self, mod, target=None, target_host=None, params=None):
"""
target = self._update_target(target)
target_host = self._update_target_host(target, target_host)
+
+ if isinstance(target, dict):
+ for k in target:
+ target[k] = tvm.target.Target(target[k], target_host)
+ target_host = target[k].host
Review comment:
I saw the same logic all over this file. would you like to add a helper function?
##########
File path: src/relay/transforms/memory_alloc.cc
##########
@@ -415,6 +415,10 @@ class DialectRewriter : public ExprMutator {
namespace transform {
Pass ManifestAlloc(Target target_host, Map<tvm::Integer, tvm::Target> targets) {
+ for (auto& iter : targets) {
+ targets.Set(iter.first, Target(targets[iter.first], target_host));
+ target_host = targets[iter.first]->GetHost().value_or(Target());
+ }
Review comment:
ditto
##########
File path: src/driver/driver_api.cc
##########
@@ -253,31 +255,45 @@ std::pair<IRModule, IRModule> SplitDevHostFuncs(IRModule mod_mixed, const Target
}
// Build for heterogeneous execution.
-runtime::Module build(const Map<Target, IRModule>& inputs, const Target& target_host) {
+runtime::Module build(const Map<Target, IRModule>& inputs, const Target& target_host_arg) {
auto pass_ctx = transform::PassContext::Current();
std::vector<runtime::Module> device_modules;
- Target target_host_val = target_host;
+ Target target_host = target_host_arg;
+ Map<Target, IRModule> updated_inputs;
+
+ // Fetch previous defined target host in targets
+ for (const auto& it : inputs) {
+ auto target = Target(it.first, target_host);
+ target_host = target->GetHost().value_or(Target());
+ }
+
if (!target_host.defined()) {
for (const auto& it : inputs) {
if (it.first->kind->device_type == kDLCPU || it.first->kind->device_type == kDLMicroDev) {
- target_host_val = it.first;
+ target_host = it.first;
break;
}
}
}
- if (!target_host_val.defined()) {
- target_host_val = DefaultTargetHost(target_host_val);
+ if (!target_host.defined()) {
+ target_host = DefaultTargetHost(target_host);
+ }
+
+ // Update target host for all targets
+ for (const auto& it : inputs) {
+ auto target = Target(it.first, target_host);
+ updated_inputs.Set(target, it.second);
}
Review comment:
ditto
##########
File path: tests/micro/qemu/test_zephyr.py
##########
@@ -46,7 +46,7 @@
def _make_sess_from_op(model, zephyr_board, west_cmd, op_name, sched, arg_bufs):
target = tvm.target.target.micro(model)
with tvm.transform.PassContext(opt_level=3, config={"tir.disable_vectorize": True}):
- mod = tvm.build(sched, arg_bufs, target, target_host=target, name=op_name)
+ mod = tvm.build(sched, arg_bufs, tvm.target.Target(target, target), name=op_name)
Review comment:
move the Target construction to the beginning of the function
##########
File path: src/target/target.cc
##########
@@ -408,12 +408,17 @@ Map<String, ObjectRef> TargetNode::Export() const {
{"tag", this->tag},
{"keys", this->keys},
};
+ if (this->host.defined()) result.Set("host", this->GetHost().value_or(Target())->Export());
Review comment:
```suggestion
if (this->host.defined()) {
result.Set("host", Downcast<Target>(this->host)->Export());
}
```
##########
File path: src/driver/driver_api.cc
##########
@@ -304,21 +320,26 @@ runtime::Module build(const Map<Target, IRModule>& inputs, const Target& target_
}
// Build for heterogeneous execution when target is a string.
-runtime::Module build(const Map<String, IRModule>& inputs, const Target& target_host) {
- Map<Target, IRModule> updated_input;
- for (const auto& it : inputs) {
- auto target = Target(it.first);
+runtime::Module build(const Map<String, IRModule>& inputs_arg, const Target& target_host_arg) {
+ Map<Target, IRModule> updated_inputs;
+ Target target_host = target_host_arg;
+ for (const auto& it : inputs_arg) {
+ auto target = Target(Target(it.first), target_host);
+ target_host = target->GetHost().value_or(Target());
Optional<String> device = target->GetAttr<String>("device");
if (device.defined() && device.value() == "vta") {
target = Target("ext_dev");
}
- updated_input.Set(target, it.second);
+ updated_inputs.Set(target, it.second);
}
- return build(updated_input, target_host);
+ return build(updated_inputs, target_host);
}
// Build for homogeneous execution.
-runtime::Module build(const IRModule& funcs, const Target& target, const Target& target_host) {
+runtime::Module build(const IRModule& funcs, const Target& target_arg,
+ const Target& target_host_arg) {
+ auto target = Target(target_arg, target_host_arg),
+ target_host = target->GetHost().value_or(Target());
Review comment:
split into two statements
##########
File path: tutorials/get_started/tensor_expr_get_started.py
##########
@@ -154,7 +153,7 @@
# - fadd runs the actual computation.
# - asnumpy() copies the GPU array back to the CPU and we can use this to verify correctness
#
-ctx = tvm.context(tgt, 0)
+ctx = tvm.context(str(tgt), 0)
Review comment:
```suggestion
ctx = tvm.context(tgt.kind.name, 0)
```
##########
File path: tests/python/integration/test_tuning.py
##########
@@ -131,12 +131,11 @@ def teardown_module():
def get_sample_task(target=tvm.target.cuda(), target_host=None):
+ target = tvm.target.Target(target, target_host)
+ target_host = target.host
Review comment:
is this used?
----------------------------------------------------------------
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] junrushao1994 edited a comment on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793062861
Notes on the tutorial/tests change:
As we are changing user-facing tutorials, we should stick to what is the most readable & encouraged way of using Target APIs to new-comers.
My proposal is replacing:
```python
target = "cuda"
target_host = "llvm"
```
with
```python
target = tvm.target.Target("cuda", host="llvm")
```
This way, we are not deprecating the old ways of using `target` and `target_host` separately, but encouraging the new ways of combining them from the very beginning for a new-comer, which could make life easier if we finally agree to deprecate old APIs.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r593511802
##########
File path: tests/python/integration/test_tuning.py
##########
@@ -131,12 +131,11 @@ def teardown_module():
def get_sample_task(target=tvm.target.cuda(), target_host=None):
+ target = tvm.target.Target(target, target_host)
+ target_host = target.host
Review comment:
The `target` here is used to create a search task in the function. If `target_host` is not `None`, this would be called to update the target and host.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r603708249
##########
File path: include/tvm/target/target.h
##########
@@ -168,5 +175,11 @@ class Target : public ObjectRef {
TVM_DLL void ExitWithScope();
};
+using TargetsMap = Map<Integer, Target>;
+
+TVM_DLL void RefreshHost(Target*, Target*);
+TVM_DLL void RefreshHost(TargetsMap*, Target*);
+TVM_DLL void RefreshHost(Map<Target, IRModule>*, Target*);
Review comment:
I found that I cannot move the part of the helper function into the Target class because it requires a `Map<Target, ObjectRef>` type which cannot be compiled correctly. Therefore, I would put the functions outside the class 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:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r597244567
##########
File path: python/tvm/autotvm/task/relay_integration.py
##########
@@ -122,6 +124,9 @@ def extract_from_multiple_program(mods, params, target, target_host=None, ops=No
env = TaskExtractEnv.get()
+ # merge target and target host
+ target, target_host = refresh_host(target, target_host)
Review comment:
I have checked every occurance of the function and moved them.
--
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] junrushao1994 commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r596567764
##########
File path: include/tvm/target/target.h
##########
@@ -168,5 +175,11 @@ class Target : public ObjectRef {
TVM_DLL void ExitWithScope();
};
+using TargetsMap = Map<Integer, Target>;
+
+TVM_DLL void RefreshHost(Target*, Target*);
+TVM_DLL void RefreshHost(TargetsMap*, Target*);
+TVM_DLL void RefreshHost(Map<Target, IRModule>*, Target*);
Review comment:
1. Functions in the header file needs clear documentation. More specifically, we need to document very clearly that those functions are not encouraged for common use :-)
2. Please list the names of arguments.
3. Also, please consider a name for the function better indicating they are dedicated to legacy behavior. What about `Target::SplitIntoLegacyTargetPair`?
4. Consider moving those functions into static functions of the Target class
##########
File path: include/tvm/target/target.h
##########
@@ -168,5 +175,11 @@ class Target : public ObjectRef {
TVM_DLL void ExitWithScope();
};
+using TargetsMap = Map<Integer, Target>;
Review comment:
Do not expose this in the header file, because it is not an encouraged behavior in the future
##########
File path: src/auto_scheduler/measure_record.cc
##########
@@ -163,8 +163,10 @@ struct Handler<::tvm::auto_scheduler::SearchTaskNode> {
writer->WriteArrayItem(std::string(data.workload_key));
writer->WriteArrayItem(data.target->str());
writer->WriteArrayItem(*data.hardware_params.get());
- if (data.target_host.defined()) {
- writer->WriteArrayItem(data.target_host->str());
+ ::tvm::Target target = data.target, target_host = data.target_host;
Review comment:
nitpick: split into two lines, also perhaps we don't need the prefix "::tvm::"
##########
File path: python/tvm/target/target.py
##########
@@ -494,3 +498,32 @@ def _load_config_dict(config_dict_str):
if not isinstance(key, str):
return None
return config
+
+
+def refresh_host(target, host=None, target_is_key=True):
+ """Helpfer function to return a target and target host after updating each other.
Review comment:
```suggestion
"""A helper function that merges a legacy "target, target_host" pair, then returns the merged target and its host field.
```
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -418,6 +419,9 @@ def set_task(self, task):
def _build_func_common(measure_input, check_gpu=None, cuda_arch=None, build_option=None):
"""Common part for building a configuration"""
target, task, config = measure_input
+
Review comment:
remove the blank line
##########
File path: python/tvm/autotvm/task/relay_integration.py
##########
@@ -122,6 +124,9 @@ def extract_from_multiple_program(mods, params, target, target_host=None, ops=No
env = TaskExtractEnv.get()
+ # merge target and target host
+ target, target_host = refresh_host(target, target_host)
Review comment:
if possible, move this line to the very beginning to each function, so that it could be easier to handle if we want to deprecate target_host in the future
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -130,6 +132,9 @@ def lower(self, mod, target=None, target_host=None):
"""
target = self._update_target(target)
target_host = self._update_target_host(target, target_host)
+
Review comment:
remove the blank line
##########
File path: python/tvm/target/target.py
##########
@@ -494,3 +498,32 @@ def _load_config_dict(config_dict_str):
if not isinstance(key, str):
return None
return config
+
+
+def refresh_host(target, host=None, target_is_key=True):
+ """Helpfer function to return a target and target host after updating each other.
+
+ Parameters
+ ----------
+ target : Union[str, Dict[str, Any], Target]
Review comment:
remove the unnecessary spaces
##########
File path: src/target/target.cc
##########
@@ -375,7 +403,7 @@ Target::Target(const Map<String, ObjectRef>& config) {
Target::Target(Target target, Target host) {
ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
- CHECK(!n->host.defined())
+ CHECK(!n->host.defined() || n->host == host)
Review comment:
Use pointer equality explicitly
```suggestion
CHECK(!n->host.defined() || n->host.same_as(host))
```
##########
File path: python/tvm/target/target.py
##########
@@ -494,3 +498,32 @@ def _load_config_dict(config_dict_str):
if not isinstance(key, str):
return None
return config
+
+
+def refresh_host(target, host=None, target_is_key=True):
Review comment:
let's also move this function as a static function of the Target class and document clearly that it is for legacy target pair. Let's name it consistent to that on the C++ side.
##########
File path: python/tvm/relay/backend/vm.py
##########
@@ -167,6 +172,9 @@ def optimize(self, mod, target=None, target_host=None, params=None):
"""
target = self._update_target(target)
target_host = self._update_target_host(target, target_host)
+
Review comment:
usually we do not insert unnecessary blank lines in the same function
##########
File path: python/tvm/relay/build_module.py
##########
@@ -205,7 +208,7 @@ def _build_module_no_factory(mod, target=None, target_host=None, params=None, mo
This wrapper is suitable to be used from other programming languages as
the runtime::Module can be freely passed between language boundaries.
"""
- return build(mod, target, target_host, params, mod_name).module
+ return build(mod, target, target_host, params=params, mod_name=mod_name).module
Review comment:
i assume we don't need this change?
##########
File path: python/tvm/target/target.py
##########
@@ -494,3 +498,32 @@ def _load_config_dict(config_dict_str):
if not isinstance(key, str):
return None
return config
+
+
+def refresh_host(target, host=None, target_is_key=True):
+ """Helpfer function to return a target and target host after updating each other.
+
+ Parameters
+ ----------
+ target : Union[str, Dict[str, Any], Target]
+ The target or heterogeneous target
+ host : Union[str, Dict[str, Any], Target, None]
+ The target host
+ target_is_key : Bool
Review comment:
```suggestion
target_is_dict_key : bool
```
----------------------------------------------------------------
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] zxybazh commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793079051
> Notes on the tutorial/tests change:
>
> As we are changing user-facing tutorials, we should stick to what is the most readable & encouraged way of using Target APIs to new-comers.
>
> My proposal is replacing:
>
> ```python
> target = "cuda"
> target_host = "llvm"
> ```
>
> with
>
> ```python
> target = tvm.target.Target("cuda", host="llvm")
> ```
>
> This way, we are not deprecating the old ways of using `target` and `target_host` separately, but encouraging the new ways of combining them from the very beginning for a new-comer, which could make life easier if we finally agree to deprecate old APIs.
Good point. I have fixed that in the tutorials and another way of declaring targets like `Target(
"cuda --host=llvm")` was also demonstrated.
----------------------------------------------------------------
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] junrushao1994 commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793080470
@zxybazh Thanks Xiyou for updating it! Let's focus on the encouraged canonical way (i.e. `tvm.target.Target("cuda", host="llvm")`), and do not use other syntactic sugars (e.g. raw string representation)
----------------------------------------------------------------
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] junrushao1994 commented on pull request #7534: [WIP][Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-786873242
Let's also note the potential influence that this change can affect the target string used in AutoTVM/Ansor and avoid introducing any regression.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [WIP][Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r583829327
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -216,7 +216,35 @@ def test_target_host_warning():
attributes fails as expected.
"""
with pytest.raises(ValueError):
- tgt = tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+ tvm.target.Target("cuda --host nvidia/jetson-nano", "llvm")
+
+
+def test_target_host_merge_0():
+ tgt = tvm.target.Target(tvm.target.Target("cuda --host nvidia/jetson-nano"), None)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_53"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 32768
+
+
+def test_target_host_merge_1():
+ tgt = tvm.target.Target("cuda --host llvm")
+ tgt = tvm.target.Target(tgt, tgt.host)
+ assert tgt.kind.name == "cuda"
+ assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_merge_2():
+ with pytest.raises(ValueError):
+ tvm.target.Target(tvm.target.Target("cuda --host llvm"), tvm.target.Target("llvm"))
+
+
+def test_target_host_merge_3():
+ with pytest.raises(ValueError):
+ tvm.target.Target(tvm.target.Target("cuda --host llvm"), 12.34)
if __name__ == "__main__":
Review comment:
Wow, works like a charm. Good point!
----------------------------------------------------------------
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] zxybazh commented on pull request #7534: [WIP][Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-786347109
This is the python script part and a lot of testcases could be added.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r597207631
##########
File path: python/tvm/autotvm/task/relay_integration.py
##########
@@ -122,6 +124,9 @@ def extract_from_multiple_program(mods, params, target, target_host=None, ops=No
env = TaskExtractEnv.get()
+ # merge target and target host
+ target, target_host = refresh_host(target, target_host)
Review comment:
Yeah, that's definitely a good idea. Sometimes it appears the target or target host would be updated in the function. Otherwise I can move it forward as much as I can.
--
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] zxybazh commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793066671
Hi, I have got the tests passed and updated tutorials / tests as well. Please review, thanks! @comaniac @leandron @areusch @junrushao1994 .
----------------------------------------------------------------
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] junrushao1994 commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793062861
Notes on the tutorial/tests change:
As we are changing user-facing tutorials, we should stick to what is the most readable to new-comers. My proposal is:
Replacing:
```python
target = "cuda"
target_host = "llvm"
```
with
```python
target = tvm.target.Target("cuda", host="llvm")
```
----------------------------------------------------------------
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] junrushao1994 merged pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7534:
URL: https://github.com/apache/tvm/pull/7534
--
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] junrushao1994 edited a comment on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-793062861
Notes on the tutorial/tests change:
As we are changing user-facing tutorials, we should stick to what is the most readable to new-comers. My proposal is:
Replacing:
```python
target = "cuda"
target_host = "llvm"
```
with
```python
target = tvm.target.Target("cuda", host="llvm")
```
This way, we are not deprecating the old ways of using `target` and `target_host` separately, but encouraging the new ways of combining them from the very beginning for a new-comer, which could make life easier if we finally agree to deprecate old APIs.
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r597212646
##########
File path: src/auto_scheduler/measure_record.cc
##########
@@ -163,8 +163,10 @@ struct Handler<::tvm::auto_scheduler::SearchTaskNode> {
writer->WriteArrayItem(std::string(data.workload_key));
writer->WriteArrayItem(data.target->str());
writer->WriteArrayItem(*data.hardware_params.get());
- if (data.target_host.defined()) {
- writer->WriteArrayItem(data.target_host->str());
+ ::tvm::Target target = data.target, target_host = data.target_host;
Review comment:
Sure I can split into two lines. In this file specifically, the name space tvm is not used. The prefix is aloso used in other statements as well. Therefore, I'll keep it here 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:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#issuecomment-801265986
@zxybazh would you like to fix the CI issue? Thanks a lot!
----------------------------------------------------------------
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] zxybazh commented on a change in pull request #7534: [Target] Add support for target object with host field compatible with previous api
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7534:
URL: https://github.com/apache/tvm/pull/7534#discussion_r589885852
##########
File path: python/tvm/auto_scheduler/measure.py
##########
@@ -223,8 +223,8 @@ def recover_measure_input(inp, rebuild_state=False):
task = inp.task
new_task = SearchTask(
workload_key=task.workload_key,
- target=task.target,
- target_host=task.target_host,
+ target=tvm.target.Target(task.target, task.target_host),
+ target_host=None,
Review comment:
Right, removed for simplicity.
----------------------------------------------------------------
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