-
Notifications
You must be signed in to change notification settings - Fork 71
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
Adds submdspan_mapping for padded layouts #343
Adds submdspan_mapping for padded layouts #343
Conversation
// For layout_left as source StaticStride is static_extent(0) | ||
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_left { | ||
// Neither StaticStride nor any of the looked for extents can zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Neither StaticStride nor any of the looked for extents can zero. | |
// Neither StaticStride nor any of the provided extents can be zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doen
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_left { | ||
// Neither StaticStride nor any of the looked for extents can zero. | ||
// StaticStride never can be zero, the static_extents we are looking at are associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// StaticStride never can be zero, the static_extents we are looking at are associated with | |
// StaticStride can never be zero, the static_extents we are looking at are associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// We are reusing the same thing for layout_left and layout_left_padded | ||
// For layout_left as source StaticStride is static_extent(0) | ||
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_left { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct Compute_S_static_layout_left { | |
struct compute_s_static_layout_left { |
following the names of the other structs in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// For layout_right as source StaticStride is static_extent(Rank-1) | ||
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_right { | ||
// Neither StaticStride nor any of the looked for extents can zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Neither StaticStride nor any of the looked for extents can zero. | |
// Neither StaticStride nor any of the provided extents can be zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_right { | ||
// Neither StaticStride nor any of the looked for extents can zero. | ||
// StaticStride never can be zero, the static_extents we are looking at are associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// StaticStride never can be zero, the static_extents we are looking at are associated with | |
// StaticStride can never be zero, the static_extents we are looking at are associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// We are reusing the same thing for layout_right and layout_right_padded | ||
// For layout_right as source StaticStride is static_extent(Rank-1) | ||
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct Compute_S_static_layout_right { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct Compute_S_static_layout_right { | |
struct compute_s_static_layout_right { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// HIP needs deduction guides to have markups so we need to be explicit | ||
// NVCC 11.0 has a bug with deduction guide here, tested that 11.2 does not have | ||
// the issue But Clang-CUDA also doesn't accept the use of deduction guide so | ||
// disable it for CUDA alltogether | ||
#if defined(_MDSPAN_HAS_HIP) || defined(_MDSPAN_HAS_CUDA) | ||
std::tuple<decltype(MDSPAN_IMPL_STANDARD_NAMESPACE::detail::stride_of(slices))...>{ | ||
MDSPAN_IMPL_STANDARD_NAMESPACE::detail::stride_of(slices)...})), | ||
#else | ||
std::tuple{MDSPAN_IMPL_STANDARD_NAMESPACE::detail::stride_of(slices)...})), | ||
#endif | ||
offset | ||
}; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this style is used elsewhere as well but do we really gain anything from trying to use CTAD in some cases instead of always being explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it for now we can discuss this elsewhere. Generally we want to at least note that we are running into issues with CTAD>
tests/test_submdspan.cpp
Outdated
// MSVC seems to have an issue with taking just data here | ||
mds_org_t src(&data[0], map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is still complaining with
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: 'Kokkos::mdspan<int,ExtentsOrg,LayoutOrg,Kokkos::default_accessor<ElementType>>::mdspan': no overloaded function takes 2 arguments [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: with [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: [ [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: ExtentsOrg=Kokkos::extents<size_t>, [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: LayoutOrg=Kokkos::Experimental::layout_right_padded<4>, [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: ElementType=int [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
D:\a\mdspan\mdspan\mdspan-src\tests\test_submdspan.cpp(347,18): error C2661: ] [D:\a\mdspan\mdspan\mdspan-build\tests\test_submdspan.vcxproj]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is soo weird
c39a3e6
to
1ba37ba
Compare
This also fixes up getting the right static padding value, for some cases where submdspan_mapping of layout_left generates a layout_left_padded.
Also adds missing constexpr to assignment in layout_padded
4409714
to
451cce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only I prefer to keep the name of variables consistent
template<class Extents, size_t NumGaps, size_t StaticStride> | ||
struct compute_s_static_layout_left { | ||
// Neither StaticStride nor any of the provided extents can be zero. | ||
// StaticStride can never be zero, the static_extents we are looking at are associated with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we static_assert(StaticStride != 0)
?
// integral slice specifiers - which wouldn't be valid for zero extent | ||
template<size_t ... Idx> | ||
MDSPAN_INLINE_FUNCTION | ||
static constexpr size_t value(std::index_sequence<Idx...>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for readability we can make clear in this comment that this function returns dynamic_extent
if any extents are dynamic and otherwise the fwd-prod-of-extents for the static extents
MDSPAN_IMPL_STANDARD_NAMESPACE::detail::any_slice_out_of_bounds(this->extents(), slices...); | ||
auto offset = static_cast<size_t>( | ||
out_of_bounds ? this->required_span_size() | ||
: this->operator()(MDSPAN_IMPL_STANDARD_NAMESPACE::detail::first_of(slices)...)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is what was done before but we could just do (*this)(MDSPAN_IMPL_STANDARD_NAMESPACE::detail::first_of(slices)...));
No need to change unless you really want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This also fixes up getting the right static padding value, for some cases where submdspan_mapping of layout_left/right generates a layout_left/right_padded.