[TVMC] Add composite target passes for compilation and tuning #7304

leandron · 2021-01-18T16:34:55Z

This adds composite target integration on TVMC.

Extends --target syntax to cover multiple targets for compilation and tuning
Add a new tvm.driver.tvmc.composite_target module to implement custom codegen passes into TVMC
Provide implementation to integrate TVMC with Codegen for Arm Ethos-N NPU

Change-Id: Iaee53fe22f0c14eb4e4c8ec47e72bade0c5e32cc

leandron · 2021-01-18T16:38:44Z

This is the relates to https://discuss.tvm.apache.org/t/byoc-partitioning-support-on-tvmc/8901, in which we can discuss about the motivations behind this PR.

manupak

Few nits and suggestions! See if you agree.

python/tvm/driver/tvmc/byoc.py

tests/python/driver/tvmc/test_compiler.py

python/tvm/driver/tvmc/compiler.py

python/tvm/driver/tvmc/byoc.py

comaniac · 2021-02-05T23:47:51Z

python/tvm/driver/tvmc/common.py


    # TODO(@leandron) We don't have an API to collect a list of supported
    #       targets yet
    logger.debug("creating target from input: %s", target)

-    return tvm.target.Target(target)
+    return tvm.target.Target(target), extra_codegens


So only plain text target supports composite target? Seems doesn't make sense to me.

I feel the JSON support in general from TVMC is something that we should review. The JSON is something we should be hiding from users in the command line, and be using internally as an API protocol.

The inline format as in --target={ json_formatted_string } is not something end-users really need IMHO. The <path to json> requires a lot of internal knowledge about how the internal APIs work, so it is a very advanced developer option, on a format not really documented AFAIK for end-users.

So, after a lot of thinking in this one, I think we have two options:
a. We can keep JSON support, to talk directly to the Target API, and hence only supporting what the API supports (it would be something like a dev. option), or
b. We hide the JSON support from the command-line end-user (removing it on a separate PR), and when support for composite targets appears from an API point of view, we keep the user-friendly syntax from the command line, and under-the-hood generate the appropriate JSON to fulfill what the user asked.

So, as a plan, I suggest we go with a for now, and open a broader discussion to potentially pursue b if there is agreements.

I don't really agree that JSON should hide from end-users. According to the target specification plan, we will encourage everyone to use a simple string "tag" to specify defined targets, including composite targets. Meanwhile, JSON will become the major format of specifying a target in addition to tag. As a result, --target=llvm -mcpu=... won't be a recommended format anymore.

However, since we haven't deprecated the current string target mechanism, it's also fine for me if you say this is just a temporary solution. We could align the semantic to the new target system (plain text -> tag, JSON -> full target specification) when it is there.

cc @junrushao1994

Does this refer to the RFC at https://discuss.tvm.apache.org/t/rfc-composite-target/7744? I see your point for the internal APIs, but having the target exclusively in JSON makes prototyping and experimenting with different targets and combinations of targets, unnecessarily difficult from a command-line point of view.

From the JSON syntax I read in the Discuss thread above, I couldn't find something in there that we can't express in the format of an inline target at the moment - I might be wrong. Concretely, it represents a list of a sorted list of devices, and a fallback target host.

Specifically for TVMC, I feel the solution is to come up with a way the the command line do the work of converting inline targets, to satisfy the internal APIs with the JSON format it needs, as well as accept the JSON specification. I think we should go on that direction.

It looks more meaningful for an end user familiar with any other compiler, to type: --target="ethos-n77, opencl, llvm", rather than encode that into a target_spec.json and use --target=target_spec.json or even to have --target="{ 'kind': 'composite', 'target_host': 'llvm', 'devices': [{'kind': 'ethos-n77'}, {'kind': 'opencl'}]}".

It is TVMC role to - when support arrives - to get from --target="ethos-n77, opencl, llvm", and end-up with { 'kind': 'composite', 'target_host': 'llvm', 'devices': [{'kind': 'ethos-n77'}, {'kind': 'opencl'}]}" to satisfy the internal APIs.

@comaniac @junrushao1994 what do you think?

Im kind of inclined to agree with @leandron.

For me, apart from the syntax, JSON provides a hierarchical target structure that is valuable for internal tvm components. I think we need to think hard whether this hierarchical structure provides a real degree of freedom to the user -- For e.g., should the user be concerned about target_host and kind ? if so what would that enable the user from doing something different from the command line.

