-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add support for CUDA 10.2 #886
Conversation
The
Any ideas what we can do about this? |
@BenjaminW3 I don't. It might be an incompatibility between these compiler and CUDA versions, not the first time it happens. But I don't know for sure, might still be smth on our side. |
dfb21ff
to
6c8b617
Compare
The problems of nvcc 10.2 with clang 8.0 within |
6c8b617
to
ce1dc4e
Compare
Interesting, Clang 8.0 host-compiler support is not even new but part of 10.1.168+ already... Does compiling in |
I tried both, release and debug. Does not help. With nvcc 10.1 it works in CI. They probably broke something. |
Urgh, thanks for testing! |
Feedback from my Nvidia contact: Clang 8.0 works, but we should not pass |
1341bee
to
f9c0b21
Compare
You are right, with libstdc++it seems to work. |
a9374fa
to
30b5083
Compare
30b5083
to
3f5acf3
Compare
Ready for approval/merging! |
Change requested due to not properly understanding.
GET_TARGET_PROPERTY(_COMMON_COMPILE_OPTIONS common COMPILE_OPTIONS) | ||
# If the property does not exist, the variable is set to NOTFOUND. | ||
IF(_COMMON_COMPILE_OPTIONS) | ||
STRING(REPLACE ";" " " _COMMON_COMPILE_OPTIONS_STRING "${_COMMON_COMPILE_OPTIONS}") | ||
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${_COMMON_COMPILE_OPTIONS_STRING}") | ||
ENDIF() | ||
# nvcc supports werror starting with 10.2 | ||
IF(CUDA_VERSION GREATER_EQUAL 10.2) |
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.
Adding this here is not ideal either, since for example package managers in unanticipated environments might run your tests and a simple warning will mark a package as broken.
Please just enable -Werror=all-warnings
in CI, e.g. via a travis environment variable or additional CMake option of Alpaka (that is by default not on). Shipping auto-enabled -Werror
flags of any kind in CMake scripts is a bug :)
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.
We already did this for years with the normal compilers Werror flag as most other open source projects are also doing. So this is at least not regression.
Furthermore it is the exact goal of setting the Werror flag to let the tests fail if they do not compile cleanly. Why should we treat warnings differently to compile errors? Having no watnings is our definition of our tests passing. So if a package manager for any reason (I do not know why it should do this at all) runs the tests and they produce a warning on this platform, then our tests fail and this is exactly what we want. That's the whole goal behind Werror.
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 am just telling you from a package management point of view, that will use software with newer environments than what were available and testable at release, that this is not an ideal practice.
Package managers such as spack, homebrew, et al. run tests to see if a package is working in a larger environment (integration tests). One usually just runs what comes with the package, to avoid duplication and maintenance overhead.
As developers, we can recommend best practices but it is not our place to anticipate new additions to -Wall
, -Wextra
or whatever a compiler decides to warn on. There are more compilers and workflows out there than what we do daily, e.g. people that build their own compiler frontends, optimizer passes, tooling, etc. on projects such as ours. Maybe a new NVCC just warns on something totally minor. Let's not patronize them. Warnings are called warnings for a reason.
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 see your point. However a newer, self built or unknown compiler is in no way different to newer CUDA versions, newer or self built CMake versions, newer OSs or any other dependency. The Compiler is also only a dependency of alpaka. All of those dependencies can make the build or the tests fail at compile or runtime.
I do not see a reason why we should have lower requirements than is possible on the compilation while we expect everything to generate, run and calculate perfectly.
If we lower the requirements on one dependency, we should also reduce the requirements on numerical stability, quality of random numbers and other things that may behave unexpectedly on untested platforms.
Finding issues on untested platforms is the goal of tests and for alpaka a clean compilation is what we defined as a requirement for a successful test execution.
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.
If we lower the requirements on one dependency, we should also reduce the requirements on numerical stability, quality of random numbers and other things that may behave unexpectedly on untested platforms.
I think it's not that black and white. A compiler warning in a newer nvcc might just be fine, no reason to break the tests outside of CI for this. A test that passes and throws a warning is still a tests that passes its primary coded purpose. A CMake flag that also enables -Werror
intentionally can then also be used on top of this.
This is just compartmentalization of goals into smaller scopes of interest. Our goal in mainline is to have the code base error and warning free. That's why we require -Werror
for all code to come in. A user's primary downstream goal is to have the tests technically compute the right results as a baseline for whatever they want to do. They can decide if they want to treat warnings as errors as well. I tell you from experience that many use cases do not want this to be the case, because warnings are not errors, some warnings are false positives in new environments, etc. Let's just optionalize this and enable this for us.
Implements #885
Enables
-Werror=all-warnings
so that warnings from the cuda compiler frontend also make the build fail.