-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[Offload] Prevent by default passing CMAKE_CXX_FLAGS to GPU build #160881
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
base: main
Are you sure you want to change the base?
Conversation
Summary: People will use this to pass incompatible flags. The default behavior is to inherit the top level options for all the runtimes. Some of these aren't compatible. The cache file should put users in a most-useful state, and this also serves as documentation. Just override it so users need to explicit if they want a non-standard flag on their GPU build.
@llvm/pr-subscribers-offload Author: Joseph Huber (jhuber6) ChangesSummary: Full diff: https://github.com/llvm/llvm-project/pull/160881.diff 1 Files Affected:
diff --git a/offload/cmake/caches/Offload.cmake b/offload/cmake/caches/Offload.cmake
index 3747a1d3eb299..09262f8ce6d1e 100644
--- a/offload/cmake/caches/Offload.cmake
+++ b/offload/cmake/caches/Offload.cmake
@@ -7,3 +7,9 @@ set(RUNTIMES_nvptx64-nvidia-cuda_CACHE_FILES "${CMAKE_SOURCE_DIR}/../libcxx/cmak
set(RUNTIMES_amdgcn-amd-amdhsa_CACHE_FILES "${CMAKE_SOURCE_DIR}/../libcxx/cmake/caches/AMDGPU.cmake" CACHE STRING "")
set(RUNTIMES_nvptx64-nvidia-cuda_LLVM_ENABLE_RUNTIMES "compiler-rt;libc;openmp;libcxx;libcxxabi" CACHE STRING "")
set(RUNTIMES_amdgcn-amd-amdhsa_LLVM_ENABLE_RUNTIMES "compiler-rt;libc;openmp;libcxx;libcxxabi" CACHE STRING "")
+
+# Prevent people mistakenly sending CPU flags to the GPU build.
+set(RUNTIMES_nvptx64-nvidia-cuda_CMAKE_CXX_FLAGS "" CACHE STRING "")
+set(RUNTIMES_amdgcn-amd-amdhsa_CMAKE_CXX_FLAGS "" CACHE STRING "")
+set(RUNTIMES_nvptx64-nvidia-cuda_CMAKE_C_FLAGS "" CACHE STRING "")
+set(RUNTIMES_amdgcn-amd-amdhsa_CMAKE_C_FLAGS "" CACHE STRING "")
|
Based on my experience, many people do use this flag globally to pass compile flags, even for GPU targets, not to build LLVM though. I understand is not great and do support this change, but I think it might be better to document this. |
My thinking is that the vast majority of people are not intending this flag to show up on the GPU portion, and if they want it to they can override this. I really need to write some more comprehensive runtimes documentation, what we have now is certainly lacking. |
I agree that this should be documented properly. And I personally would say that the update to the documentation should be part of this PR. |
From https://openmp.llvm.org/SupportAndFAQ.html#q-how-to-build-an-openmp-gpu-offload-capable-compiler
|
The goal of this is to set a reasonable default if someone uses the cache file, and to document the flags used if someone's thinking of modifying them. |
Summary:
People will use this to pass incompatible flags. The default behavior is
to inherit the top level options for all the runtimes. Some of these
aren't compatible. The cache file should put users in a most-useful
state, and this also serves as documentation. Just override it so users
need to explicit if they want a non-standard flag on their GPU build.