Cpp14 support by oliverlee · Pull Request #314 · kokkos/mdspan

oliverlee · 2024-02-10T04:02:43Z

I've updated the P0009 headers to compile without C++17 extensions.

For the last commit, I've dumped a few common utilities in the extents header. Please feel free to edit as desired.

mhoemmen · 2024-02-12T05:50:37Z

include/experimental/__p0009_bits/extents.hpp

 #endif
 #include <array>
+#include <type_traits>
+#include <stdexcept>


Nothing in this header threw an std::exception subclass before. Furthermore, the only reason this code now throws an exception is to trigger a call to terminate in a noexcept function.

Would you consider just calling std::abort as before? Alternately, would you consider adding a configurable precondition violation handler (e.g., a macro that takes a message and just calls std::abort by default)?

Would you consider just calling std::abort as before? Alternately, would you consider adding a configurable precondition violation handler (e.g., a macro that takes a message and just calls std::abort by default)?

Sure that sounds like a better approach. I'm also wondering if extents.hpp is the right location for the terminate_if_invalid_strides function. Maybe layout_stride.hpp is a better place?

mhoemmen · 2024-02-12T05:52:14Z

include/experimental/__p0009_bits/extents.hpp

+  static_assert(std::is_same<Layout, layout_left>::value or
+                std::is_same<Layout, layout_right>::value
+                , "");


Would you consider adding a nonempty message here?

mhoemmen · 2024-02-12T06:00:05Z

include/experimental/__p0009_bits/fwd.hpp

+template <std::size_t N>
+struct with_rank {};


Would you consider making this an alias of std::integral_constant, so as to avoid more instantiations of types?

mhoemmen · 2024-02-12T06:05:29Z

Thanks for the contribution! There are some nice refactorings in there as well. @crtrott et al. will likely want to comment.

Do mdspan's tests and examples all build with C++14, or do these changes only just make your application build? We're generally only interested in the former.
Would you consider adding C++14 testing to https://github.com/kokkos/mdspan/blob/stable/.github/workflows/cmake.yml as a part of this PR? Current PR automatic testing will not exercise C++14.

oliverlee · 2024-02-12T16:32:44Z

1. Do mdspan's tests and examples all build with C++14, or do these changes only just make your application build?  We're generally only interested in the former.

I've only tested with the tests defined in CMake along with C++14 so far. All tests pass before and after this PR - however after this PR now builds without warnings.

2. Would you consider adding C++14 testing to https://github.com/kokkos/mdspan/blob/stable/.github/workflows/cmake.yml as a part of this PR?  Current PR automatic testing will not exercise C++14.

Sure.

mhoemmen · 2024-02-13T15:53:30Z

Thanks for making the changes! Building with C++14 results in the following CMake error message.

CMake Error at CMakeLists.txt:200 (MESSAGE):
  Benchmarks are not available in C++14 or C++23 mode.  Turn
  MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.

I think you could just change line 200 of CMakeLists.txt (

mdspan/CMakeLists.txt

Line 200 in 0e6a69d

    
               MESSAGE(FATAL_ERROR "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")

) from what it is currently,

MESSAGE(FATAL_ERROR "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")

to something like

set(MDSPAN_ENABLE_BENCHMARKS OFF)
MESSAGE(STATUS "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")

oliverlee · 2024-02-13T17:17:50Z

Thanks for making the changes! Building with C++14 results in the following CMake error message.
CMake Error at CMakeLists.txt:200 (MESSAGE):
  Benchmarks are not available in C++14 or C++23 mode.  Turn
  MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.
I think you could just change line 200 of CMakeLists.txt (

mdspan/CMakeLists.txt

Line 200 in 0e6a69d

MESSAGE(FATAL_ERROR "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")

) from what it is currently,
MESSAGE(FATAL_ERROR "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")
to something like
set(MDSPAN_ENABLE_BENCHMARKS OFF)
MESSAGE(STATUS "Benchmarks are not available in C++14 or C++23 mode. Turn MDSPAN_ENABLE_BENCHMARKS OFF or use C++17 or C++20.")

I disabled benchmarking for C++14 using the same approach for C++23 (i.e. via matrix settings in cmake.yml) since I thought it would be simpler to maintain. Let me know if you still prefer a CMake option based approach.

crtrott

This looks pretty good. Finally found sufficient time to review it. The new macros are not a bad idea, but I think they should all be IMPL macros.

include/experimental/__p0009_bits/extents.hpp

include/experimental/__p0009_bits/macros.hpp

tests/test_alternate_precondition_violation_handler.cpp

tests/test_macros.cpp

crtrott · 2024-05-02T20:41:39Z

I will rebase this and merge my suggestions.

crtrott · 2024-05-02T23:39:56Z

@oliverlee FYI: I rebased and force pushed. I'd like to get this in before I work on more of the features for C++26.

oliverlee · 2024-05-03T00:11:34Z

Not sure why the death tests are failing now - they were fine a few months back.

I'll probably have time to look into it tomorrow.

crtrott · 2024-05-03T01:12:29Z

They passed now. We might have changed Debug to Release build at some point and so the rebase made them fail.

oliverlee · 2024-05-03T02:14:36Z

It should be possible to enable precondition checks in release mode with a preprocessor macro - I imagine this is enabled only for tests and disabled for benchmarks.

It shouldn't be too much trouble to do in a separate PR if you want to merge this PR and use it as a base for c++26 stuff.

oliverlee · 2024-05-03T15:48:15Z

@crtrott It looks like the macro in test/CMakeLists.txt was missed in the rename commit.

I've created two commits - please squash or do whatever is normal for this repo.

crtrott · 2024-05-03T15:53:23Z

Thanks! I had overlooked that. That said I am not sure it's the best way to go about this. Now the tests never take the code path where the preconditions are skipped. Maybe instead we should have a mix of debug and non-debug test configs.

crtrott · 2024-05-03T15:53:48Z

Also I am waiting for a second review to finish (should happen latest by Monday I pinged appropriate people in my team).

oliverlee · 2024-05-03T16:00:30Z

That said I am not sure it's the best way to go about this.

Same - whatever makes the most sense to you in terms of maintenance.

I've seen precondition tests defined in a separate test file. That could remove the need to conditionally define test cases with ASSERT_DEATH and simplify running tests with debug and non-debug configs. Would that make sense here?

crtrott · 2024-05-03T16:34:47Z

It could make sense to separate them out. We had some platforms where the death tests never fully worked ...

oliverlee · 2024-05-03T16:53:37Z

It could make sense to separate them out. We had some platforms where the death tests never fully worked ...

There are only a few DEATH_TESTS now.

It makes sense to do it earlier rather than later if this is approach you want to pursue.

crtrott · 2024-05-03T16:58:45Z

Yeah let's do it. Do you have time to tackle that? If not I might get around to it some point this weekend.

oliverlee · 2024-05-03T17:05:40Z

Yeah let's do it. Do you have time to tackle that? If not I might get around to it some point this weekend.

Maybe this afternoon? If not, also this weekend.

crtrott · 2024-05-03T17:09:16Z

Sounds good. Also I send you an invite to our slack channel in case you are interested.

dalg24 · 2024-05-06T15:22:34Z

include/experimental/__p0009_bits/macros.hpp

+namespace detail {
+
+template <bool check = MDSPAN_IMPL_CHECK_PRECONDITION>
+constexpr void precondition(const char* cond, const char* file, unsigned line)


I think this needs to be decorated with MDSPAN_FUNCTION

Did you consider tag dispatching instead ,having two non templated overloads

MDSPAN_FUNCTION constexpr void precondition(std::false_type /*checked*/, const char* /*cond*/, const char* /*file*/, unsigned /*line*/) { /* do nothing */ } MDSPAN_FUNCTION constexpr void precondition(std::true_type /*checked*/, const char* cond, const char* file, unsigned line) { MDSPAN_IMPL_PRECONDITION_VIOLATION_HANDLER(cond, file, line); }

I might have a while back? Not sure.

But I tested it out just now, and this approach produces warnings with Clang and GCC:

Clang-18:

.../mdspan/include/mdspan/../experimental/__p0009_bits/macros.hpp:142:16: error: constexpr function never produces a constant expression [-Winvalid-constexpr] 142 | constexpr void precondition(std::true_type, const char* cond, const char* file, unsigned line)

GCC-11:

.../mdspan/include/mdspan/../experimental/__p0009_bits/macros.hpp:120:81: error: call to non-‘constexpr’ function ‘void Kokkos::detail::default_precondition_violation_handler(const char*, const char*, unsigned int)’

I expect the issue is the constexpr specifier

I suppose you needed it to be call it in other functions that themselves are callable in a constant expression...

include/experimental/__p0009_bits/utility.hpp

dalg24 · 2024-05-06T15:54:39Z

include/experimental/__p0009_bits/macros.hpp

+namespace MDSPAN_IMPL_STANDARD_NAMESPACE {
+namespace detail {
+
+inline void default_precondition_violation_handler(const char* cond, const char* file, unsigned line)


Fine by me but just noting that the default handler would not work with CUDA/HIP

Please feel free to make any changes if CUDA/HIP compatibility is necessary.

printf works just fine with CUDA; it's HIP that's broken lol ; - )

it is a fprintf and it is qualified so there is no way it works on CUDA. As far as I know HIP behaves just the same as CUDA here.

dalg24 · 2024-05-06T16:53:44Z

include/experimental/__p2642_bits/layout_padded_fwd.hpp

+    layout_padded_constants<typename _OtherMapping::layout_type,
+                            _ExtentsType>::padded_stride_idx;
+  constexpr auto extent_to_pad_idx = layout_padded_constants<typename _OtherMapping::layout_type, _ExtentsType>::extent_to_pad_idx;
+  assert(other_mapping.stride(padded_stride_idx) == other_mapping.extents().extent(extent_to_pad_idx));


Should we not be using MDSPAN_IMPL_PRECONDITION here?

Sure let's do that.

dalg24 · 2024-05-06T17:01:45Z

tests/CMakeLists.txt

  target_link_libraries(${name} mdspan gtest_main)
  add_test(${name} ${name})
-endmacro()
+  set_property(TARGET ${name} PROPERTY COMPILE_WARNING_AS_ERROR ON)


I am not necessarily opposed to this but IMO it does not belong to this PR

This change is related to C++14 support to some degree - otherwise C++17 language features are still usable with some compilers when building for C++14. But I also see that the impact of this change has wider consequences.

What do you think about breaking out this change into a separate PR and rebasing this PR on top of that? I can do that later this evening - or feel free to change this PR as you see fit.

Christian did you mean to keep this?

tests/test_alternate_precondition_violation_handler.cpp

tests/test_macros.cpp

mhoemmen · 2024-05-06T22:41:31Z

include/experimental/__p0009_bits/macros.hpp

+{
+  (void)std::fprintf(::stderr, "%s:%u: precondition failure: `%s`\n", file, line, cond);
+  std::abort();
+}


I don't know how HIP or SYCL work, but the following should be fine for CUDA. Note that in device code, printf may have a performance cost even if it is never called at run time. (For example, the compiler may need to reserve registers for it.)

Suggested change

{

(void)std::fprintf(::stderr, "%s:%u: precondition failure: `%s`\n", file, line, cond);

std::abort();

}

{

#if defined(__CUDA_ARCH__)

# if ! defined(NDEBUG)

printf("%s:%u: precondition failure: `%s`\n", file, line, cond);

assert(false);

# endif

#else

fprintf(stderr, "%s:%u: precondition failure: `%s`\n", file, line, cond);

assert(false);

#endif

}

HIP works the same as Cuda. For SYCL (oneAPI), you would need to use sycl::ext::oneapi::experimental::printf.

nmm0

I'm good with almost everything, just lets keep the padded layout stuff isolated (since it requires C++17 anyway)

include/experimental/__p0009_bits/layout_left.hpp

include/experimental/__p2642_bits/layout_padded_fwd.hpp

include/experimental/__p0009_bits/layout_right.hpp

crtrott · 2024-05-23T00:41:18Z

I just noticed that this requires CXX Extensions for GCC. I.e. -std=gnu++14 - it doesn't work with -std=c++14. Do you consider that acceptable?

Use the `_MDSPAN_FOLD_AND` macro to avoid using fold expressions when building for C++14.

Add an empty message to static assert in `layout_padded_fwd.hpp` to avoid use of C++17 extensions when building for C++14.

Replace use of constexpr if in headers used to implement p0009 in order to allow compilation for C++14 without C++17 extensions.

Move test cases that test preconditions with `ASSERT_DEATH` to separate test files. This allows other (non-precondition) tests to be compiled with or without preconditions. CMake test targets will enable preconditions during compilation if passed the option `ENABLE_PRECONDITIONS`. This only requires moving a single test case from `test_layout_ctors` - `test_alternate_precondition_violation_handler` and `test_macros` are test files have been written primary to test preconditions.

addressed

oliverlee · 2024-05-23T15:59:30Z

I just noticed that this requires CXX Extensions for GCC. I.e. -std=gnu++14 - it doesn't work with -std=c++14. Do you consider that acceptable?

Do you know what requires extensions? I didn't intend for that and I believe I am only using -std=c++14 in projects using mdspan (however, those don't use CMake).

crtrott · 2024-05-23T21:20:23Z

Its now fixed.

include/experimental/__p0009_bits/layout_stride.hpp

include/experimental/__p0009_bits/macros.hpp

dalg24 · 2024-05-23T22:36:45Z

include/experimental/__p0009_bits/macros.hpp

+MDSPAN_FUNCTION inline void default_precondition_violation_handler(const char* cond, const char* file, unsigned line)
+{
+  printf("%s:%u: precondition failure: `%s`\n", file, line, cond);
+  assert(0);


Please confirm that you did not see issue with Clang as the CUDA compiler.

Yeah I couldn't find a variant which fails and we will use in Kokkos Kokkos::abort anyway.

dalg24 · 2024-05-23T22:40:00Z

tests/CMakeLists.txt

  target_link_libraries(${name} mdspan gtest_main)
  add_test(${name} ${name})
-endmacro()
+  set_property(TARGET ${name} PROPERTY COMPILE_WARNING_AS_ERROR ON)


Christian did you mean to keep this?

tests/test_layout_preconditions.cpp

dalg24 · 2024-05-23T22:50:34Z

tests/test_mdspan_size.cpp


 TEST(TestMdspan, MdspanSizeReturnTypeAndPrecondition)
 {
+  (void) test_info_;


tests/test_macros.cpp

dalg24

Good enough.
Thank you Oliver.

oliverlee force-pushed the cpp14-support branch from ece2592 to e2f47b2 Compare February 10, 2024 04:02

mhoemmen reviewed Feb 12, 2024

View reviewed changes

oliverlee force-pushed the cpp14-support branch 3 times, most recently from 5c44070 to 22258de Compare February 12, 2024 21:30

oliverlee force-pushed the cpp14-support branch from 22258de to 38d3f2f Compare February 13, 2024 17:15

crtrott requested changes May 2, 2024

View reviewed changes

crtrott force-pushed the cpp14-support branch from 3971ec2 to 65d2066 Compare May 2, 2024 23:38

crtrott force-pushed the cpp14-support branch from 65d2066 to ffe7c27 Compare May 2, 2024 23:47

crtrott approved these changes May 2, 2024

View reviewed changes

dalg24 reviewed May 6, 2024

View reviewed changes

mhoemmen reviewed May 6, 2024

View reviewed changes

nmm0 previously requested changes May 15, 2024

View reviewed changes

include/experimental/__p0009_bits/layout_left.hpp Show resolved Hide resolved

include/experimental/__p2642_bits/layout_padded_fwd.hpp Show resolved Hide resolved

include/experimental/__p0009_bits/layout_right.hpp Show resolved Hide resolved

crtrott force-pushed the cpp14-support branch from 6a4e011 to 65bb2f2 Compare May 23, 2024 15:27

oliverlee and others added 15 commits May 23, 2024 09:28

ci: test with C++14 and treat compile warnings as errors

163d109

replace fold expressions in extents.hpp with macro

205f52a

Use the `_MDSPAN_FOLD_AND` macro to avoid using fold expressions when building for C++14.

use empty message with static assert

f55542b

Add an empty message to static assert in `layout_padded_fwd.hpp` to avoid use of C++17 extensions when building for C++14.

replace constexpr if in p0009 bits

c6826a5

Replace use of constexpr if in headers used to implement p0009 in order to allow compilation for C++14 without C++17 extensions.

define precondition macro and violation handler

08171bd

Make new precondition macros IMPL only

e1d3d69

Make test for dims C++14 compatible

6c599bc

replace std::is_signed_v for C++14 compatibility

edd5698

use MDSPAN_IMPL_PRECONDITION in layout_padded_fwd

85fe0c9

test that precondition checks can be disabled

5573da9

handle precondition error printing for multiple archs

c303fb1

Fix CUDA (and hopefully HIP/SYCL) compilation

9e0c03f

Fix CUDA build and run issues

dc0f4da

More fixes for cpp14 and warnings

fd0be5b

crtrott force-pushed the cpp14-support branch from 65bb2f2 to fd0be5b Compare May 23, 2024 15:28

Guard inclusion of layout_padded_fwd

f3a1bbb

dalg24 reviewed May 23, 2024

View reviewed changes

dalg24 approved these changes May 23, 2024

View reviewed changes

dalg24 merged commit 2f5ae3d into kokkos:stable May 23, 2024

nmm0 mentioned this pull request Feb 14, 2025

treat warnings as errors in mdspan tests #328

Closed

-{
-  (void)std::fprintf(::stderr, "%s:%u: precondition failure: `%s`\n", file, line, cond);
-  std::abort();
-}
+{
+#if defined(__CUDA_ARCH__)
+#  if ! defined(NDEBUG)
+  printf("%s:%u: precondition failure: `%s`\n", file, line, cond);
+  assert(false);
+#  endif
+#else
+  fprintf(stderr, "%s:%u: precondition failure: `%s`\n", file, line, cond);
+  assert(false);
+#endif
+}

Conversation

oliverlee commented Feb 10, 2024

Uh oh!

mhoemmen Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhoemmen commented Feb 12, 2024

Uh oh!

oliverlee commented Feb 12, 2024

Uh oh!

mhoemmen commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverlee commented Feb 13, 2024

Uh oh!

crtrott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crtrott commented May 2, 2024

Uh oh!

crtrott commented May 2, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

oliverlee commented May 3, 2024

Uh oh!

crtrott commented May 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhoemmen Feb 12, 2024 •

edited

Loading

mhoemmen commented Feb 13, 2024 •

edited

Loading