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.

Sign up for GitHub

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

Jump to bottom

Fix mdspan standard deviations #286

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Fix mdspan standard deviations #286

merged 4 commits into from
Nov 1, 2023

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Oct 27, 2023

No description provided.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
include/experimental/__p0009_bits/mdspan.hpp
MDSPAN_CONDITIONAL_EXPLICIT(
!_MDSPAN_TRAIT(std::is_convertible, const typename OtherLayoutPolicy::template mapping<OtherExtents>&, mapping_type) ||
!_MDSPAN_TRAIT(std::is_convertible, const OtherAccessor&, accessor_type)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

explicitness of the ctor was not specified while it should.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
include/experimental/__p0009_bits/mdspan.hpp
_MDSPAN_TRAIT(std::is_convertible, SizeType, index_type) &&
_MDSPAN_TRAIT(std::is_nothrow_constructible, index_type, SizeType)
_MDSPAN_TRAIT(std::is_convertible, const SizeType&, index_type) &&
_MDSPAN_TRAIT(std::is_nothrow_constructible, index_type, const SizeType&)
Copy link
Member Author

Choose a reason for hiding this comment

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

wrong type used for constraints - the llvm tests check for the difference.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
include/experimental/__p0009_bits/mdspan.hpp
@@ -311,7 +322,7 @@ class mdspan
#endif // __cpp_lib_span
#endif // MDSPAN_USE_PAREN_OPERATOR

MDSPAN_INLINE_FUNCTION constexpr size_t size() const noexcept {
MDSPAN_INLINE_FUNCTION constexpr size_type size() const noexcept {
Copy link
Member Author

Choose a reason for hiding this comment

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

this returns size_type not size_t. Note that actually makes one of our old tests not work, I removed that thing.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
include/experimental/__p0009_bits/mdspan.hpp
MDSPAN_INLINE_FUNCTION constexpr bool is_strided() const noexcept { return __mapping_ref().is_strided(); };
MDSPAN_INLINE_FUNCTION constexpr bool is_unique() const { return __mapping_ref().is_unique(); };
MDSPAN_INLINE_FUNCTION constexpr bool is_exhaustive() const { return __mapping_ref().is_exhaustive(); };
MDSPAN_INLINE_FUNCTION constexpr bool is_strided() const { return __mapping_ref().is_strided(); };
MDSPAN_INLINE_FUNCTION constexpr index_type stride(size_t r) const { return __mapping_ref().stride(r); };
Copy link
Member Author

Choose a reason for hiding this comment

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

These are not actually noexcept in the standard.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
include/experimental/__p0009_bits/mdspan.hpp
@@ -374,7 +385,7 @@ class mdspan
#if defined(_MDSPAN_USE_CLASS_TEMPLATE_ARGUMENT_DEDUCTION)
MDSPAN_TEMPLATE_REQUIRES(
class ElementType, class... SizeTypes,
/* requires */ _MDSPAN_FOLD_AND(_MDSPAN_TRAIT(std::is_integral, SizeTypes) /* && ... */) &&
/* requires */ _MDSPAN_FOLD_AND(_MDSPAN_TRAIT(std::is_convertible, SizeTypes, size_t) /* && ... */) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

wrong constraint on deduction guide.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
tests/libcxx-backports/CustomTestLayouts.h
@@ -177,6 +177,7 @@ class layout_wrapping_integral<WrapArg>::mapping {

#if MDSPAN_HAS_CXX_23
friend constexpr void swap(mapping& x, mapping& y) noexcept {
using std::swap;
Copy link
Member Author

Choose a reason for hiding this comment

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

extent is not in namespace std anymore, so need to make this findable.

crtrott
crtrott commented Oct 27, 2023
View reviewed changes
tests/test_mdspan_size.cpp
@@ -58,9 +58,6 @@ TEST(TestMdspan, MdspanSizeReturnTypeAndPrecondition)

static_assert(std::numeric_limits<std::int8_t>::max() == 127, "max int8_t != 127");
test_mdspan_size(storage, Kokkos::extents<std::int8_t, 12, 11>{}); // 12 * 11 == 132

static_assert(std::numeric_limits<std::uint8_t>::max() == 255, "max uint8_t != 255");
test_mdspan_size(storage, Kokkos::extents<std::uint8_t, 16, 17>{}); // 16 * 17 == 272
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't supposed to work according to standard, because size() returns size_type, the unsigned version of index_type

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that change/comment.
Everything else looks good.

Copy link
Member Author

Choose a reason for hiding this comment

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

We previously returned size_t so mdspan::size() would work even if it overflowed uint8_t, but mdspan size return the unsigned version of index_type: so we can't return 272

Copy link
Member

Choose a reason for hiding this comment

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

Wanna do 16, 15 then?

nmm0
nmm0 reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Some initial comments

tests/libcxx-backports/mdspan/mdspan.index_operator.pass.cpp
@@ -38,6 +39,9 @@
#include "../ConvertibleToIntegral.h"
#include "../CustomTestLayouts.h"

// This test uses the bracket operator, but its not something we have at configure time
#if MDSPAN_USE_BRACKET_OPERATOR
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still test these with the operator() equivalents otherwise we may have holes in our test (e.g. C++20 span call but you mentioned above)

Copy link
Member Author

Choose a reason for hiding this comment

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

No I want to keep the libcxx-backports as close to the LLVM files as possible. I.e. the diffs should be minimal.

include/experimental/__p0009_bits/mdspan.hpp Show resolved Hide resolved
nmm0
nmm0 previously requested changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

I think there are a few instances where we are still using clang version macros to test for features such as https://github.com/kokkos/mdspan/pull/286/files#diff-1d6fc31052dd79dccd274528da7249a98a2ab178fc57087164a4752a64fc83b5R90-R94 that won't work with other compilers

tests/libcxx-backports/mdspan/mdspan.index_operator.pass.cpp Show resolved Hide resolved
@nmm0 nmm0 dismissed their stale review October 30, 2023 16:57

Will re-review

@crtrott crtrott merged commit 417204e into kokkos:stable Nov 1, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmm0 nmm0 nmm0 left review comments

@dalg24 dalg24 dalg24 approved these changes

@mhoemmen mhoemmen mhoemmen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crtrott @dalg24 @mhoemmen @nmm0