Skip to content
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

Allow to set a CMAKE_CUDA_STANDARD different from CMAKE_CXX_STANDARD #700

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aurianer
Copy link
Contributor

@aurianer aurianer commented Jun 16, 2023

Fixes #699

  • Add a CI config using a different CXX and CUDA standard
  • Add a GPU stdexec config with different CXX and CUDA standards

Note: Looks like there are problems with fmt@10, will look into it in a separate PR.

@aurianer aurianer self-assigned this Jun 16, 2023
@aurianer aurianer added this to the 0.17.0 milestone Jun 16, 2023
@aurianer
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 16, 2023
@bors
Copy link
Contributor

bors bot commented Jun 16, 2023

try

Build failed:

@aurianer aurianer changed the title Allow to set a CMAKE_CUDA_STANDARD different from CMAKE_CXX_STANDARD Allow to set a CMAKE_CUDA_STANDARD different from CMAKE_CXX_STANDARD Jun 16, 2023
@aurianer aurianer marked this pull request as ready for review June 20, 2023 14:48
@aurianer aurianer marked this pull request as draft June 20, 2023 14:48
@aurianer
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 20, 2023
@bors
Copy link
Contributor

bors bot commented Jun 20, 2023

try

Build failed:

@aurianer
Copy link
Contributor Author

aurianer commented Jul 3, 2023

bors try

bors bot added a commit that referenced this pull request Jul 3, 2023
@aurianer aurianer marked this pull request as ready for review July 3, 2023 11:08
@bors
Copy link
Contributor

bors bot commented Jul 3, 2023

try

Build failed:

@aurianer
Copy link
Contributor Author

aurianer commented Jul 3, 2023

bors try

bors bot added a commit that referenced this pull request Jul 3, 2023
@bors
Copy link
Contributor

bors bot commented Jul 3, 2023

try

Build failed:

@aurianer aurianer force-pushed the separate_cuda_and_cxx_standards branch from f4bc291 to 42fc27c Compare July 3, 2023 18:00
@aurianer
Copy link
Contributor Author

aurianer commented Jul 3, 2023

bors try

bors bot added a commit that referenced this pull request Jul 3, 2023
@bors
Copy link
Contributor

bors bot commented Jul 3, 2023

try

Build failed:

@aurianer
Copy link
Contributor Author

aurianer commented Jul 3, 2023

Looks good now :)

Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aurianer for looking into this! I think we should test this with +stdexec since that is the use case for this and I think enabling that will break in CI. Could explore a bit how this fails with stdexec enabled so that we can see how to tackle this best? In particular I'm worried about the feature tests being insufficient if we start allowing different standard versions.

This PR is in general lower priority until we've tested the CPU-only configurations in DLA-Future though.

gcc_version="9.3.0"
boost_version="1.75.0"
hwloc_version="2.0.3"
cuda_version="11.2.0"
fmt_version="9.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the problem with 10.0.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. During compilation of which file does that happen?

We definitely have that covered here:

template <>
struct fmt::formatter<pika::util::logging::level> : fmt::formatter<std::string>
{
template <typename FormatContext>
auto format(pika::util::logging::level const& value, FormatContext& ctx)
{
return fmt::formatter<std::string>::format(pika::util::logging::levelname(value), ctx);
}
};
. But this may be one (of many) problems that we get from allowing different standards. I'd like to see how badly other things break from this as well before we decide what to do here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of the cuda files, the error I pasted is for cuda_sender_test though, will try to add the GPU stdexec and then see what's happening

.jenkins/cscs/env-gcc-cuda-11.sh Show resolved Hide resolved
.jenkins/cscs/env-gcc-cuda-11.sh Show resolved Hide resolved
cmake/pika_setup_cuda.cmake Show resolved Hide resolved
@aurianer aurianer force-pushed the separate_cuda_and_cxx_standards branch from 42fc27c to c390d33 Compare July 5, 2023 12:08
@aurianer aurianer removed this from the 0.17.0 milestone Jul 31, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell E 16 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@msimberg msimberg marked this pull request as draft September 4, 2023 07:54
@aurianer aurianer force-pushed the separate_cuda_and_cxx_standards branch from c390d33 to 021b605 Compare November 5, 2023 17:38
# file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

configuration_slurm_constraint="gpu"
configuration_skip_pr="true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to set

Suggested change
configuration_skip_pr="true"
configuration_skip_pr="false"

for now if you want to test this in CI. In this case it may be easier to just test it manually though.

@aurianer aurianer force-pushed the separate_cuda_and_cxx_standards branch from 021b605 to 54d4f04 Compare November 7, 2023 13:17
@aurianer aurianer force-pushed the separate_cuda_and_cxx_standards branch from 54d4f04 to 1f95f41 Compare January 10, 2024 09:31
@msimberg
Copy link
Contributor

@aurianer should we abandon this now that we've got DLA-Future compiling with C++20?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Separate CMake CXX and CUDA standards
2 participants