junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577335256
##########
File path: python/tvm/target/target.py
##########
@@ -86,10 +86,24 @@ def __init__(self, tag_or_str_or_dict):
mfloat-abi : str (optional)
An llvm setting that is one of 'hard' or 'soft' indicating
whether to use
hardware or software floating-point operations.
+ host : Union[str, Dict[str, Any]] (optional)
+ Description for target host. Can be recursive. Similar to
tag_or_str_or_dict.
+ host_tag_or_str_or_dict : Union[str, Dict[str, Any]]
Review comment:
```suggestion
host_tag_or_str_or_dict : Optional[Union[str, Dict[str, Any]]]
```
##########
File path: python/tvm/target/target.py
##########
@@ -86,10 +86,24 @@ def __init__(self, tag_or_str_or_dict):
mfloat-abi : str (optional)
An llvm setting that is one of 'hard' or 'soft' indicating
whether to use
hardware or software floating-point operations.
+ host : Union[str, Dict[str, Any]] (optional)
+ Description for target host. Can be recursive. Similar to
tag_or_str_or_dict.
+ host_tag_or_str_or_dict : Union[str, Dict[str, Any]]
+ Similar to tag_or_str_or_dict but for target host. Can be one of a
literal
+ target host string, a json string describing a configuration, or a
dictionary of
+ configuration options. When using a dictionary or json string to
configure target,
+ the possible values are same as tag_or_str_or_dict.
"""
if not isinstance(tag_or_str_or_dict, (dict, str, Target)):
raise ValueError("target has to be a string or dictionary.")
- self.__init_handle_by_constructor__(_ffi_api.Target,
tag_or_str_or_dict)
+ if host_tag_or_str_or_dict is not None:
+ if not isinstance(host_tag_or_str_or_dict, (dict, str, Target)):
+ raise ValueError("target host has to be a string or
dictionary.")
+ self.__init_handle_by_constructor__(
+ _ffi_api.Target, tag_or_str_or_dict, host_tag_or_str_or_dict
+ )
Review comment:
This trick could make our life easier:
```suggestion
self.__init_handle_by_constructor__(
_ffi_api.Target, Target(tag_or_str_or_dict),
Target(host_tag_or_str_or_dict)
)
```
##########
File path: src/target/target.cc
##########
@@ -456,6 +478,11 @@ void TargetInternal::ConstructorDispatcher(TVMArgs args,
TVMRetValue* rv) {
<< runtime::ArgTypeCode2Str(arg.type_code());
}
return;
+ } else if (args.num_args == 2) {
+ const auto &argt = args[0], &argh = args[1];
+ auto func = PackedFunc(ConstructorDispatcher);
+ *rv = Target(func(argt).AsObjectRef<Target>(),
func(argh).AsObjectRef<Target>());
+ return;
}
LOG(FATAL) << "ValueError: Invalid number of arguments. Expect 1, but gets:
" << args.num_args;
Review comment:
Perhaps we should improve this error message
##########
File path: src/target/target.cc
##########
@@ -456,6 +478,11 @@ void TargetInternal::ConstructorDispatcher(TVMArgs args,
TVMRetValue* rv) {
<< runtime::ArgTypeCode2Str(arg.type_code());
}
return;
+ } else if (args.num_args == 2) {
+ const auto &argt = args[0], &argh = args[1];
+ auto func = PackedFunc(ConstructorDispatcher);
+ *rv = Target(func(argt).AsObjectRef<Target>(),
func(argh).AsObjectRef<Target>());
Review comment:
We only care about the case that both arguments are "Target"
```suggestion
if (args[0]->IsObjectRef<Target>() && args[1]->IsObjectRef<Target>()) {
Target target = args[0];
Target host = args[1];
*rv = Target(target, host);
}
```
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -158,6 +155,44 @@ 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():
Review comment:
Yeah I think the tests are written pretty well. Thanks for the effort!
Let's add a few more, like
`cuda --host llvm`
`cuda --host nvidia/jetson-nano`
##########
File path: src/target/target.cc
##########
@@ -373,6 +380,21 @@ Target::Target(const Map<String, ObjectRef>& config) {
data_ = std::move(target);
}
+Target::Target(const Map<String, ObjectRef>& config, const Map<String,
ObjectRef>& host_config) {
+ ObjectPtr<TargetNode> n =
+ make_object<TargetNode>(*Target(Target(config),
Target(host_config)).get());
+ data_ = std::move(n);
+}
+
+Target::Target(Target target, Target host) {
+ ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
+ if (n->host.defined())
+ throw dmlc::Error(": Error when adding target host to target already with
host");
Review comment:
```suggestion
CHECK(!n->host.defined()) << "ValueError: Adding a host to a target whose
host field has been defined"
```
##########
File path: src/target/target_kind.cc
##########
@@ -219,6 +219,7 @@ TVM_REGISTER_TARGET_KIND("llvm", kDLCPU)
.add_attr_option<Bool>("system-lib")
.add_attr_option<String>("runtime")
.add_attr_option<Bool>("link-params", Bool(false))
+ .add_attr_option<Target>("host")
Review comment:
No need to add them separately, you may add this line to the
`TVM_REGISTER_TARGET_KIND ` macro
----------------------------------------------------------------
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:
[email protected]
