-
Notifications
You must be signed in to change notification settings - Fork 12k
CMAKE CUDA fix: Unknown option '-Wall' #5543
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
I was also facing this issue with CLion CMake import, thanks a lot @clibdev |
Co-authored-by: slaren <[email protected]>
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 we can merge this to solve the immediate problem, but the way CXX_FLAGS
and the function get_flags
is being used looks completely wrong to me. get_flags
is only used with LLAMA_ALL_WARNINGS
, yet it also contains optimization flags for the Intel compiler. get_flags
is also used with CUDA to set the host compiler flags, and it actually checks what is the host compiler instead of assuming that it is the same than for the rest of the C++ code, which is great, but there are still flags from CXX_FLAGS
that should also be passed to the host compiler. Adding CXX_FLAGS
to CUDA_CXX_FLAGS
is not correct either, because CXX_FLAGS
also includes the flags from get_flags
, but checked for the wrong compiler.
@slaren Thanks for the review
@abhilash1910 Could you take a look into this - it does look incorrect to add Intel compiler flags here Will wait for @cebtenzzre to review |
This was added by @ngxson in #5068, I didn't review that PR because the title and commit messages only mentioned Docker.
The idea is that CXX_FLAGS contains generic flags, and GF_CXX_FLAGS contains the compiler-specific flags - they must both be combined to get the full list of flags. For the host this is done in the add_compile_options step, for CUDA this is currently not done consistently (mainly because I've never personally seen nvcc choke on e.g. |
I propose this solution: #5570 |
Yes it does not appear to be correct as that is in the scope of LLAMA_ALL_WARNINGS. I will remove this. Thanks for pointing this out. |
@cebtenzzre Thanks for pointing it out, #5068 was a trial to make the compilation with intel dpcpp yield more performance, but this is pretty much deprecated now after the introduction of sycl. I'm planning to remove these flags. |
Closes #5542