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:
[email protected]
