Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TVMC][VitisAI] Enable Vitis AI target through TVMC #7577

Merged
merged 13 commits into from
Apr 7, 2021

Conversation

jtuyls
Copy link
Contributor

@jtuyls jtuyls commented Mar 3, 2021

This adds Vitis AI as a composite target to TVMC. As we need access to the CLI options in the partitioning function for the Vitis AI target, this PR updates the TVMC compiler and tuner interface to pass those options to the partitioning functions. The Arm Compute Lib and Ethos-N partitioning functions are adjusted accordingly.

Documentation
I think we should document the use of these targets but not sure what the best place is. For example, we can add documentation to https://tvm.apache.org/docs/tutorials/get_started/tvmc_command_line_driver.html or in the target deploy and integration documentation (for example here: https://tvm.apache.org/docs/deploy/vitis_ai.html). Or we can do both?

CI
The following PR should be merged first for tests to pass: #7575

@leandron @comaniac @mbaret

@comaniac comaniac self-assigned this Mar 3, 2021
python/tvm/contrib/target/vitis_ai.py Outdated Show resolved Hide resolved
python/tvm/contrib/target/vitis_ai.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/composite_target.py Outdated Show resolved Hide resolved
python/tvm/relay/op/contrib/vitis_ai.py Outdated Show resolved Hide resolved
python/tvm/relay/op/contrib/vitis_ai.py Outdated Show resolved Hide resolved
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM, my only concern is with the already overused term target, that if we can find some other term to make it clear, it would be good. BTW, @jtuyls thanks for the effort to implement TVMC support.

Apart from that, some test nits.

python/tvm/relay/op/contrib/vitis_ai.py Outdated Show resolved Hide resolved
python/tvm/contrib/target/vitis_ai.py Outdated Show resolved Hide resolved
tests/python/driver/tvmc/test_compiler.py Outdated Show resolved Hide resolved
tests/python/driver/tvmc/test_compiler.py Outdated Show resolved Hide resolved
@comaniac comaniac added the status: need update need update based on feedbacks label Mar 4, 2021
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM, two minor comments.

docs/deploy/vitis_ai.rst Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/composite_target.py Outdated Show resolved Hide resolved
@jtuyls
Copy link
Contributor Author

jtuyls commented Mar 8, 2021

@leandron Do you have any thoughts on documenting TVMC target usage?

Documentation
I think we should document the use of these targets but not sure what the best place is. For example, we can add documentation to https://tvm.apache.org/docs/tutorials/get_started/tvmc_command_line_driver.html or in the target deploy and integration documentation (for example here: https://tvm.apache.org/docs/deploy/vitis_ai.html). Or we can do both?

@leandron
Copy link
Contributor

leandron commented Mar 8, 2021

@leandron Do you have any thoughts on documenting TVMC target usage?

Documentation
I think we should document the use of these targets but not sure what the best place is. For example, we can add documentation to https://tvm.apache.org/docs/tutorials/get_started/tvmc_command_line_driver.html or in the target deploy and integration documentation (for example here: https://tvm.apache.org/docs/deploy/vitis_ai.html). Or we can do both?
I agree this is

Yes, I agree this is something we need to improve. Actually in general for targets in TVM it is not easy to obtain meta-information without inspecting the code.

Ideally, I'd like to make the targets self-documenting, so that we don't need to update a page and keep it in sync with changes we do in the code, and users can find useful help on the --help option - it is fine if you have your own documentation page, to write about the options, but I'm not sure this should be our official documentation. So that would be my recommendation for now.

I was planning to submit a separate PR, making --help list the available TVM targets plus our composite targets here. This one is very easy (yet not 100% complete as a feature), but I think should be done in a separate PR.

For the inner options, such as -mcpu for llvm, and -dpu for vitis-ai, I think it would require a bit of refactoring, to make that an exposed API in TVM level, just consuming that on TVMC.

Concretely, I don't think this should block your PR here. But certainly is a very important topic to start a broader discussion. (cc @masahi @comaniac @junrushao1994)

@jtuyls
Copy link
Contributor Author

jtuyls commented Mar 8, 2021

For the inner options, such as -mcpu for llvm, and -dpu for vitis-ai, I think it would require a bit of refactoring, to make that an exposed API in TVM level, just consuming that on TVMC.

