Require C++17 #5277

masterleinad · 2022-07-27T13:45:33Z

This still needs to update compiler requirements.

Makefile.kokkos

crtrott · 2022-07-27T14:16:16Z

core/src/Kokkos_Core.hpp

@@ -102,8 +102,8 @@ void declare_configuration_metadata(const std::string& category,

 }  // namespace Impl

-KOKKOS_ATTRIBUTE_NODISCARD bool is_initialized() noexcept;
-KOKKOS_ATTRIBUTE_NODISCARD bool is_finalized() noexcept;
+[[nodiscard]] bool is_initialized() noexcept;


before we do this, can we check that every minimum compiler supports this?

https://godbolt.org/z/5Pz8jWeqo shows that the minimal compiler versions (godbolt has) are clang-5, MSVC 14, GCC 7.1, and ICC 18.0.0.

Following https://en.cppreference.com/w/cpp/compiler_support/17, these compiler versions are good candidates anyway. Additionally, that would suggest ICC 19.0.0 (if constexpr), msvc 19.15 (std::is_aggregate), no changes for IntelLLVM, whatever HIPCC (we are already testing with C++17 support in one build with the minimum), nvcc 11.0, pgi 19.1.

From my experience doing some of the downstream porting and testing work on Intel 'classic', you need 19.0.1, and I recommend Intel 19.0.5 or 19.1. The bare 19.0 is painfully buggy, and even 19.0.[1-4] are pretty bad. The Trilinos ATDM configuration I got added was for 19.0.5, so I don't think you'll get any complaints about picking that. 19.1 avoids some further bugs and stupid workarounds that shouldn't be necessary.

Apparently, Intel "19.0.5" as reported on the command line icpc --version is actually 19.5 as reported by the compiler's macros. We'll need to see what CMake thinks of it

dalg24

Require C++17, update the CI build but do not start changing the code that you do not absolutely have to in this PR

crtrott

Looks pretty good. Lets decide what to do with the other standard options, and also change the CI to include minimum compilers, and do a manual check with other minimum compilers (e.g. Intel 19.1)

masterleinad · 2022-07-27T14:42:53Z

Require C++17, update the CI build but do not start changing the code that you do not absolutely have to in this PR

I honestly feel much better if we have the CI pass with all these changes (which for the most part, i.e., basically without the STATIC_ASSERT changes, basically only remove guards). I don't mind pushing these changes to a separate pull request in the end.

.jenkins

crtrott · 2022-07-27T18:29:54Z

GCC 7.3
Clang 8.0
Intel 19.1
CUDA 11.0
ROCM 5.2

PhilMiller · 2022-07-27T19:25:01Z

GCC 7.3

There are a number of ICE cases that code generated by nvcc triggers in GCC 7.x for x<5. Since 7.3 was a deployed standard on a lot of lab systems, and 7.5 wasn't, we may want to require GCC 8.y or 9.z instead

PhilMiller · 2022-07-27T19:27:56Z

There's also a noted correction for DRs regarding CTAD in GCC 8 per https://gcc.gnu.org/projects/cxx-status.html#cxx17

masterleinad · 2022-07-29T03:36:27Z

CUDA-11.7-NVCC is still failing in cuda.reducers_bhalf_t. Otherwise, things are looking good in my opinion.
All that's left then is to finalize the minimum compiler version requirements.

masterleinad · 2022-07-29T03:50:06Z

This still relies on #5286 for updating one CI build.

masterleinad · 2022-07-29T19:29:00Z

Apart from updating compiler versions (which we likely could do separately), this should be ready now. I pushed all other cleanup that was initially here and that isn't required to #5295.

.jenkins

Makefile.kokkos

bin/nvcc_wrapper

dalg24 · 2022-07-29T22:21:12Z

bin/nvcc_wrapper

-    fi
-    shared_args="$shared_args $std_flag"
-    ;;
-  --std=c++11|-std=c++11|--std=c++14|-std=c++14)


Did you mean to remove that line?

Yes, [-]-std=c++17 is the only language flag we need to support for now and we require >=cuda-11 anyway.

cmake/kokkos_test_cxx_std.cmake

dalg24 · 2022-07-29T22:35:03Z

cmake/kokkos_corner_cases.cmake

@@ -49,11 +49,11 @@ ENDIF()

 IF (KOKKOS_CXX_STANDARD STREQUAL 17)
  IF (KOKKOS_CXX_COMPILER_ID STREQUAL GNU AND KOKKOS_CXX_COMPILER_VERSION VERSION_LESS 7)
