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.

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

mdspan converting constructors #6830

Open
wants to merge 56 commits into
base: develop
Choose a base branch
from

Conversation

nmm0
Copy link
Contributor

@nmm0 nmm0 commented Feb 21, 2024

Closes #6805

This PR adds constructors for Kokkos::View that convert from std::mdspan.

  • If the VIew is compile-time unmanaged, the conversion is implicit (only matters in C++20)
  • Constraints: the layout is mappable to mdspan layout mappings
    • Kokkos::LayoutLeft -> std::layout_left_padded<N>::mapping
    • Kokkos::LayoutRight -> std::layout_right_padded<N>::mapping
    • Kokkos::LayoutStride -> std::layout_stride

It also adds to_mdspan and operator mdspan that converts to the "natural" mdspan type, that is the type that closely represents the internal view state.

Requirements for this PR (currently based on top of these changes):

@crtrott
Copy link
Member

crtrott commented Feb 28, 2024

Besides separating PRs for moving stuff and adding the new functionality: can't we have this all simpler?
Is there something wrong with defining the "natural mdspan type" for a View, and then having a constructor which takes exactly that View, after which we can rely largely on the convertability of the incoming mdspan to the natural mdspan?

This code snippet here defines natural mdspan

template<class L>
struct LayoutMap;

template<>
struct LayoutMap<Kokkos::LayoutRight> { using type = Kokkos::layout_right; };
template<>
struct LayoutMap<Kokkos::LayoutLeft> { using type = Kokkos::layout_left; };
template<>
struct LayoutMap<Kokkos::LayoutStride> { using type = Kokkos::layout_stride; };

template<class ViewType>
void foo() {
  using element_type = typename ViewType::value_type;
  using index_type = typename ViewType::execution_space::size_type;
  using extents_type = typename Kokkos::Experimental::Impl::ExtentsFromDataType<index_type, typename ViewType::data_type>::type;
  using layout_type = typename LayoutMap<typename ViewType::array_layout>::type;
  using mdspan_type = Kokkos::mdspan<element_type, extents_type, layout_type>;
  printf("%s\n", typeid(ViewType).name());
  printf("%s\n", typeid(mdspan_type).name());
}

And then the constructor we would need is simply:

MDSPAN_CONDITIONAL_EXPLICIT(traits::is_managed)
View(mdspan_type mds):View(mds.data_handle(), kokkos_layout_from_mapping(mds.mapping()) ) {}

Do you see anything wrong with this approach? The mdspan convertability and constructibility should enforce all the desired "this constructor works" properties wouldn't it?

@nmm0
Copy link
Contributor Author

nmm0 commented Feb 28, 2024

Besides separating PRs for moving stuff and adding the new functionality: can't we have this all simpler? Is there something wrong with defining the "natural mdspan type" for a View, and then having a constructor which takes exactly that View, after which we can rely largely on the convertability of the incoming mdspan to the natural mdspan?

This code snippet here defines natural mdspan

template<class L>
struct LayoutMap;

template<>
struct LayoutMap<Kokkos::LayoutRight> { using type = Kokkos::layout_right; };
template<>
struct LayoutMap<Kokkos::LayoutLeft> { using type = Kokkos::layout_left; };
template<>
struct LayoutMap<Kokkos::LayoutStride> { using type = Kokkos::layout_stride; };

template<class ViewType>
void foo() {
  using element_type = typename ViewType::value_type;
  using index_type = typename ViewType::execution_space::size_type;
  using extents_type = typename Kokkos::Experimental::Impl::ExtentsFromDataType<index_type, typename ViewType::data_type>::type;
  using layout_type = typename LayoutMap<typename ViewType::array_layout>::type;
  using mdspan_type = Kokkos::mdspan<element_type, extents_type, layout_type>;
  printf("%s\n", typeid(ViewType).name());
  printf("%s\n", typeid(mdspan_type).name());
}

And then the constructor we would need is simply:

