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 using C++23 #5283

Merged
merged 14 commits into from
Jul 31, 2022
Merged

Allow using C++23 #5283

merged 14 commits into from
Jul 31, 2022

Conversation

masterleinad
Copy link
Contributor

This still assumes that C++14 is allowed.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Looks good to me

@masterleinad
Copy link
Contributor Author

This is good enough for me now. To have at least one C++20 and one C++23 it seemed easiest to pick CI builds that require very recent compilers anyway. I first played with OpenMPTarget and SYCL and ran into toolchain issues with the GCC headers. Bumping the HIP builds, though, seemed to work fine.
I still kept the OpenMPTarget changes since we will be running into those warnings anyway.

@masterleinad masterleinad marked this pull request as ready for review July 28, 2022 21:35
@masterleinad
Copy link
Contributor Author

The OPENACC-NVHPC-CUDA-11.6 build should be fixed by #5288.

@masterleinad masterleinad requested a review from nliber July 28, 2022 21:36
dalg24
dalg24 previously requested changes Jul 29, 2022
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I object to the CI changes as they are now
Bumping 14 -> 17 and 17 -> 20, sure. But not C++23

Comment on lines 24 to 29
ELSEIF(${KOKKOS_CXX_STANDARD} STREQUAL "c++20")
MESSAGE(WARNING "Deprecated Kokkos C++ standard set as 'c++20'. Use '20' instead.")
SET(KOKKOS_CXX_STANDARD "20")
ELSEIF(${KOKKOS_CXX_STANDARD} STREQUAL "c++23")
MESSAGE(WARNING "Deprecated Kokkos C++ standard set as 'c++23'. Use '23' instead.")
SET(KOKKOS_CXX_STANDARD "23")
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue for turning the deprecation message above into an error and remove that block altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss that in another pull request.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I do want one C++23 build, but its fine with me if that is a GCC build on github. using GCC 12 preferrably.

Copy link
Member

Choose a reason for hiding this comment

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

This can happenlater btw.

Comment on lines +127 to +128
#if (defined(KOKKOS_ENABLE_CXX17) || defined(KOKKOS_ENABLE_CXX20) || \
defined(KOKKOS_ENABLE_CXX23)) && \
Copy link
Member

Choose a reason for hiding this comment

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

Mark it FIXME_CXX17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these places are already (supposed to be part of #5295) and I would assume any cleanup after requiring C++17 to search for KOKKOS_ENABLE_CXX17 anyway.

@@ -644,7 +645,8 @@ static constexpr bool kokkos_omp_on_host() { return false; }
#define KOKKOS_ENABLE_CUDA_LDG_INTRINSIC
#endif

#if defined(KOKKOS_ENABLE_CXX17) || defined(KOKKOS_ENABLE_CXX20)
#if defined(KOKKOS_ENABLE_CXX17) || defined(KOKKOS_ENABLE_CXX20) || \
defined(KOKKOS_ENABLE_CXX23)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

Comment on lines +87 to +88
#if defined(KOKKOS_ENABLE_CXX17) || defined(KOKKOS_ENABLE_CXX20) || \
defined(KOKKOS_ENABLE_CXX23)
Copy link
Member

Choose a reason for hiding this comment

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

FIXME_CXX17

@dalg24 dalg24 dismissed their stale review July 31, 2022 14:14

Changes requested were implemented

@dalg24 dalg24 merged commit b2371de into kokkos:develop Jul 31, 2022
@masterleinad masterleinad mentioned this pull request Sep 7, 2022
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.

None yet

4 participants