Skip to content

fix ccache#11189

Merged
areusch merged 1 commit intoapache:mainfrom
cconvey:fix-ccache
May 17, 2022
Merged

fix ccache#11189
areusch merged 1 commit intoapache:mainfrom
cconvey:fix-ccache

Conversation

@cconvey
Copy link
Copy Markdown
Contributor

@cconvey cconvey commented Apr 29, 2022

[build] Fix ccache logic in build system.

  • Fix several bugs in existing ccache logic.

  • When building for Hexagon, ensure that
    related CMake variables propagate through
    CMake ExternalProject_add calls.

  • Add user documentation for enabling ccache
    in TVM builds.

@cconvey cconvey force-pushed the fix-ccache branch 5 times, most recently from 3fc0f39 to d305099 Compare April 29, 2022 22:31
@cconvey cconvey changed the title [WIP] changes to fix ccache changes to fix ccache Apr 29, 2022
@cconvey
Copy link
Copy Markdown
Contributor Author

cconvey commented Apr 29, 2022

CC @csullivan @mehrdadh @adstraw

Comment thread CMakeLists.txt
@cconvey cconvey changed the title changes to fix ccache fix ccache Apr 29, 2022
@cconvey cconvey force-pushed the fix-ccache branch 3 times, most recently from 0e3d99d to f49e714 Compare May 3, 2022 15:43
Copy link
Copy Markdown
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @cconvey , one comment here

Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread apps/hexagon_api/CMakeLists.txt
@cconvey cconvey marked this pull request as draft May 3, 2022 19:22
Copy link
Copy Markdown
Contributor

@adstraw adstraw left a comment

Choose a reason for hiding this comment

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

Overall LGTM. A few nits.

Comment thread CMakeLists.txt Outdated
Comment thread docs/install/from_source.rst
- Remove TVM's `USE_CCACHE` option in favor
  of CMake's built-in `CMAKE_C_COMPILER_LAUNCHER`
  and `CMAKE_CXX_COMPILER_LAUNCHER` variables.

  This eliminates a significant source of
  complexity, especially:

  - TVM's CI scripts, which use `sccache`
    instead of `ccache`, and

  - calls to `ExternalProject_add` in TVM's CMake logic.

- Ensure that `CMAKE_C[XX]_COMPILER_LAUNCHER` variables
  are passed through in all `ExternalProject_add` calls.

- Update user documentation.
@cconvey cconvey marked this pull request as ready for review May 15, 2022 18:20
@cconvey cconvey requested a review from mehrdadh May 15, 2022 20:16
@cconvey
Copy link
Copy Markdown
Contributor Author

cconvey commented May 15, 2022

@mehrdadh : Ready for merge if/when you're happy.

Copy link
Copy Markdown
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! Waiting for @areusch to take another look.

@mehrdadh mehrdadh requested a review from areusch May 16, 2022 20:32
Copy link
Copy Markdown
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @cconvey ! left one suggestion which could be a follow-on for sure

export TVM_LOG_DEBUG="ir/transform.cc=1;relay/ir/transform.cc=1"

- TVM requires LLVM for for CPU codegen. We highly recommend you to build with the LLVM support on.
- TVM requires LLVM for CPU codegen. We highly recommend you to build with the LLVM support on.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can fix this in a follow-on, but suggest:

Suggested change
- TVM requires LLVM for CPU codegen. We highly recommend you to build with the LLVM support on.
- TVM requires LLVM for CPU codegen. When building for compilation purposes, we highly recommend you to build with the LLVM support on.

@areusch areusch merged commit 9b66f66 into apache:main May 17, 2022
shingjan pushed a commit to shingjan/tvm that referenced this pull request May 17, 2022
- Remove TVM's `USE_CCACHE` option in favor
  of CMake's built-in `CMAKE_C_COMPILER_LAUNCHER`
  and `CMAKE_CXX_COMPILER_LAUNCHER` variables.

  This eliminates a significant source of
  complexity, especially:

  - TVM's CI scripts, which use `sccache`
    instead of `ccache`, and

  - calls to `ExternalProject_add` in TVM's CMake logic.

- Ensure that `CMAKE_C[XX]_COMPILER_LAUNCHER` variables
  are passed through in all `ExternalProject_add` calls.

- Update user documentation.
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