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 layout_padded example #180

Closed
wants to merge 13 commits into from

Conversation

mhoemmen
Copy link
Contributor

I added a layout_padded example. This is a strided layout that ensures a given overalignment of the stride-1 extent (either the leftmost, for column-major mode, or the rightmost, for row-major mode).

I've tested this on a few different compilers, as one can see here: https://godbolt.org/z/3Kn833dj8

  • gcc 12.1 -std=c++2b
  • clang 14.0.0 -std=c++20
  • icc 2021.5.0 -std=c++17
  • x86 (32-bit size_t!) MSVC 19.32 /std:c++latest
  • gcc 10.1 -std=c++14
  • gcc 9.4.0 with C++17 (doesn't work in the Compiler Explorer, because of how the mdspan single header was configured, but works fine with a local mdspan build)

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

We should talk about this, in case it becomes canonical. You are storing two extents objects here, even though only a single dimension is padded. I think we can simplify this. Also I don't particular like the storage order part. For example: why not simply take another layout policy as an argument, if we anyway store an inner layout? That is assuming that simply introducing layout_left_padded and layout_right_padded is not the right thing to do in the first place.

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.

Looks good for the most part

examples/layout_padded/layout_padded.cpp Outdated Show resolved Hide resolved
examples/layout_padded/layout_padded.cpp Outdated Show resolved Hide resolved
examples/layout_padded/layout_padded.cpp Outdated Show resolved Hide resolved
@mhoemmen
Copy link
Contributor Author

@crtrott wrote:

You are storing two extents objects here, even though only a single dimension is padded. I think we can simplify this.

An excellent point -- I had originally thought that we wanted to pad all but a single dimension; that would have justified the current design more in terms of code reuse. Padding just a single dimension suggests a different way to compute strides.

Also I don't particular like the storage order part. For example: why not simply take another layout policy as an argument, if we anyway store an inner layout?
...
That is assuming that simply introducing layout_left_padded and layout_right_padded is not the right thing to do in the first place.

We had a nice chat about this offline -- thanks!

@mhoemmen
Copy link
Contributor Author

mhoemmen commented Aug 30, 2022

Here is a summary of suggested revisions from this morning's offline discussion with Christian.

layout_padded and the "BLAS general layouts" in P1673 have the same mathematical properties.

  1. They all add padding -- extra, unviewed, but valid elements -- to one extent. This effectively increases one stride -- like layout_stride, but with one of the extents fixed to unit stride at compile time.
  2. The difference between these layouts and layout_stride is that the padding elements are "valid," that is, accessible. This means we can use them in optimizations -- e.g., to make copies contiguous. (This implies, in turn, that the only valid "nested layouts" for layout_padded are layout_left or layout_right. It doesn't make sense to template on a general "inner layout.")
  3. For the rank-2 case, all these layouts are invariant under any subview of contiguous rows and or columns. For the rank > 2 case, this still holds if we store (rank - 1) strides.

Here are the only differences between the current layout_padded design and P1673's BLAS general layouts.

  1. P1673 currently limits its BLAS general layouts to have rank 2. However, generalization is possible (see (3) above).
    2 layout_padded expresses padding via "overalignment factor." This is really just an interface convenience for use with aligned_accessor to make an aligned mdspan. Once layout_padded computes the padded extents, it no longer needs the overalignment factor. (We could create an alias and/or function to express a mapping from overalignment factor to padded layout.)

This suggests that we only need two new layouts to express both layout_padded and P1673's BLAS general layouts: layout_left_padded, and layout_right_padded. We don't use an enum to distinguish the two cases, for the same reason that layout_left and layout_right are separate types and not one type with an enum non-type template parameter.

As a size optimization, we only need to store the padded extents and the one input extent that was padded (and thus differs from the corresponding padded extent). We could use extents<index_type, InputExtent> (or its underlying "partially static array" representation) to represent the one input extent as either a compile-time or a run-time value.

It would make sense for submdspan of a layout_(left,right) mdspan, with the appropriate slices, to produce a layout_(left,right)_padded mdspan.

@mhoemmen mhoemmen force-pushed the Add-layout-padded-example branch 2 times, most recently from d2bc405 to 24df549 Compare September 1, 2022 17:22
@mhoemmen mhoemmen marked this pull request as draft September 1, 2022 17:22
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 1, 2022

I've converted this to a draft, so I can finish implementing the new layout_{left,right}_padded design that Christian and I discussed a few days ago. This will make it possible to unify the P1673 "BLAS general layouts" with padded layouts.

@mhoemmen mhoemmen marked this pull request as ready for review September 5, 2022 23:05
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 5, 2022

@crtrott @dalg24 @nliber This PR is ready for re-review; thanks!

This is a redesign of the previous layout_padded design,
based on a discussion with Christian Trott
about what we might like to standardize.
1. layout_left_padded::mapping(extents_type) constructor
   is now only enabled if padding != dynamic_extent,
   using a pre-C++20 technique instead of `requires`.

2. This should hopefully fix the icpc build.
1. Implement layout_right_padded

2. Improve tests to reduce code duplication
1. Do not constrain layout mapping constructors
    that just take extents to static padding;
    just use no padding in that case.

2. Remove default padding value = 0 (not correct anyway)
    from the layout mapping constructors
    that take both extents and padding value.
is_nothrow_constructible_v requires C++17.
Use is_nothrow_constructible::value instead.
Document what public constructors are missing from
layout_{left,right}_padded.  Bring exposition-only names
in line with their counterparts in the proposal.
@mhoemmen
Copy link
Contributor Author

I've handed off this work to a collaborator for now.

@mhoemmen
Copy link
Contributor Author

This PR is superseded by PR #237. Thanks all!

@mhoemmen mhoemmen closed this Mar 21, 2023
mhoemmen added a commit to mhoemmen/mdspan that referenced this pull request Jul 26, 2023
…s_impl

[ready] kokkos impl of `matrix_{frob,one,inf}_norm` and `vector_sum_of_squares`
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.

3 participants