I guess my concern is given a set of target kinds and a host, is there a true degree of freedom in which the composite target could be assembled (differently, apart from offloading priority) that means a different thing for tvm. If not my humble opinion is that there is no value exposing this interface to the user. WDYT ?

If you have something in mind, please do share an example where this logic fails -- happy to discuss.

It seems to me that the point we should discuss here is not the TVM target system. The discussion about whether target system should use hierarchical structure or not should be in a separate thread.

Instead, what I always care is to let the commandline interface align to the core system behavior as possible to reduce 1) the possible future discrepancy and maintenance efforts. Specifically, do we need to change the target processing logic in TVMC frequently if there's something changed in the target system (e.g., new attribute, new target, etc)?

Well, agreed. Here I did not mean to discuss the agreed composite target. (I think it could be quite useful to have it accessible in the PassContext :) )

So the matter of constructing of hierarchical target should be an internal choice of tvm -- whether the TargetRegistry offers a constructor or TVMC does the translation. However, what we should decide is what we are going to provide the user with ? and that should be intuitive and complexities should be justified for the different usecase it opens up for the user.

So, once we decide on that, I think it may sound this conversion should be handled by TargetRegistry as it is aware of the details a flat target(and kinds) and how it should be translated to the hierarchical target that is eventually stored in PassContext, I suppose. Therefore, if there is a unique 1:1 mapping to a hierarchical target from a flat target, I think flat target might be preferred from TVMC.

But then again, I might be wrong if there is 1:many mapping to a hierarchical target from a flat target where use wants to pick one (this what I meant by a useful degree of freedom). Do you have something in mind to that regard ?

It seems to me that the point we should discuss here is not the TVM target system. The discussion about whether target system should use hierarchical structure or not should be in a separate thread.

Agreed. The main point in discussion (that I tried exemplifying in my comment above) is whether or not the decision of having the internal Target API to accept only JSON or a tag as inputs, and making that the only way to specify targets on TVMC, makes the usability of the command line tool to be good enough for the end-user.

Having the inline JSON input directly into the tool makes it overly complicated (from the usual command-line tool user expectations). The tags system is a good shortcut to save the hassle of typing JSON, however it lacks the flexibility, as we can't pass parameters to them, nor change order of targets without going to the same hassle of JSON maintenance and editing.

This is why I think we will need this translation from an inline format to the internal JSON format that will be passed to the Target API. For now, to support new codegens, we'll need to do that registration process, as proposed in this PR.

the possible future discrepancy and maintenance efforts. Specifically, do we need to change the target processing logic in TVMC frequently if there's something changed in the target system (e.g., new attribute, new target, etc)?

In the current JSON schema, we can map all that is being proposed into an inline sorted list of target, as string. If there are global top level "new attributes" to the target specification, they will correspond to top level --new-attribute <value> options (and will require the implementation of the new attribute accordingly). If options are nested into the targets themselves, they will be part of the existing --target option.

Specifically for new targets, once support for the partition comes from the Target API, we can keep the current syntax and just move it be processes by the Target API, and we'll just need to maintain the syntax-sugar for the end user.

python/tvm/driver/tvmc/common.py

python/tvm/driver/tvmc/compiler.py

comaniac · 2021-02-06T00:01:44Z

cc @zhiics @junrushao1994 we really need to push the TVM target...

junrushao · 2021-02-06T00:03:21Z

Yeah @zxybazh and I are actively working towards this :-)

zhiics · 2021-02-06T00:58:56Z

@comaniac you meant composite target, right?

comaniac · 2021-02-06T01:04:26Z

@comaniac you meant composite target, right?

Yeah both composite target and target tags.

python/tvm/driver/tvmc/compiler.py

python/tvm/driver/tvmc/composite_target.py

comaniac · 2021-02-16T00:41:58Z

python/tvm/driver/tvmc/byoc.py

+
+
+@register_codegen(kind="ethos-n77")
+class TVMCEthosNCodegen(TVMCCodegen):


In fact, this is exactly the reason I would like to add others -- make sure this registration mechanism works for not only one or certain codegens. If adding TensorRT would change the way we register the codegen, the registry should be improved. Of course, it's also possible to keep the current registry map and change the TensorRT partition function. It would be better to at least figure out what to change in this PR.

cc @trevor-m

comaniac

Overall LGTM. Two left comments:

The function naming.
Clarify the status of adding TensorRT.

