-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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] Do not auto-build apps when building TVM #9970
[Hexagon] Do not auto-build apps when building TVM #9970
Conversation
cc: @mehrdadh |
This replaces #9904. |
The Hexagon cmakes have recently become unwieldy due to a complex network of dependencies between various automatically built components. This was in large part because of trying to automatically build some apps, which then tried to build TVM runtimes again, but with their own configurations. This patch removes the ability to automatically build any Hexagon- -related apps from the main TVM build. The following cmake options are now deprecated: - `USE_HEXAGON_LAUNCHER` - `USE_HEXAGON_PROXY_RPC` In order to build the binaries needed for HexagonLauncher from tvm.contrib.hexagon: - Build TVM+runtime for x86, with codegen for Hexagon enabled. This can be done via `USE_HEXAGON_DEVICE=sim` or `target`. - Build Android runtime and tvm_rpc with `-DUSE_RPC=ON`, `-DUSE_CPP_RPC=ON`, and `-DUSE_HEXAGON_RPC=ON`. - Build Hexagon runtime with `-DUSE_HEXAGON_RPC=ON`, and `-DBUILD_STATIC_RUNTIME=ON`.
I like the idea of simplifying the build process, but I'm worried that this would make it more difficult for developers to test changes. This PR would reduce the complexity of the existing cmake setup, but I think it would result in passing that complexity to the developer to manually handle. Build details, as I understand them.
Brainstorming on options
Between the high-level options, I'd prefer Option 1, since it would maintain the same developer-facing interactions. Thoughts? |
I chatted with Mehrdad and we're going with an app that will build the Hexagon RPC support, i.e. option 3 in your list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change! Just few minor things to make sure tests are passing.
apps/hexagon_api/CMakeLists.txt
Outdated
# USE_HEXAGON_TOOLCHAIN (Path to Hexagon toolchain ending with "Tools") | ||
|
||
set(TVM_SOURCE_DIR "${CMAKE_SOURCE_DIR}/../..") | ||
set(HEXAGON_API_BINARY_DIR "${CMAKE_BINARY_DIR}/hexagon_rpc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change HEXAGON_API_BINARY_DIR to this to be compatible with current HexagonLauncher design:
set(HEXAGON_API_BINARY_DIR "${CMAKE_BINARY_DIR}/../../../build/hexagon_rpc")
This is with the assumption of building this CmakeLists.txt under apps/hexagon_api/build
. Not the best solution, but we need to make sure everything is copied to build/hexagon_rpc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be really fragile. Instead I propose a flag USE_OUTPUT_BINARY_DIR
that could be set to any location, including .../build/hexagon_rpc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kparzysz-quic makes sense. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kparzysz-quic I tested with hardware and it LGTM!
* [Hexagon] Do not auto-build apps when building TVM The Hexagon cmakes have recently become unwieldy due to a complex network of dependencies between various automatically built components. This was in large part because of trying to automatically build some apps, which then tried to build TVM runtimes again, but with their own configurations. This patch removes the ability to automatically build any Hexagon- -related apps from the main TVM build. The following cmake options are now deprecated: - `USE_HEXAGON_LAUNCHER` - `USE_HEXAGON_PROXY_RPC` In order to build the binaries needed for HexagonLauncher from tvm.contrib.hexagon: - Build TVM+runtime for x86, with codegen for Hexagon enabled. This can be done via `USE_HEXAGON_DEVICE=sim` or `target`. - Build Android runtime and tvm_rpc with `-DUSE_RPC=ON`, `-DUSE_CPP_RPC=ON`, and `-DUSE_HEXAGON_RPC=ON`. - Build Hexagon runtime with `-DUSE_HEXAGON_RPC=ON`, and `-DBUILD_STATIC_RUNTIME=ON`. * Add README.md * Restart CI * Add optional variable to set output directory
* [Hexagon] Do not auto-build apps when building TVM The Hexagon cmakes have recently become unwieldy due to a complex network of dependencies between various automatically built components. This was in large part because of trying to automatically build some apps, which then tried to build TVM runtimes again, but with their own configurations. This patch removes the ability to automatically build any Hexagon- -related apps from the main TVM build. The following cmake options are now deprecated: - `USE_HEXAGON_LAUNCHER` - `USE_HEXAGON_PROXY_RPC` In order to build the binaries needed for HexagonLauncher from tvm.contrib.hexagon: - Build TVM+runtime for x86, with codegen for Hexagon enabled. This can be done via `USE_HEXAGON_DEVICE=sim` or `target`. - Build Android runtime and tvm_rpc with `-DUSE_RPC=ON`, `-DUSE_CPP_RPC=ON`, and `-DUSE_HEXAGON_RPC=ON`. - Build Hexagon runtime with `-DUSE_HEXAGON_RPC=ON`, and `-DBUILD_STATIC_RUNTIME=ON`. * Add README.md * Restart CI * Add optional variable to set output directory
The Hexagon cmakes have recently become unwieldy due to a complex network of dependencies between various automatically built components. This was in large part because of trying to automatically build some apps, which then tried to build TVM runtimes again, but with their own configurations.
This patch removes the ability to automatically build any Hexagon-related apps from the main TVM build. The following cmake options are now deprecated:
USE_HEXAGON_LAUNCHER
USE_HEXAGON_PROXY_RPC
In order to build the binaries needed for HexagonLauncher from
tvm.contrib.hexagon
:USE_HEXAGON_DEVICE=sim
ortarget
.-DUSE_RPC=ON
,-DUSE_CPP_RPC=ON
, and-DUSE_HEXAGON_RPC=ON
.-DUSE_HEXAGON_RPC=ON
, and-DBUILD_STATIC_RUNTIME=ON
.Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.