[GitHub] [tvm] junrushao1994 edited a comment on pull request #7462: [Target] Add target host field for target specification

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

junrushao1994 edited a comment on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781584977


   Hey @leandron, thank you for the review!
   
   Per the [Target RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844), this PR implement the logic that folds `target_host` into the `target` class, so that is why we added a `host` field in the `TargetNode` class.
   
   > Does this mean we now have two ways of declaring target_host? 
   
   Yes. We can create a `target` with a single dictionary containing the `host` field (the new canonical), or with two `Target`s combined (syntactic sugar I), or with a target string like `cuda --host=llvm` (syntactic sugar II).
   
   This design is a solution to keep perfect backward compatibility. In your example, right now, the signature of the build API is:
   
   ```python
   def build(..., target, target_host...): 
   ```
   
   To be compatible with the existing API without changing them, our refactoring plan is to create a single `target_with_host = Target(target, target_host)`, and proceed with the subsequent workflow. This way could work with
   1) `target` contains the `host` field, and `target_host` is None - we use `target.host` as `target_with_host.host`
   2) `target` doesn't contain the `host` field, and `target_host` is None - `target_with_host.host` will be None 
   3) `target` contains the `host` field, and `target_host` is not None - we will error out
   4) `target` doesn't contain the `host` field, and `target_host` is not None - `target_with_host.host` will be `target_host`
   
   > If so, are planning to deprecate one of them?
   
   Since our solution keeps perfect backward compatibility, we are still deciding whether we really need deprecation at this time point. Even if we finally decided to deprecate the current API and move to a unified target in the function signature, I think we need to keep it for 2 release cycles so that the community could really get onboard.
   
   > 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?
   
   Yeah. We are quite overwhelmed by the upcoming TensorIR upstreaming work. After that, I should really spend some time on doc improvement, etc.
   
   
   
   


----------------------------------------------------------------
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