* Extend --target syntax to cover multiple targets for compilation and tuning * Add a new composite_target module to implement custom codegen passes into TVMC * Provide implementation to integrate TVMC, to target Arm Ethos-N NPU and Compute Library for the Arm Architecture (ACL) Change-Id: Iaee53fe22f0c14eb4e4c8ec47e72bade0c5e32cc

leandron · 2021-02-19T09:14:42Z

Overall LGTM. Two left comments:
1. The function naming.

Updated the names for the ones suggested here.

2. Clarify the status of adding TensorRT.

I'd really prefer to poke someone (maybe @trevor-m) to add TensorRT on a separate patch - see my notes about the default values on the partition function in #7304 (comment).

There is also vitis-ai that comes to mind, for example, we could poke @jtuyls to see whether there is interest in adding it on a separate patch, once the mechanism here is merged.

@comaniac can you also have a look on the patch again, when you have some time?

comaniac

LGTM.
Gentle ping @trevor-m @jtuyls to take a look at the codegen registration mechanism.

comaniac · 2021-02-19T23:39:35Z

Merged first to avoid unnecessary blocking. In case the codegen registration has to be improved for other backends, we will do that in the follow-up PRs.

Thanks @leandron @manupa-arm

…#7304) * Extend --target syntax to cover multiple targets for compilation and tuning * Add a new composite_target module to implement custom codegen passes into TVMC * Provide implementation to integrate TVMC, to target Arm Ethos-N NPU and Compute Library for the Arm Architecture (ACL) Change-Id: Iaee53fe22f0c14eb4e4c8ec47e72bade0c5e32cc

ZihengJiang added the status: need review label Jan 20, 2021

manupak reviewed Feb 5, 2021

View reviewed changes

python/tvm/driver/tvmc/byoc.py Outdated Show resolved Hide resolved

tests/python/driver/tvmc/test_compiler.py Show resolved Hide resolved

python/tvm/driver/tvmc/compiler.py Show resolved Hide resolved

comaniac requested changes Feb 6, 2021

View reviewed changes

leandron mentioned this pull request Feb 6, 2021

[TVMC] Allow manual shape specification in tvmc #7366

Merged

leandron changed the title ~~[TVMC] Add custom codegen (BYOC) passes for compilation and tuning~~ [TVMC] Add composite target passes for compilation and tuning Feb 15, 2021

comaniac reviewed Feb 16, 2021

View reviewed changes

comaniac approved these changes Feb 19, 2021

View reviewed changes

comaniac merged commit d16f282 into apache:main Feb 19, 2021

leandron mentioned this pull request Feb 22, 2021

[Target] Add target host field for target specification #7462

Merged

leandron deleted the byoc_tvmc branch February 22, 2021 10:35

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TVMC] Add composite target passes for compilation and tuning #7304

[TVMC] Add composite target passes for compilation and tuning #7304

leandron commented Jan 18, 2021 •

edited

leandron commented Jan 18, 2021

manupak left a comment

comaniac Feb 5, 2021

leandron Feb 15, 2021

comaniac Feb 16, 2021

leandron Feb 16, 2021

manupak Feb 16, 2021 •

edited

comaniac Feb 16, 2021

manupak Feb 16, 2021

leandron Feb 16, 2021

comaniac commented Feb 6, 2021

junrushao commented Feb 6, 2021

zhiics commented Feb 6, 2021

comaniac commented Feb 6, 2021 •

edited

comaniac Feb 16, 2021

comaniac left a comment

leandron commented Feb 19, 2021

comaniac left a comment

comaniac commented Feb 19, 2021



		@register_codegen(kind="ethos-n77")
		class TVMCEthosNCodegen(TVMCCodegen):

[TVMC] Add composite target passes for compilation and tuning #7304

[TVMC] Add composite target passes for compilation and tuning #7304

Conversation

leandron commented Jan 18, 2021 • edited

leandron commented Jan 18, 2021

manupak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manupak Feb 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comaniac commented Feb 6, 2021

junrushao commented Feb 6, 2021

zhiics commented Feb 6, 2021

comaniac commented Feb 6, 2021 • edited

Choose a reason for hiding this comment

comaniac left a comment

Choose a reason for hiding this comment

leandron commented Feb 19, 2021

comaniac left a comment

Choose a reason for hiding this comment

comaniac commented Feb 19, 2021

leandron commented Jan 18, 2021 •

edited

manupak Feb 16, 2021 •

edited

comaniac commented Feb 6, 2021 •

edited