Fixes for Kokkos::Array #6372

nliber · 2023-08-23T00:12:12Z

(Part of issue #6355)

This P/R contains fixes for

Kokkos::Array<T, KOKKOS_INVALID_INDEX, Kokkos::Array<>::contiguous>::empty()
Kokkos::Array<T, KOKKOS_INVALID_INDEX, Kokkos::Array<>::strided>::empty()
Kokkos::Array<T, KOKKOS_INVALID_INDEX, Kokkos::Array<>::strided>::operator=(...)

As well as unit tests for the above and the rest of Kokkos::Array.

Added 2023-Aug-23:

Changed the return value for Kokkos::Array<T, 0>::data() from pointer(0) to nullptr. Technically this wasn't a bug, but it did trigger warnings under some builds.

PhilMiller · 2023-08-23T13:36:57Z

Code looks fine, compilers are being silly about char subscripting and need to be addressed, though

nliber · 2023-08-23T18:58:00Z

Code looks fine, compilers are being silly about char subscripting and need to be addressed, though

@PhilMiller sigh. I had addressed that for clang, but not gcc. Anyway, fixed.

PhilMiller · 2023-08-23T21:47:26Z

You've still got some CUDA and HIP build failures in Jenkins.

nliber · 2023-08-25T02:09:05Z

You've still got some CUDA and HIP build failures in Jenkins.

Testing fix now...

PhilMiller · 2023-08-25T15:35:23Z

The OpenMPTarget failure is noise.

The CUDA-Clang failure seems to be from the compiler thinking that is_integral_v<__int128> == false! I think that's probably in line with the standard, but maybe requires changing the test.

masterleinad · 2023-08-25T15:56:52Z

core/unit_test/TestArrayOps.hpp

+#if defined(__clang__)
+  if constexpr (std::is_integral_v<__int128>) {
+    auto i128 = static_cast<__int128>(index);
+    ASSERT_EQ(a[i128], a[index]);
+    ASSERT_EQ(ca[i128], a[index]);
+
+    auto u128 = static_cast<unsigned __int128>(index);
+    ASSERT_EQ(a[u128], a[index]);
+    ASSERT_EQ(ca[u128], a[index]);
+  }
+#endif


We don't use or test __int128 anywhere else in Kokkos. Why do you care about it here?

I was trying to have a complete set of is_integral tests, as that is what the functions are currently templated on. This is a publicly available class, so it should work with compilers that consider __int128 to be an integral type.

I would just drop it. __int128 isn't portable anyway.

ldh4 · 2023-08-25T20:07:43Z

core/src/Kokkos_Array.hpp

@@ -199,7 +199,7 @@ struct Array<T, KOKKOS_INVALID_INDEX, Array<>::contiguous> {
  using const_pointer   = std::add_const_t<T>*;

  KOKKOS_INLINE_FUNCTION constexpr size_type size() const { return m_size; }
-  KOKKOS_INLINE_FUNCTION constexpr bool empty() const { return 0 != m_size; }
+  KOKKOS_INLINE_FUNCTION constexpr bool empty() const { return 0 == m_size; }


I'm surprised that this didn't cause any problem elsewhere. Was it just never used or never tested before?

Definitely never tested before. Calling empty() only makes sense in either generic code or with data structures whose size is determined at run time. The latter is only true for the Proxy partial specialisations of Kokkos::Array.

I suspect no one uses Kokkos::Array<T, KOKKOS_INVALID_INDEX, Proxy> directly. As I state in Issue #6355, it has very unusual semantics (unlike Kokkos::Array<T, N>) as it is non-owning with weird (and untested until now) semantics for assignment.

nliber · 2023-08-25T21:42:16Z

The OpenMPTarget failure is noise.

The CUDA-Clang failure seems to be from the compiler thinking that is_integral_v<__int128> == false! I think that's probably in line with the standard, but maybe requires changing the test.

Until now, I thought clang considered __int128 to be an integral type, while gcc does not. (I believe gcc is correct with regards to the standard.). It looks like that isn't true for the CUDA part of the build.

dalg24 · 2023-09-01T12:31:55Z

core/unit_test/TestArrayOps.hpp

+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wchar-subscripts"
+#endif
+
+#if defined(__GNUC__) && !defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wchar-subscripts"
+#endif


Why do we bother with accesses with all integral types known to men?

I'd like to eventually change it to is_convertible_v<T, size_t> (which is closer to how std::array works). Because it has been around a long time, I don't feel comfortable making that change w/o a full set of tests to make sure nothing is missing or generates other warnings.

std::array just uses size_t. I'm assuming the is_integral was used in Array to eliminate compiler warnings that the implicit conversion sometimes generates.

Could array_element_access test be wrapped into a type-parametrized test?
https://github.com/google/googletest/blob/main/docs/advanced.md#type-parameterized-tests

Ok, I couldn't resist and tried to rewrite it on my own. Consider cz4rs@be291da, which would produce following test output:

Note: Google Test filter = *ArrayOpsTest* [==========] Running 24 tests from 12 test suites. [----------] Global test environment set-up. [----------] 2 tests from serial/ArrayOpsTest/0, where TypeParam = signed char [ RUN ] serial/ArrayOpsTest/0.array_element_access [ OK ] serial/ArrayOpsTest/0.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/0.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/0.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/0 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/1, where TypeParam = unsigned char [ RUN ] serial/ArrayOpsTest/1.array_element_access [ OK ] serial/ArrayOpsTest/1.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/1.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/1.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/1 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/2, where TypeParam = short [ RUN ] serial/ArrayOpsTest/2.array_element_access [ OK ] serial/ArrayOpsTest/2.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/2.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/2.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/2 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/3, where TypeParam = unsigned short [ RUN ] serial/ArrayOpsTest/3.array_element_access [ OK ] serial/ArrayOpsTest/3.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/3.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/3.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/3 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/4, where TypeParam = int [ RUN ] serial/ArrayOpsTest/4.array_element_access [ OK ] serial/ArrayOpsTest/4.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/4.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/4.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/4 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/5, where TypeParam = unsigned int [ RUN ] serial/ArrayOpsTest/5.array_element_access [ OK ] serial/ArrayOpsTest/5.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/5.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/5.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/5 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/6, where TypeParam = long [ RUN ] serial/ArrayOpsTest/6.array_element_access [ OK ] serial/ArrayOpsTest/6.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/6.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/6.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/6 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/7, where TypeParam = unsigned long [ RUN ] serial/ArrayOpsTest/7.array_element_access [ OK ] serial/ArrayOpsTest/7.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/7.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/7.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/7 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/8, where TypeParam = long long [ RUN ] serial/ArrayOpsTest/8.array_element_access [ OK ] serial/ArrayOpsTest/8.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/8.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/8.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/8 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/9, where TypeParam = unsigned long long [ RUN ] serial/ArrayOpsTest/9.array_element_access [ OK ] serial/ArrayOpsTest/9.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/9.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/9.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/9 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/10, where TypeParam = (anonymous namespace)::Enum [ RUN ] serial/ArrayOpsTest/10.array_element_access [ OK ] serial/ArrayOpsTest/10.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/10.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/10.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/10 (0 ms total) [----------] 2 tests from serial/ArrayOpsTest/11, where TypeParam = (anonymous namespace)::EnumShort [ RUN ] serial/ArrayOpsTest/11.array_element_access [ OK ] serial/ArrayOpsTest/11.array_element_access (0 ms) [ RUN ] serial/ArrayOpsTest/11.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/11.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/11 (0 ms total) [----------] Global test environment tear-down [==========] 24 tests from 12 test suites ran. (0 ms total) [ PASSED ] 24 tests.

Interesting...

The error messages look fine as well (as they show what TypeParam is on the FAILED line:

[----------] 2 tests from serial/ArrayOpsTest/0, where TypeParam = signed char [ RUN ] serial/ArrayOpsTest/0.array_element_access /Users/nliber/git/github.com/kokkos/kokkos-array/core/unit_test/TestArrayOps.hpp:48: Failure Expected: (a[cast_index]) != (a[index]), actual: 5 vs 5 [ FAILED ] serial/ArrayOpsTest/0.array_element_access, where TypeParam = signed char (0 ms) [ RUN ] serial/ArrayOpsTest/0.array_contiguous_element_access [ OK ] serial/ArrayOpsTest/0.array_contiguous_element_access (0 ms) [----------] 2 tests from serial/ArrayOpsTest/0 (0 ms total)

That was my concern with using C++ templates for these tests (if you don't get the template parameter name when the test fails, it is hard to track down what actually broke), but this looks fine.

Unfortunately, some of the GoogleTest macros involved with the type parameterized tests don't work across our compiler suite. Reverting...

Oh, I've never tried to do full CI run :| In that case I think this test is good as it is.

pointer(0), as this eliminates some compiler warnings

(left unsigned char and signed char, as uint8_t and int8_t are sometimes aliases for them) Changed EnumBool to EnumShort as it is slightly more realistic

cz4rs

Looks good to me.

dalg24 · 2023-09-13T13:56:53Z

Timeouts HIP builds and SYCL build

dalg24 · 2023-09-13T13:57:08Z

Make sure you update the 4.2 changelog

This was referenced Aug 23, 2023

Kokkos::Array deduction guide #6373

Merged

Adding Kokkos::to_Array #6374

Closed

Adding Kokkos::to_array #6375

Merged

Improvements to Kokkos::Array (including a bug fix!) #6280

Closed

nliber marked this pull request as ready for review August 23, 2023 00:41

PhilMiller approved these changes Aug 23, 2023

View reviewed changes

masterleinad reviewed Aug 25, 2023

View reviewed changes

ldh4 reviewed Aug 25, 2023

View reviewed changes

nliber force-pushed the arrayfixes branch 2 times, most recently from 0a13e4a to f957041 Compare August 30, 2023 22:40

dalg24 reviewed Sep 1, 2023

View reviewed changes

nliber force-pushed the arrayfixes branch from f957041 to b15e0d3 Compare September 7, 2023 00:37

nliber added 11 commits September 12, 2023 17:34

WIP: Adding unit tests for Kokkos::Array

f16fdf4

Remove ScopedEnum tests (as they don't compile)

01e8015

Fix for empty() bug with contiguous and strided Kokkos::Array

3a1749f

Fixed strided Kokkos::Array assignment operators

9820dca

Clean up unit tests

4543372

Fix for char subscript warning during Kokkos::Array unit tests

511be33

Changed Kokkos::Array<T, 0>::data() to return nullptr instead of

da9b1e5

pointer(0), as this eliminates some compiler warnings

Fixes for __int128 and CUDA-11.0.3 build

0ef60c3

Removed tests for __int128

6e2c048

Fixed ch (which should be c8) for char8_t indexing tests

999e0ca

Removed char and *char_t tests

0d4f701

(left unsigned char and signed char, as uint8_t and int8_t are sometimes aliases for them) Changed EnumBool to EnumShort as it is slightly more realistic

nliber force-pushed the arrayfixes branch from b15e0d3 to 6159f42 Compare September 13, 2023 00:39

nliber force-pushed the arrayfixes branch from 6159f42 to 0d4f701 Compare September 13, 2023 01:29

cz4rs approved these changes Sep 13, 2023

View reviewed changes

dalg24 merged commit 615fc1a into kokkos:develop Sep 13, 2023
28 of 29 checks passed

masterleinad mentioned this pull request Sep 14, 2023

CHANGELOG: 4.2.0 #6197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for Kokkos::Array #6372

Fixes for Kokkos::Array #6372

nliber commented Aug 23, 2023 •

edited

PhilMiller commented Aug 23, 2023

nliber commented Aug 23, 2023 •

edited

PhilMiller commented Aug 23, 2023

nliber commented Aug 25, 2023

PhilMiller commented Aug 25, 2023

masterleinad Aug 25, 2023

nliber Aug 25, 2023 •

edited

masterleinad Aug 25, 2023

nliber Aug 30, 2023

ldh4 Aug 25, 2023

nliber Aug 25, 2023 •

edited

nliber commented Aug 25, 2023

dalg24 Sep 1, 2023

nliber Sep 6, 2023

cz4rs Sep 6, 2023

cz4rs Sep 7, 2023

nliber Sep 12, 2023 •

edited

nliber Sep 13, 2023

cz4rs Sep 13, 2023

cz4rs left a comment

dalg24 commented Sep 13, 2023

dalg24 commented Sep 13, 2023

Fixes for Kokkos::Array #6372

Fixes for Kokkos::Array #6372

Conversation

nliber commented Aug 23, 2023 • edited

PhilMiller commented Aug 23, 2023

nliber commented Aug 23, 2023 • edited

PhilMiller commented Aug 23, 2023

nliber commented Aug 25, 2023

PhilMiller commented Aug 25, 2023

Choose a reason for hiding this comment

nliber Aug 25, 2023 • 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

nliber Aug 25, 2023 • edited

Choose a reason for hiding this comment

nliber commented Aug 25, 2023

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

nliber Sep 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cz4rs left a comment

Choose a reason for hiding this comment

dalg24 commented Sep 13, 2023

dalg24 commented Sep 13, 2023

nliber commented Aug 23, 2023 •

edited

nliber commented Aug 23, 2023 •

edited

nliber Aug 25, 2023 •

edited

nliber Aug 25, 2023 •

edited

nliber Sep 12, 2023 •

edited