[GitHub] [tvm] leandron commented on a change in pull request #7462: [Target] Add target host field for target specification

[GitBox <gi...@apache.org>]

leandron commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578324044



##########
File path: src/target/target.cc
##########
@@ -373,6 +373,15 @@ Target::Target(const Map<String, ObjectRef>& config) {
   data_ = std::move(target);
 }
 
+Target::Target(Target target, Target host) {
+  ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
+  CHECK(!n->host.defined())
+      << "ValueError: Adding a host to a target whose host field has been defined";

Review comment:
       Please correct me if my question doesn't make sense, but am I correct to assume that this check will always pass, as you're adding the field directly to the macro ` TVM_REGISTER_TARGET_KIND`, which is used by all existing targets, and therefore `host` is a field for all of them?

##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -158,6 +155,61 @@ def test_list_kinds():
     assert all(isinstance(target_name, str) for target_name in targets)
 
 
+def test_target_host_tags():
+    tgt = tvm.target.Target("nvidia/jetson-nano", "nvidia/geforce-rtx-2080-ti")
+    assert tgt.kind.name == "cuda"
+    assert tgt.attrs["arch"] == "sm_53"
+    assert tgt.attrs["shared_memory_per_block"] == 49152
+    assert tgt.attrs["max_threads_per_block"] == 1024
+    assert tgt.attrs["thread_warp_size"] == 32
+    assert tgt.attrs["registers_per_block"] == 32768
+    assert tgt.host.kind.name == "cuda"
+    assert tgt.host.attrs["arch"] == "sm_75"
+    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"] == 65536
+
+
+def test_target_host_tag_dict():
+    tgt = tvm.target.Target("nvidia/jetson-nano", {"kind": "llvm"})
+    assert tgt.kind.name == "cuda"
+    assert tgt.attrs["arch"] == "sm_53"
+    assert tgt.attrs["shared_memory_per_block"] == 49152
+    assert tgt.attrs["max_threads_per_block"] == 1024
+    assert tgt.attrs["thread_warp_size"] == 32
+    assert tgt.attrs["registers_per_block"] == 32768
+    assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_single_dict():
+    tgt = tvm.target.Target({"kind": "llvm", "host": "nvidia/jetson-nano"})
+    assert tgt.kind.name == "llvm"
+    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_single_string():
+    tgt = tvm.target.Target("cuda --host llvm")

Review comment:
       (putting this comment here, but this is not comment about this test, just wanted to pin it somewhere with a reference)
   
   I noticed there are some syntax changes in this. If this becomes (with this PR) our preferred way to declare the "target to target host" dependency, should we also update the tutorials to reflect that?

##########
File path: include/tvm/target/target_kind.h
##########
@@ -376,7 +376,8 @@ inline TargetKindRegEntry& TargetKindRegEntry::set_name() {
           .add_attr_option<String>("tag")                         \
           .add_attr_option<String>("device")                      \
           .add_attr_option<String>("model")                       \
-          .add_attr_option<Array<String>>("libs")
+          .add_attr_option<Array<String>>("libs")                 \
+          .add_attr_option<Target>("host")

Review comment:
       I'm just curious on how this plays with existing logic in `build()` at `build_module.py`, which has a `target_host` parameter:
   
   https://github.com/apache/tvm/blob/b7e0cfb6d469c3745ae2195908daadea9c64d87e/python/tvm/relay/build_module.py#L197
   
   Does this mean we now have two ways of declaring `target_host`? If so, are planning to deprecate one of them?
   
   Also, in case the user declares both (_doesn't make much sense, but can be done once this PR is meged_), which one takes precedece, or, should we show an error? I'm referrin to something like `relay.build(target="cuda --host nvidia/jetson-nano", target_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