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

[Hexagon] Deprecate USE_HEXAGON_DEVICE, introduce USE_HEXAGON #11025

Merged
merged 1 commit into from
Apr 18, 2022
Merged

[Hexagon] Deprecate USE_HEXAGON_DEVICE, introduce USE_HEXAGON #11025

merged 1 commit into from
Apr 18, 2022

Conversation

kparzysz-quic
Copy link
Contributor

@kparzysz-quic kparzysz-quic commented Apr 15, 2022

The new cmake flag USE_HEXAGON=[ON|OFF] enables/disables Hexagon support in TVM and TVM runtime. It should be turned on whenever Hexagon support is required, even when compiling TVM runtime for Hexagon itself.

This is one in a series of commits intended to remove offload support, and make the whole-model support the default mode of operation.

With USE_HEXAGON_DEVICE deprecated, offload runtime is not built anymore, so register device_api.hexagon to be same as .v2 (presence of device API is taken as evidence of support for the device in TVM, so this step is necessary).

cc @mehrdadh

The new cmake flag `USE_HEXAGON=[ON|OFF]` enables/disables Hexagon
support in TVM and TVM runtime. It should be turned on _whenever_
Hexagon support is required, even when compiling TVM runtime for
Hexagon itself.

This is one in a series of commits intended to remove offload
support, and make the whole-model support the default mode of
operation.

With `USE_HEXAGON_DEVICE` deprecated, offload runtime is not built
anymore, so register `device_api.hexagon` to be same as `.v2`
(presence of device API is taken as evidence of support for the
device in TVM, so this step is necessary).
@mehrdadh
Copy link
Member

are you keeping src/runtime/hexagon/android/hexagon_device_api.cc because of apps/hexagon_launcher?

@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented Apr 15, 2022

Not really, I just broke the process up in a few steps:

  1. Replace USE_HEXAGON_DEVICE with USE_HEXAGON.
  2. Delete unnecessary files.
  3. Final cleanup, specifically rename device_api.hexagon.v2 to device_api.hexagon, and remove all "reset_device_api" functions and the like.

Edit: The reason is that I didn't want to make too many changes at the same time, so that each commit is focused on a more-or-less one thing.

Copy link
Contributor

@csullivan csullivan left a comment

Choose a reason for hiding this comment

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

LGTM with a few suggestions for future improvements

"-DCMAKE_CXX_STANDARD=14"
"-DUSE_HEXAGON_ARCH=${USE_HEXAGON_ARCH}"
"-DUSE_HEXAGON=ON"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Place USE_HEXAGON=ON before other hexagon cmake variable definitions

@@ -40,8 +40,8 @@ tvm_option(USE_SPIRV_KHR_INTEGER_DOT_PRODUCT "whether enable SPIRV_KHR_DOT_PRODU
tvm_option(USE_METAL "Build with Metal" OFF)
tvm_option(USE_ROCM "Build with ROCM" OFF)
tvm_option(ROCM_PATH "The path to rocm" /opt/rocm)
tvm_option(USE_HEXAGON_DEVICE "Build with Hexagon device support in TVM runtime" OFF)
tvm_option(USE_HEXAGON_SDK "Path to the Hexagon SDK root (required for Hexagon support in TVM runtime or for building TVM runtime for Hexagon)" /path/to/sdk)
tvm_option(USE_HEXAGON "Build with Hexagon support" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Some TVM config options provide [ON|OFF|PATH_TO_SDK], with ON indicating to search for the SDK in common directories and fail otherwise.

@@ -35,6 +35,7 @@ ExternalProject_Add(x86_tvm_runtime_rpc
"-DUSE_LIBBACKTRACE=OFF"
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't catch the motivation on introduction of hexagon_api earlier this year, but to add to the backlog it would be nice if the hexagon_api was built automatically as part of a USE_HEXAGON=ON build. It's easy for users to forget to recompile the hexagon_api after making changes and only rebuilding tvm.

Copy link
Member

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mehrdadh mehrdadh merged commit 28aac0e into apache:main Apr 18, 2022
@kparzysz-quic kparzysz-quic deleted the cmake-use-hexagon branch April 18, 2022 16:36
Lucien0 pushed a commit to Lucien0/tvm that referenced this pull request Apr 19, 2022
…#11025)

The new cmake flag `USE_HEXAGON=[ON|OFF]` enables/disables Hexagon
support in TVM and TVM runtime. It should be turned on _whenever_
Hexagon support is required, even when compiling TVM runtime for
Hexagon itself.

This is one in a series of commits intended to remove offload
support, and make the whole-model support the default mode of
operation.

With `USE_HEXAGON_DEVICE` deprecated, offload runtime is not built
anymore, so register `device_api.hexagon` to be same as `.v2`
(presence of device API is taken as evidence of support for the
device in TVM, so this step is necessary).
altanh pushed a commit to altanh/tvm that referenced this pull request Apr 28, 2022
…#11025)

The new cmake flag `USE_HEXAGON=[ON|OFF]` enables/disables Hexagon
support in TVM and TVM runtime. It should be turned on _whenever_
Hexagon support is required, even when compiling TVM runtime for
Hexagon itself.

This is one in a series of commits intended to remove offload
support, and make the whole-model support the default mode of
operation.

With `USE_HEXAGON_DEVICE` deprecated, offload runtime is not built
anymore, so register `device_api.hexagon` to be same as `.v2`
(presence of device API is taken as evidence of support for the
device in TVM, so this step is necessary).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants