-
Notifications
You must be signed in to change notification settings - Fork 12.2k
cmake: fix clang build when CUDA is enabled (#4208) #4323
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
Conversation
Don't use the cxx_flags for .cu files so that -Wunreachable-code-break and -Wunreachable-code-return would not be sent to to GCC which doesn't understand them. By default, nvcc uses gcc as the preprocessor even when Clang is used for .c/.cpp. This should fix the CUDA build with clang 14 (when gcc is used with nvcc). With more complex configuration, it probably would be possible to use clang itself (instead of nvcc) to compile CUDA code or use clang as the preprocessor for nvcc. But these are probably to be quite rare use cases. It seems that Clang 16 would require even more changes.
set(warning_flags ${warning_flags} -Wunreachable-code-break -Wunreachable-code-return) | ||
set(c_flags ${c_flags} -Wunreachable-code-break -Wunreachable-code-return) | ||
# cxx_flags are used for C++ files but not for CUDA | ||
set(cxx_flags -Wunreachable-code-break -Wunreachable-code-return) |
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.
Hm, I think we should be passing cxx_flags to nvcc as in the Makefile - that's how I intended it. We probably shouldn't pass host_cxx_flags to nvcc at all, since we can't assume that it uses the same compiler as the host. We could possibly use -Xcompiler for cxx_flags, not sure if there's any benefit.
So I think warning_flags should be split into c_flags and host_cxx_flags.
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.
I think it's a valid point that it would be nice if the Makefile and CMakeLists.txt would have somewhat similar patterns and variable names. Anyway, now that I checked, it it seems that also the Makefile fails in building CUDA enabled version with clang.
E.g.:
export CXX=/usr/bin/clang++
export CC=/usr/bin/clang
make LLAMA_CUBLAS=1
...
/usr/bin/clang++ -I. -Icommon -D_XOPEN_SOURCE=600 -D_GNU_SOURCE -DNDEBUG -DGGML_USE_CUBLAS -I/usr/local/cuda/include -I/opt/cuda/include -I/targets/x86_64-linux/include -std=c++11 -fPIC -O3 -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wmissing-declarations -Wmissing-noreturn -pthread -Wunreachable-code-break -Wunreachable-code-return -Wmissing-prototypes -Wextra-semi -march=native -mtune=native -c common/build-info.cpp -o build-info.o
nvcc --forward-unknown-to-host-compiler -use_fast_math -arch=native -DGGML_CUDA_DMMV_X=32 -DGGML_CUDA_MMV_Y=1 -DK_QUANTS_PER_ITERATION=2 -DGGML_CUDA_PEER_MAX_BATCH_SIZE=128 -I. -Icommon -D_XOPEN_SOURCE=600 -D_GNU_SOURCE -DNDEBUG -DGGML_USE_CUBLAS -I/usr/local/cuda/include -I/opt/cuda/include -I/targets/x86_64-linux/include -std=c++11 -fPIC -O3 -Wall -Wextra -Wpedantic -Wcast-qual -Wno-unused-function -Wmissing-declarations -Wmissing-noreturn -pthread -Wno-pedantic -Xcompiler "-Wunreachable-code-break -Wunreachable-code-return -Wmissing-prototypes -Wextra-semi -march=native -mtune=native " -c ggml-cuda.cu -o ggml-cuda.o
gcc: error: unrecognized command-line option '-Wunreachable-code-break'; did you mean '-Wunreachable-code'?
gcc: error: unrecognized command-line option '-Wunreachable-code-return'; did you mean '-Wunreachable-code'?
make: *** [Makefile:451: ggml-cuda.o] Error 1
To sum up, I think the parameters get set correctly using this PR. But perhaps the variable names could be clarified, and possible there could be some changes in the way they get set.
Then the Makefile should probably have similar changes.
Don't use the cxx_flags for .cu files so that -Wunreachable-code-break and -Wunreachable-code-return would not be sent to GCC which doesn't understand them.
By default, nvcc uses gcc as the preprocessor even when Clang is used for .c/.cpp.
This should fix the CUDA build with clang 14 (when gcc is used with nvcc). With more complex configuration, it probably would be possible to use clang itself (instead of nvcc) to compile CUDA code or use clang as the preprocessor for nvcc. But these are probably to be quite rare use cases. It seems that Clang 16 would require even more changes.