MDSPAN_CONDITIONAL_EXPLICIT(traits::is_managed)
View(mdspan_type mds):View(mds.data_handle(), kokkos_layout_from_mapping(mds.mapping()) ) {}

Do you see anything wrong with this approach? The mdspan convertability and constructibility should enforce all the desired "this constructor works" properties wouldn't it?

@crtrott Yeah I think the simpler approach is better, I was trying to make the API like mdarray but thinking about it a bit more I realized that's not what we want. Actually the approach I have right now doesn't have the right constraints anyway.

@nmm0
Copy link
Contributor Author

nmm0 commented Feb 28, 2024

Also it still needs to be constrained on is_managed but it doesn't need to be conditionally explicit

@crtrott
Copy link
Member

crtrott commented Feb 28, 2024

I think conditional explicit is fine. We allow construction from pointer of a "managed" view.

@nmm0 nmm0 force-pushed the 6805-mdspan-conversion-operators branch 2 times, most recently from 7960ee0 to f41c2f7 Compare March 7, 2024 00:18
@nmm0 nmm0 marked this pull request as ready for review March 7, 2024 00:19
@nmm0 nmm0 requested review from crtrott, dalg24 and nliber March 7, 2024 00:23
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Extents.hpp Outdated Show resolved Hide resolved
@@ -106,6 +107,14 @@ struct DataTypeFromExtents {
// Will cause a compile error if it is malformed (i.e. dynamic after static)
using type = typename ::Kokkos::Impl::ViewDataType<T, dimension_type>::type;
};

/// Convert from a mdspan extent to a Kokkos extent, inserting 0s for static
/// extents
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want 0s for static extents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's what currently the ViewOffset does, so this would have a similar behavior

core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_ViewMapping.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Member

dalg24 commented Apr 2, 2024

Please expand the description. Referring to an issue that in turn point to an ISO C++ paper makes it difficult to audit.

core/src/Kokkos_Macros.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_ViewMapping.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_View.hpp Outdated Show resolved Hide resolved
core/src/View/MDSpan/Kokkos_MDSpan_Layout.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

A few extra minor comments on the tests

core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
core/unit_test/view/TestMDSpanConversion.hpp Outdated Show resolved Hide resolved
@nmm0 nmm0 force-pushed the 6805-mdspan-conversion-operators branch from a6c304c to bc07459 Compare April 10, 2024 22:45
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Tests were in alphabetic order and there is a MDSpan test.
I suppose you meant to define it in the same exe as the other view tests but I tend to think you should define it L187 in sources 1B.

Other than that looks good

@dalg24
Copy link
Member

dalg24 commented Apr 11, 2024

Retest this please

@dalg24
Copy link
Member

dalg24 commented Apr 11, 2024

Failure in the CUDA-11.6-NVCC-DEBUG build on Jenkins

4: [ RUN      ] cuda.view_mdspan_conversion
4: The specified run-time extent for Kokkos::View 'UNMANAGED' does not match the compile-time extent in dimension 0. The given extent is 0 but should be 7.
4: 
4: Backtrace:
4: [0x55555661903f] 
4: [0x5555565f8962] 
4: [0x5555555997ab] 
4: [0x555556116fd1] 
4: [0x55555610ebd8] 
4: [0x55555610af67] 
4: [0x5555560ef6ed] 
4: [0x5555560de0de] 
4: [0x5555560a1118] 
4: [0x555556099dfd] 
4: [0x5555560987af] 

crtrott and others added 26 commits May 15, 2024 09:35
This explicit construction:
View<int[8]> a(mdspan<int,dextents<int,1>>(ptr,8))
needs the new ctor, because the mdspan provided is not implicitly
convertible to the natural mdspan type.
@nmm0 nmm0 force-pushed the 6805-mdspan-conversion-operators branch from c143450 to 53efc51 Compare May 15, 2024 16:35
@masterleinad masterleinad requested a review from crtrott May 15, 2024 18:30
@crtrott
Copy link
Member

crtrott commented May 16, 2024

Retest this please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add conversion operators from/to View and mdspan
5 participants