-    MESSAGE(FATAL_ERROR "You have requested C++17 support for GCC ${KOKKOS_CXX_COMPILER_VERSION}. Although CMake has allowed this and GCC accepts -std=c++1z/c++17, GCC < 7 does not properly support *this capture. Please reduce the C++ standard to 14 or upgrade the compiler if you do need C++17 support.")
+    MESSAGE(FATAL_ERROR "You have requested C++17 support for GCC ${KOKKOS_CXX_COMPILER_VERSION}. Although CMake has allowed this and GCC accepts -std=c++1z/c++17, GCC < 7 does not properly support *this capture. Please upgrade the compiler if you do need C++17 support.")


The entire IF (KOKKOS_CXX_STANDARD STREQUAL 17) block is to be removed when we complete the full transition. Maybe mark it as a FIXME_CXX17

masterleinad · 2022-08-01T13:14:11Z

Makefile.kokkos

@@ -330,14 +316,11 @@ endif

 # Set C++ version flags.
 ifeq ($(KOKKOS_INTERNAL_COMPILER_PGI), 1)
-  KOKKOS_INTERNAL_CXX14_FLAG := --c++14
  KOKKOS_INTERNAL_CXX17_FLAG := --c++17
  KOKKOS_INTERNAL_CXX20_FLAG := --c++20
  KOKKOS_INTERNAL_CXX23_FLAG := --c++23
 else


For PGI, CMake gives https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Compiler/PGI-CXX.cmake, i.e., only support up to C++17.

masterleinad · 2022-08-01T13:17:43Z

Makefile.kokkos

  KOKKOS_INTERNAL_CXX17_FLAG := --c++17
  KOKKOS_INTERNAL_CXX20_FLAG := --c++20
  KOKKOS_INTERNAL_CXX23_FLAG := --c++23
 else
  ifeq ($(KOKKOS_INTERNAL_COMPILER_XL), 1)
-     KOKKOS_INTERNAL_CXX14_FLAG := -std=c++14
-     KOKKOS_INTERNAL_CXX1Y_FLAG := -std=c++1y
     KOKKOS_INTERNAL_CXX17_FLAG := -std=c++17
     KOKKOS_INTERNAL_CXX1Z_FLAG := -std=c++1z
     KOKKOS_INTERNAL_CXX20_FLAG := -std=c++20


According to https://en.cppreference.com/w/cpp/compiler_support/17 and https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Compiler/XL-CXX.cmake, XL doesn't really have C++17 support, so I guess we can just drop it.

masterleinad · 2022-08-01T13:27:01Z

bin/nvcc_wrapper

-    fi
-    shared_args="$shared_args $std_flag"
-    ;;
-  --std=c++11|-std=c++11|--std=c++14|-std=c++14)


Yes, [-]-std=c++17 is the only language flag we need to support for now and we require >=cuda-11 anyway.

cmake/kokkos_test_cxx_std.cmake

bin/nvcc_wrapper

PhilMiller · 2022-08-01T22:07:33Z

cmake/compile_tests/cplusplus17.cpp

