You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/04/29 17:47:07 UTC
[GitHub] [tvm] cconvey opened a new pull request, #11189: [WIP] changes to fix ccache
cconvey opened a new pull request, #11189:
URL: https://github.com/apache/tvm/pull/11189
TODO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864019509
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
Review Comment:
typo: "thet"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864023404
##########
apps/hexagon_api/CMakeLists.txt:
##########
@@ -30,6 +30,11 @@ ExternalProject_Add(x86_tvm_runtime_rpc
SOURCE_DIR "${TVM_SOURCE_DIR}"
BUILD_COMMAND $(MAKE) runtime tvm_rpc
CMAKE_ARGS
+ "-DUSE_CCACHE=${USE_CCACHE}"
+ "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
+ "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
Review Comment:
This has the potential to change which compiler is used when building this external project. I should update the commit message to mention this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864952809
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
+if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
+
+ #-------------------------------------------------------------------------------------------------
+ # Determine which requested policy.
+ # If ccache is required but can't be found, fail with an error message.
+ #-------------------------------------------------------------------------------------------------
+ if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED FALSE)
+ elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED TRUE)
+ else() # /path/to/ccache
+ set(CCACHE_PROGRAM_CANDIDATE "${USE_CCACHE}")
+ set(CCACHE_REQUIRED TRUE)
+ endif()
+
+ find_program(CCACHE_PROGRAM "${CCACHE_PROGRAM_CANDIDATE}")
+ if (CCACHE_PROGRAM)
+ message(STATUS "Found ccache program: '${CCACHE_PROGRAM}'")
+ else()
+ # Note: Once we require cmake >= 3.18, we can simplify this logic using
+ # `find_program(... REQUIRED)`.
+ if (CCACHE_REQUIRED)
+ message(FATAL_ERROR "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
+ else()
+ message(STATUS "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
Review Comment:
Good question. I think the basic issue is that we can't distinguish the two causes of `USE_CCACHE=AUTO`:
- the user explicitly set it that way, vs.
- the user left the default value in place, perhaps unwittingly.
If we knew that the user explicitly set it to `AUTO`, I think `WARNING` is a better choice than `STATUS`.
Since we don't know either way, my personal preference is to bias towards clean builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864022510
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
+if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
+
+ #-------------------------------------------------------------------------------------------------
+ # Determine which requested policy.
Review Comment:
Clearly written before my second cup of coffee. Needs rewording.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864960842
##########
docs/install/from_source.rst:
##########
@@ -315,7 +332,7 @@ configuration. A workaround for this is to do the following commands:
brew install openblas gfortran
- pip install pybind11 cython pythran
+ pip install pybind11 cython pythran
Review Comment:
I have a vim plugin that automatically removes trailing spaces. So IIUC this is actually fixing a space error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864139622
##########
apps/hexagon_api/CMakeLists.txt:
##########
@@ -30,6 +30,11 @@ ExternalProject_Add(x86_tvm_runtime_rpc
SOURCE_DIR "${TVM_SOURCE_DIR}"
BUILD_COMMAND $(MAKE) runtime tvm_rpc
CMAKE_ARGS
+ "-DUSE_CCACHE=${USE_CCACHE}"
+ "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
+ "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
Review Comment:
what happens if `USE_CCACHE` is OFF and `CMAKE_C_COMPILER_LAUNCHER` is not set as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r873800649
##########
CMakeLists.txt:
##########
@@ -84,6 +84,7 @@ tvm_option(USE_BLAS "The blas library to be linked" none)
tvm_option(USE_MKL "MKL root path when use MKL blas" OFF)
tvm_option(USE_MKLDNN "Build with MKLDNN" OFF)
tvm_option(USE_DNNL_CODEGEN "Enable MKLDNN (DNNL) codegen" OFF)
+tvm_option(USE_CCACHE "Use the 'ccache' compiler wrapper when building TVM" AUTO)
Review Comment:
This is moot now they the PR removes `USE_CCACHE` entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: changes to fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r862242186
##########
CMakeLists.txt:
##########
@@ -747,35 +795,6 @@ if(BUILD_FOR_HEXAGON)
endif()
endif()
-#Caches the build.
-#Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
-#need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
-
-if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
- if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(STATUS "Didn't find the path to CCACHE, disabling ccache")
- endif(CCACHE_FOUND)
- elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(FATAL_ERROR "Cannot find ccache. Set USE_CCACHE mode to AUTO or OFF to build without ccache. USE_CCACHE=" "${USE_CCACHE}")
- endif(CCACHE_FOUND)
- else() # /path/to/ccache
- set(PATH_TO_CCACHE USE_CCACHE)
- message(STATUS "Setting ccache path to " "${PATH_TO_CCACHE}")
- endif()
- # Set the flag for ccache
- set(CXX_COMPILER_LAUNCHER PATH_TO_CCACHE)
-endif(USE_CCACHE)
-
Review Comment:
I moved this logic (modulo various bug fixes, etc.) towards the beginning of the file, so that it appears _before_ the relevant build targets. AFAICT, its previous location rendered it ineffective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864952809
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
+if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
+
+ #-------------------------------------------------------------------------------------------------
+ # Determine which requested policy.
+ # If ccache is required but can't be found, fail with an error message.
+ #-------------------------------------------------------------------------------------------------
+ if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED FALSE)
+ elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED TRUE)
+ else() # /path/to/ccache
+ set(CCACHE_PROGRAM_CANDIDATE "${USE_CCACHE}")
+ set(CCACHE_REQUIRED TRUE)
+ endif()
+
+ find_program(CCACHE_PROGRAM "${CCACHE_PROGRAM_CANDIDATE}")
+ if (CCACHE_PROGRAM)
+ message(STATUS "Found ccache program: '${CCACHE_PROGRAM}'")
+ else()
+ # Note: Once we require cmake >= 3.18, we can simplify this logic using
+ # `find_program(... REQUIRED)`.
+ if (CCACHE_REQUIRED)
+ message(FATAL_ERROR "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
+ else()
+ message(STATUS "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
Review Comment:
Good question. I think the basic issue is that we can't distinguish the two causes of `USE_CCACHE=AUTO`:
- the user explicitly set it that way (maybe should be `WARNING`), vs.
- the user left the default value in place, perhaps unwittingly (probably should be `STATUS`).
Since we don't know either way, my personal preference is to bias towards clean builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on pull request #11189: changes to fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #11189:
URL: https://github.com/apache/tvm/pull/11189#issuecomment-1113836829
CC @csullivan @mehrdadh @adstraw
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #11189:
URL: https://github.com/apache/tvm/pull/11189#issuecomment-1127018534
@mehrdadh : Ready for merge if/when you're happy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864140322
##########
CMakeLists.txt:
##########
@@ -84,6 +84,7 @@ tvm_option(USE_BLAS "The blas library to be linked" none)
tvm_option(USE_MKL "MKL root path when use MKL blas" OFF)
tvm_option(USE_MKLDNN "Build with MKLDNN" OFF)
tvm_option(USE_DNNL_CODEGEN "Enable MKLDNN (DNNL) codegen" OFF)
+tvm_option(USE_CCACHE "Use the 'ccache' compiler wrapper when building TVM" AUTO)
Review Comment:
I think by default should be OFF since we want to build from scratch in CI. Actually it may doesn't matter since it's in docker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864917777
##########
CMakeLists.txt:
##########
@@ -84,6 +84,7 @@ tvm_option(USE_BLAS "The blas library to be linked" none)
tvm_option(USE_MKL "MKL root path when use MKL blas" OFF)
tvm_option(USE_MKLDNN "Build with MKLDNN" OFF)
tvm_option(USE_DNNL_CODEGEN "Enable MKLDNN (DNNL) codegen" OFF)
+tvm_option(USE_CCACHE "Use the 'ccache' compiler wrapper when building TVM" AUTO)
Review Comment:
Thanks. Sounds like the best plan is:
- Let `USE_CCACHE` continue to default to `AUTO`.
- I'll modify `tests/scripts/task_build.py` to set `USE_CCACHE=OFF`, so that CI doesn't pointlessly waste storage space.
If/when we want to enable `ccache` or` sccache` in CI, we can just do another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
areusch merged PR #11189:
URL: https://github.com/apache/tvm/pull/11189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r863992615
##########
CMakeLists.txt:
##########
@@ -84,6 +84,7 @@ tvm_option(USE_BLAS "The blas library to be linked" none)
tvm_option(USE_MKL "MKL root path when use MKL blas" OFF)
tvm_option(USE_MKLDNN "Build with MKLDNN" OFF)
tvm_option(USE_DNNL_CODEGEN "Enable MKLDNN (DNNL) codegen" OFF)
+tvm_option(USE_CCACHE "Use the 'ccache' compiler wrapper when building TVM" AUTO)
Review Comment:
maybe we want this off by default since it would take disk? or alternatively, can you change tests/scripts/task_build.py to flip this off in CI build (we have sccache there)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] adstraw commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864338884
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
+if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
+
+ #-------------------------------------------------------------------------------------------------
+ # Determine which requested policy.
+ # If ccache is required but can't be found, fail with an error message.
+ #-------------------------------------------------------------------------------------------------
+ if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED FALSE)
+ elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED TRUE)
+ else() # /path/to/ccache
+ set(CCACHE_PROGRAM_CANDIDATE "${USE_CCACHE}")
+ set(CCACHE_REQUIRED TRUE)
+ endif()
+
+ find_program(CCACHE_PROGRAM "${CCACHE_PROGRAM_CANDIDATE}")
+ if (CCACHE_PROGRAM)
+ message(STATUS "Found ccache program: '${CCACHE_PROGRAM}'")
+ else()
+ # Note: Once we require cmake >= 3.18, we can simplify this logic using
+ # `find_program(... REQUIRED)`.
+ if (CCACHE_REQUIRED)
+ message(FATAL_ERROR "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
+ else()
+ message(STATUS "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
Review Comment:
Wondering if this should be a warning? At this point the user has expressed some desire to use ccache and we couldn't find it. Feels like `STATUS` is burying the lede.
##########
docs/install/from_source.rst:
##########
@@ -315,7 +332,7 @@ configuration. A workaround for this is to do the following commands:
brew install openblas gfortran
- pip install pybind11 cython pythran
+ pip install pybind11 cython pythran
Review Comment:
Space error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864018605
##########
CMakeLists.txt:
##########
@@ -84,6 +84,7 @@ tvm_option(USE_BLAS "The blas library to be linked" none)
tvm_option(USE_MKL "MKL root path when use MKL blas" OFF)
tvm_option(USE_MKLDNN "Build with MKLDNN" OFF)
tvm_option(USE_DNNL_CODEGEN "Enable MKLDNN (DNNL) codegen" OFF)
+tvm_option(USE_CCACHE "Use the 'ccache' compiler wrapper when building TVM" AUTO)
Review Comment:
For non-CI environments I think either choice is reasonable. "auto" was supposed to be the default all along, but AFAICT a bug meant it was basically acting like "OFF". Thoughts?
For CI environments, I'm happy to make whatever changes you / @mehrdadh would like. Just let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r873205122
##########
CMakeLists.txt:
##########
@@ -747,35 +795,6 @@ if(BUILD_FOR_HEXAGON)
endif()
endif()
-#Caches the build.
-#Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
-#need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
-
-if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
- if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(STATUS "Didn't find the path to CCACHE, disabling ccache")
- endif(CCACHE_FOUND)
- elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(FATAL_ERROR "Cannot find ccache. Set USE_CCACHE mode to AUTO or OFF to build without ccache. USE_CCACHE=" "${USE_CCACHE}")
- endif(CCACHE_FOUND)
- else() # /path/to/ccache
- set(PATH_TO_CCACHE USE_CCACHE)
- message(STATUS "Setting ccache path to " "${PATH_TO_CCACHE}")
- endif()
- # Set the flag for ccache
- set(CXX_COMPILER_LAUNCHER PATH_TO_CCACHE)
-endif(USE_CCACHE)
-
Review Comment:
Update: This comment ^^^ is moot, now that I've removed the USE_CCACHE variable entirely.
##########
CMakeLists.txt:
##########
@@ -747,35 +795,6 @@ if(BUILD_FOR_HEXAGON)
endif()
endif()
-#Caches the build.
-#Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
-#need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
-
-if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
- if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(STATUS "Didn't find the path to CCACHE, disabling ccache")
- endif(CCACHE_FOUND)
- elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
- find_program(CCACHE_FOUND ccache)
- if(CCACHE_FOUND)
- message(STATUS "Found the path to ccache, enabling ccache")
- set(PATH_TO_CCACHE ccache)
- else()
- message(FATAL_ERROR "Cannot find ccache. Set USE_CCACHE mode to AUTO or OFF to build without ccache. USE_CCACHE=" "${USE_CCACHE}")
- endif(CCACHE_FOUND)
- else() # /path/to/ccache
- set(PATH_TO_CCACHE USE_CCACHE)
- message(STATUS "Setting ccache path to " "${PATH_TO_CCACHE}")
- endif()
- # Set the flag for ccache
- set(CXX_COMPILER_LAUNCHER PATH_TO_CCACHE)
-endif(USE_CCACHE)
-
Review Comment:
Update: This comment ^^^ is moot, now that I've removed the USE_CCACHE variable entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r873801118
##########
CMakeLists.txt:
##########
@@ -509,6 +510,103 @@ set(LIBINFO_FILE ${CMAKE_CURRENT_LIST_DIR}/src/support/libinfo.cc)
add_lib_info(${LIBINFO_FILE})
list(REMOVE_ITEM COMPILER_SRCS ${LIBINFO_FILE})
+# Caches the build.
+# Note that ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still
+# need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.
+#
+# NOTE: CMAKE_C[XX]_COMPILER_LAUNCHER only affects build targets that we add *later*.
+# So we want this logic to appear towards the top of thet this CMakeLists.txt file.
+if(USE_CCACHE) # True for AUTO, ON, /path/to/ccache
+
+ #-------------------------------------------------------------------------------------------------
+ # Determine which requested policy.
+ # If ccache is required but can't be found, fail with an error message.
+ #-------------------------------------------------------------------------------------------------
+ if("${USE_CCACHE}" STREQUAL "AUTO") # Auto mode
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED FALSE)
+ elseif("${USE_CCACHE}" MATCHES ${IS_TRUE_PATTERN})
+ set(CCACHE_PROGRAM_CANDIDATE ccache)
+ set(CCACHE_REQUIRED TRUE)
+ else() # /path/to/ccache
+ set(CCACHE_PROGRAM_CANDIDATE "${USE_CCACHE}")
+ set(CCACHE_REQUIRED TRUE)
+ endif()
+
+ find_program(CCACHE_PROGRAM "${CCACHE_PROGRAM_CANDIDATE}")
+ if (CCACHE_PROGRAM)
+ message(STATUS "Found ccache program: '${CCACHE_PROGRAM}'")
+ else()
+ # Note: Once we require cmake >= 3.18, we can simplify this logic using
+ # `find_program(... REQUIRED)`.
+ if (CCACHE_REQUIRED)
+ message(FATAL_ERROR "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
+ else()
+ message(STATUS "Unable to find ccache program: '${CCACHE_PROGRAM_CANDIDATE}'")
Review Comment:
Moot now that the PR simply removes `USE_CCACHE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r875231403
##########
docs/install/from_source.rst:
##########
@@ -109,7 +109,7 @@ The configuration of TVM can be modified by editing `config.cmake` and/or by pas
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.
Review Comment:
can fix this in a follow-on, but suggest:
```suggestion
- TVM requires LLVM for CPU codegen. When building for compilation purposes, we highly recommend you to build with the LLVM support on.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] cconvey commented on a diff in pull request #11189: fix ccache
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #11189:
URL: https://github.com/apache/tvm/pull/11189#discussion_r864959425
##########
apps/hexagon_api/CMakeLists.txt:
##########
@@ -30,6 +30,11 @@ ExternalProject_Add(x86_tvm_runtime_rpc
SOURCE_DIR "${TVM_SOURCE_DIR}"
BUILD_COMMAND $(MAKE) runtime tvm_rpc
CMAKE_ARGS
+ "-DUSE_CCACHE=${USE_CCACHE}"
+ "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
+ "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
Review Comment:
> what happens if USE_CCACHE is OFF and CMAKE_C_COMPILER_LAUNCHER is not set as well?
In that case, the external project will just use the C/C++ compiler in the normal, straightforward way. It won't make any attempt to use ccache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org