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] Do not auto-build apps when building TVM #9970

Merged
merged 4 commits into from
Jan 24, 2022
Merged

[Hexagon] Do not auto-build apps when building TVM #9970

merged 4 commits into from
Jan 24, 2022

Conversation

kparzysz-quic
Copy link
Contributor

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.

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.

@kparzysz-quic
Copy link
Contributor Author

cc: @mehrdadh

@kparzysz-quic
Copy link
Contributor Author

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`.
@Lunderberg
Copy link
Contributor

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.

  • Three TVM builds are required. (Host, connected Android, connected Hexagon)
  • Each of the three builds has different values for the RPC and static/dynamic options.
  • All three builds must have the same values for several build variables.
    • Hexagon: USE_HEXAGON_SDK and USE_HEXAGON_ARCH
    • Android: USE_ANDROID_TOOLCHAIN
  • All three builds must have be built from the same version of TVM.
  • All three builds must output to the same location (currently $BUILD_DIR/apps/hexagon_*).

Brainstorming on options

  • Option 1: Top-level TVM CMake launches the three independent builds as external projects
    • Current implementation on main.
    • Pro: Developer can run make && run_my_test.py to rebuild all three tests in a single step.
    • Con: Difficult to manage setup.
  • Option 2: Developer launches the three independent builds
    • Current implementation in this PR.
    • Pro: Easier to manage cmake setup.
    • Con: Interactions between the three builds must be handled by each developer, either manually or by writing a wrapper build script.
    • Con: Removes the app from the TVM framework. Compiling TVM is no longer sufficient to use tvm.contrib.hexagon.
  • Option 3: App Cmake launches all three independent builds
    • Pro: Developer can run tests from a single location
    • Pro: Includes occur only in one direction, from the app to the TVM build.
    • Con: Removes the app from the TVM framework. Compiling TVM is no longer sufficient to use tvm.contrib.hexagon.

Between the high-level options, I'd prefer Option 1, since it would maintain the same developer-facing interactions. Thoughts?

@github-actions github-actions bot removed the request for review from a team January 19, 2022 22:57
@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented Jan 19, 2022

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.

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.

Thanks for making this change! Just few minor things to make sure tests are passing.

apps/hexagon_api/CMakeLists.txt Show resolved Hide resolved
# 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")
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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!

apps/hexagon_api/README.md Show resolved Hide resolved
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.

@kparzysz-quic I tested with hardware and it LGTM!

@kparzysz-quic kparzysz-quic merged commit 73bbfbb into apache:main Jan 24, 2022
@kparzysz-quic kparzysz-quic deleted the hexagon-cmake-no-apps branch January 24, 2022 19:08
yuanfz98 pushed a commit to yuanfz98/tvm that referenced this pull request Jan 24, 2022
* [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
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
* [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
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.

4 participants