Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

jonppe
Copy link

@jonppe jonppe commented Dec 4, 2023

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.

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.
@jonppe jonppe mentioned this pull request Dec 4, 2023
4 tasks
Comment on lines -408 to +412
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)
Copy link
Collaborator

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.

Copy link
Author

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.

@cebtenzzre
Copy link
Collaborator

@jonppe I implemented #4414 for the Makefile. Does that seem like what you need? If it works for you, I'll port it to CMake.

@jonppe
Copy link
Author

jonppe commented Dec 11, 2023

@jonppe I implemented #4414 for the Makefile. Does that seem like what you need? If it works for you, I'll port it to CMake.

Yes, look quite nice and clean to me and avoids the nasty repetition in compiler checks.

@cebtenzzre cebtenzzre closed this Dec 11, 2023
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.

2 participants