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

Add P3242: Copy and fill for mdspan #453

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

nmm0
Copy link

@nmm0 nmm0 commented Mar 21, 2024

No description provided.

```c++
template<class SrcElementType, class SrcExtents, class SrcLayoutPolicy, class SrcAccessorPolicy,
class DstElementType, class DstExtents, class DstLayoutPolicy, class DstAccessorPolicy>
void copy(const mdspan<SrcElementType, SrcExtents, SrcLayoutPolicy, SrcAccessorPolicy> &src, const mdspan<DstElementType, DstExtents, DstLayoutPolicy, DstAccessorPolicy> &dst)
Copy link
Contributor

Choose a reason for hiding this comment

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

mdspans are views; would you therefore consider passing them by value, as is done in P1673?

Suggested change
void copy(const mdspan<SrcElementType, SrcExtents, SrcLayoutPolicy, SrcAccessorPolicy> &src, const mdspan<DstElementType, DstExtents, DstLayoutPolicy, DstAccessorPolicy> &dst)
void copy(mdspan<SrcElementType, SrcExtents, SrcLayoutPolicy, SrcAccessorPolicy> src, mdspan<DstElementType, DstExtents, DstLayoutPolicy, DstAccessorPolicy> dst);

Copy link
Author

Choose a reason for hiding this comment

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

revisiting this, because even though they are views, the size of the mdspan is proportional to the number of dynamic extents; so it's very possible to have mdspans that are not passible in registers. I'm happy to keep it as we have in other papers, but this particular aspect might get challenged

Copy link
Contributor

Choose a reason for hiding this comment

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

@nmm0 I don't really mind either way. For std::linalg, all the mdspan are no more than rank 2, but here, they might have arbitrary rank.

I suspect LEWG would want to hear that argument. They might also want some performance measurements to justify a difference from the status quo.


However, existing standard library facilities are not sufficient here. Currently, `mdspan` does not have iterators or ranges that represent the span of the `mdspan`. Additionally, it's not entirely clear what this would entail.

Moreover, the manner in which an `mdspan` is copied (or filled) is highly performance sensitive, particularly in regards to caching behavior when traversing mdspan memory. A naïve user implementation is easy to get wrong in addition to being tedious for higher rank `mdspan`s. Ideally, an implementation would be free to use information about the layout of the `mdspan` known at compile time to perform optimizations; e.g. a continuous span `mdspan` copy for trivial types could be implementeed with a `memcpy`.
Copy link
Member

@dalg24 dalg24 Mar 22, 2024

Choose a reason for hiding this comment

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

Naive implementation is not tedious

for i0 in [0, extents(0)
  for i1 in [0, extents(1)
    ...
      for iK in [0, extents(K)
  dst[i0, i1, ..., iK] = src[i0, i1, ..., iK];

just (sometimes) really inefficient.

Copy link
Author

Choose a reason for hiding this comment

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

Haha well, I think writing an 8-nested for loop is pretty tedious :P. I guess it's a matter of opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

I already submitted a PR to kokkos/mdspan that does generic iteration over extents of any rank. It's not really tedious, but would be really inefficient without at least the specializations for exhaustive layouts.

mdspan_copy/mdspan_copy.md Outdated Show resolved Hide resolved
mdspan_copy/mdspan_copy.md Outdated Show resolved Hide resolved
mdspan_copy/mdspan_copy.md Outdated Show resolved Hide resolved
mdspan_copy/mdspan_copy.md Outdated Show resolved Hide resolved
@nmm0 nmm0 changed the title initial thoughts on mdspan copy Add P3242: Copy and fill for mdspan Apr 15, 2024
@nmm0 nmm0 marked this pull request as ready for review April 15, 2024 18:04
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.

4 participants