Concretely, I don't think this should block your PR here. But certainly is a very important topic to start a broader discussion.

I think it would be very useful to solve this target documentation issue in general and proceeding in that manner is better than relying on hard to maintain documentation. So I definitely agree this shouldn't block this PR but I think it would be good to start/continue looking at this so we can make these options transparent to the user.

docs/deploy/vitis_ai.rst Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/composite_target.py Outdated Show resolved Hide resolved
@leandron
Copy link
Contributor

@jtuyls with the merge of #7299 there might be a come minor conflicts with the repo now.

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please fix the CI and let's merge this PR.

@comaniac comaniac added status: accepted and removed status: need update need update based on feedbacks labels Mar 17, 2021
@jtuyls
Copy link
Contributor Author

jtuyls commented Mar 17, 2021

@comaniac The ci docker will have be updated first for the changes in PR #7575 to take effect.

@jtuyls
Copy link
Contributor Author

jtuyls commented Apr 7, 2021

@comaniac With the CI docker update and lazy loading the pyxir package also inside the the Vitis AI partitioning pass, the tests are now passing. Could you have another look?

@comaniac comaniac merged commit e426458 into apache:main Apr 7, 2021
@comaniac
Copy link
Contributor

comaniac commented Apr 7, 2021

Thanks @jtuyls @leandron

@leandron
Copy link
Contributor

leandron commented Apr 8, 2021

Thanks @jtuyls

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* Enable Vitis AI target through TVMC & change PassContext API's

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Change Vitis AI  API to  & address comments & fix linter issues

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Leandro Nunes <[email protected]>

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Cody Yu <[email protected]>

* Add Vitis AI initiliazation to separate init config in TVMC composite target registry

* Lazy load pyxir package in Vitis AI codegen to avoid hard dependency for TVMC

* Fix TVMC Vitis AI test for compiler.compile_model API change

* Lazy load pyxir package in Vitis AI partitioning pass

Co-authored-by: Jorn Tuyls <[email protected]>
Co-authored-by: Cody Yu <[email protected]>
Co-authored-by: Leandro Nunes <[email protected]>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* Enable Vitis AI target through TVMC & change PassContext API's

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Change Vitis AI  API to  & address comments & fix linter issues

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Leandro Nunes <[email protected]>

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Cody Yu <[email protected]>

* Add Vitis AI initiliazation to separate init config in TVMC composite target registry

* Lazy load pyxir package in Vitis AI codegen to avoid hard dependency for TVMC

* Fix TVMC Vitis AI test for compiler.compile_model API change

* Lazy load pyxir package in Vitis AI partitioning pass

Co-authored-by: Jorn Tuyls <[email protected]>
Co-authored-by: Cody Yu <[email protected]>
Co-authored-by: Leandro Nunes <[email protected]>
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* Enable Vitis AI target through TVMC & change PassContext API's

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Change Vitis AI  API to  & address comments & fix linter issues

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Leandro Nunes <[email protected]>

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Cody Yu <[email protected]>

* Add Vitis AI initiliazation to separate init config in TVMC composite target registry

* Lazy load pyxir package in Vitis AI codegen to avoid hard dependency for TVMC

* Fix TVMC Vitis AI test for compiler.compile_model API change

* Lazy load pyxir package in Vitis AI partitioning pass

Co-authored-by: Jorn Tuyls <[email protected]>
Co-authored-by: Cody Yu <[email protected]>
Co-authored-by: Leandro Nunes <[email protected]>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
* Enable Vitis AI target through TVMC & change PassContext API's

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Update python/tvm/contrib/target/vitis_ai.py

Co-authored-by: Cody Yu <[email protected]>

* Change Vitis AI  API to  & address comments & fix linter issues

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Leandro Nunes <[email protected]>

* Update docs/deploy/vitis_ai.rst

Co-authored-by: Cody Yu <[email protected]>

* Add Vitis AI initiliazation to separate init config in TVMC composite target registry

* Lazy load pyxir package in Vitis AI codegen to avoid hard dependency for TVMC

* Fix TVMC Vitis AI test for compiler.compile_model API change

* Lazy load pyxir package in Vitis AI partitioning pass

Co-authored-by: Jorn Tuyls <[email protected]>
Co-authored-by: Cody Yu <[email protected]>
Co-authored-by: Leandro Nunes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants