comaniac commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594519337
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen =
composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or
codegen_config
Review comment:
It means the generated codegen_config will be completely overwritten by
the user config, but we should make it like
```python
codegen_config.update(codegen_from_cli["opts"] if codegen_from_cli["opts"]
is not None else {})
```
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen =
composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
I agree that it would be easier to ask all pritition functions return a
single module. However, the init function introduced by #7577 seems not
applicable to this case to me. The config required by TensorRT is used when
building the module; while the init function used by Vitis-AI imports the
required package. I have no idea how could we make it general enough to support
both Vitis-AI and TensorRT.
On the other hand, let a partition function return a tuple of module and
build config seems more straightforward at the first glance, as I cannot think
of a better idea for now. First, partition functions won't be a Relay pass in a
pass sequence (at least for now). Second, it is reasonable that the build
config may need to be constructed during the partition. Separating them to two
APIs may introduce overheads and be more confusing to non TVMC users, which are
still majority in this community.
----------------------------------------------------------------
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]