+
+int main() {
+  // _v versions of type traits were added in C++17
+  constexpr bool same = std::is_same_v<double, int>;


Is it salient that this is testing a standard library feature, rather than a compiler feature? Variable templates themselves were part of C++14, so a standard library implementation could provide an implementation for this that would compile under C++14.

Previously, we had std::remove_cv_t<int> i = 0; for C++14.
I am happy to hear other suggestions. What about if constexpr?

As an edge example, libstdc++ as shipped with GCC 7.1 would pass this, though we're planning to set at least 7.3 as the minimum per #5285

Given that if constexpr is probably one of the most desirable headline features for us, that seems quite suitable as a target.

I added if constexpr but also kept std::is_same_v since the test case was initially designed to detect a working toolchain (with recent enough gcc headers), see #3809 and #3789.

dalg24

Looks mostly ok
I'd like for someone else to look over the nvcc_wrapper changes

dalg24 · 2022-08-02T03:47:54Z

containers/unit_tests/TestErrorReporter.hpp

@@ -195,6 +195,7 @@ struct ErrorReporterDriverUseLambda
  void execute(int reporter_capacity, int test_size) {
    Kokkos::parallel_for(
        Kokkos::RangePolicy<execution_space>(0, test_size),
+        // NOLINTNEXTLINE(kokkos-implicit-this-capture)


Wait what? Does that mean there is a bug in the clang-tidy check?

Yes, see https://kokkosteam.slack.com/archives/G5CBLMFLP/p1659012380757519 and kokkos/llvm-project#20.

dalg24 · 2022-08-02T03:49:52Z

containers/unit_tests/TestErrorReporter.hpp

-#if defined(KOKKOS_CLASS_LAMBDA) && \
+// FIXME_MSVC MSVC just gets confused when using the base class in the
+// KOKKOS_CLASS_LAMBDA
+#if !defined(KOKKOS_COMPILER_MSVC) && defined(KOKKOS_CLASS_LAMBDA) && \


Isn't KOKKOS_CLASS_LAMDA always defined now?

Yes, it is, but I avoided doing all the cleanup in this pull request.

masterleinad · 2022-08-02T12:32:17Z

I'd like for someone else to look over the nvcc_wrapper changes

I'm also fine splitting the nvcc_wrapper changes from this pull request if we have doubts about dropping C++14 and earlier support from it.

crtrott

Lets leave nvcc_wrapper more or less alone. I don't think we need to make it not deal with cpp14. In particular since some folks use it independent of Kokkos.

masterleinad · 2022-08-02T19:48:25Z

Lets leave nvcc_wrapper more or less alone. I don't think we need to make it not deal with cpp14. In particular since some folks use it independent of Kokkos.

Sure. I reverted those changes.

masterleinad · 2022-08-03T13:42:00Z

SYCL CI timing out is unrelated. That build required C++17 already.

masterleinad force-pushed the require_cxx_17 branch from f7d26e7 to 5de8588 Compare July 27, 2022 14:01

crtrott requested changes Jul 27, 2022

View reviewed changes

dalg24 requested changes Jul 27, 2022

View reviewed changes

crtrott requested changes Jul 27, 2022

View reviewed changes

ajpowelsnl reviewed Jul 27, 2022

View reviewed changes

.jenkins Outdated Show resolved Hide resolved

masterleinad force-pushed the require_cxx_17 branch 3 times, most recently from 425500b to 47fff75 Compare July 28, 2022 17:59

masterleinad force-pushed the require_cxx_17 branch from 595e6ff to 5ee2715 Compare July 29, 2022 18:46

masterleinad mentioned this pull request Jul 29, 2022

C++17 cleanup #5295

Merged

masterleinad force-pushed the require_cxx_17 branch from 5ee2715 to 808091b Compare July 29, 2022 19:26

masterleinad marked this pull request as ready for review July 29, 2022 19:29

masterleinad commented Jul 29, 2022

View reviewed changes

.jenkins Outdated Show resolved Hide resolved

dalg24 reviewed Jul 29, 2022

View reviewed changes

masterleinad mentioned this pull request Jul 30, 2022

openmp.partition_master failing for gcc-8.4.0 #5298

Closed

masterleinad added 4 commits August 1, 2022 08:53

Minimal changes to source

4c5be02

Update CI

277230b

Update build system

2b820b1

Update nvcc_wrapper

35b5728

masterleinad force-pushed the require_cxx_17 branch from c53e24f to d1cb589 Compare August 1, 2022 13:02

masterleinad commented Aug 1, 2022

View reviewed changes

masterleinad added 2 commits August 1, 2022 12:08

Use OMP_NESTED = 'true' for gcc-8.4.0 in CI

dff1f45

More nvcc_wrapper clean up

9abc0b5

Add FIXME_CXX17

7b0fedf

masterleinad force-pushed the require_cxx_17 branch from 70624bb to 7b0fedf Compare August 1, 2022 16:08

PhilMiller reviewed Aug 1, 2022

View reviewed changes

dalg24 reviewed Aug 2, 2022

View reviewed changes

Use if constexpr in cplusplus17.cpp

fefefce

masterleinad requested review from dalg24 and crtrott August 2, 2022 14:17

crtrott requested changes Aug 2, 2022

View reviewed changes

Don't change nvcc_wrapper

882076b

masterleinad requested a review from crtrott August 2, 2022 19:48

dalg24 approved these changes Aug 3, 2022

View reviewed changes

PhilMiller approved these changes Aug 3, 2022

View reviewed changes

masterleinad mentioned this pull request Aug 3, 2022

Avoid instantiating default constructor of value type when WithoutInitializing is given #5307

Merged

crtrott approved these changes Aug 3, 2022

View reviewed changes

crtrott merged commit d052ce8 into kokkos:develop Aug 3, 2022

masterleinad mentioned this pull request Sep 7, 2022

CHANGELOG: 4.0 #5439

Closed

Require C++17 #5277

Require C++17 #5277

Conversation

masterleinad commented Jul 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilMiller Jul 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

crtrott left a comment

Choose a reason for hiding this comment

masterleinad commented Jul 27, 2022

crtrott commented Jul 27, 2022

PhilMiller commented Jul 27, 2022

PhilMiller commented Jul 27, 2022

masterleinad commented Jul 29, 2022

masterleinad commented Jul 29, 2022

masterleinad commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad Aug 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Aug 2, 2022

crtrott left a comment

Choose a reason for hiding this comment

masterleinad commented Aug 2, 2022

masterleinad commented Aug 3, 2022

masterleinad commented Jul 27, 2022 •

edited

PhilMiller Jul 27, 2022 •

edited

masterleinad Aug 2, 2022 •

edited