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

Padded layout #237

Merged
merged 68 commits into from
Oct 20, 2023
Merged

Padded layout #237

merged 68 commits into from
Oct 20, 2023

Conversation

nmm0
Copy link
Contributor

@nmm0 nmm0 commented Feb 2, 2023 •
edited
Loading

This has all of p2642 except 4.3 4.4 submdspan.* which I'm adding in another PR

@nmm0 nmm0 requested review from crtrott and mhoemmen February 2, 2023 03:03
crtrott
crtrott requested changes Feb 2, 2023
View reviewed changes
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
mhoemmen
mhoemmen reviewed Feb 2, 2023
View reviewed changes
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated

template <typename _T>
static constexpr auto
__construct_with_type(const _Extents &__extents, const extents<typename _Extents::index_type, _NewExtent> &__new_extents)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a neat way to do it! I hadn't thought about how to make the left and right versions share implementation details.

include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
crtrott
crtrott requested changes Feb 5, 2023
View reviewed changes
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.

Looks pretty good. There are a few renames which I think would improve clarity.

mhoemmen
mhoemmen reviewed Feb 6, 2023
View reviewed changes
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking care of this!

include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated
static constexpr rank_type __extent_to_pad_rank = 0;

static_assert((padding_stride != 0)
|| (extents_type::static_extent(__extent_to_pad_rank) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider reformatting so that the || goes at the end of the line? The spacing here looks a bit odd.

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 personally prefer the operator at the beginning of the line for readability; however you're right that the spacing is weird. I retabbed it so that the || is aligned with the opening parenthesis

include/experimental/__p2642_bits/layout_padded.hpp Show resolved Hide resolved
crtrott
crtrott requested changes Feb 7, 2023
View reviewed changes
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.

I think the condition on the layout_left/right constructors are wrong?

include/experimental/__p0009_bits/layout_left.hpp Outdated Show resolved Hide resolved
include/experimental/__p0009_bits/layout_right.hpp Outdated Show resolved Hide resolved
dalg24
dalg24 reviewed Feb 7, 2023
View reviewed changes
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.

only scratched the surface

include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
tests/test_layout_padded_left.cpp Outdated Show resolved Hide resolved
tests/test_layout_padded_left.cpp Outdated Show resolved Hide resolved
@nmm0 nmm0 marked this pull request as ready for review March 14, 2023 21:36
@nmm0 nmm0 changed the title WIP: Padded layout Padded layout Mar 14, 2023
mhoemmen
mhoemmen approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Lots of great tests -- great work!!!

include/experimental/__p0009_bits/layout_left.hpp Outdated Show resolved Hide resolved
include/experimental/__p0009_bits/layout_left.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
@mhoemmen mhoemmen mentioned this pull request Mar 21, 2023
Closed
mhoemmen pushed a commit to mhoemmen/mdspan that referenced this pull request Jul 26, 2023
@amklinv-nnl
Remove caching of mdspan
64f5fa1 
Fixes kokkos#237
crtrott
crtrott requested changes Aug 2, 2023
View reviewed changes
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.

Bunch of comments

include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
crtrott
crtrott requested changes Aug 29, 2023
View reviewed changes
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.

First partial pass

include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded_fwd.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
include/experimental/__p2642_bits/layout_padded.hpp Outdated Show resolved Hide resolved
nmm0 and others added 27 commits October 20, 2023 10:01
@nmm0 @crtrott
p2642: rename mandate checker to be consistent with precondition checker
8af63cb
@nmm0 @crtrott
p2642: minor formatting changes
a6308a7
@nmm0 @crtrott
p2642: clarification in __get_actual_static_padding_stride static_assert
d02be07
@nmm0 @crtrott
p2642: refactor __unpadded_extent_type_impl to be a constexpr function
f008fb6
@nmm0 @crtrott
p2642: clarify comment on compiler workaround when calling stride()
50e167c
@nmm0 @crtrott
p2642: fix minor formatting
109e0c9
@nmm0 @crtrott
p2642: clarify difference from p2642r2
fe9d394
@nmm0 @crtrott
padded_layout: update padded layout and tests to use new namespace
73ca386
@nmm0 @crtrott
padded_layout: fix missing namespace qualifier
3cedeb9
@nmm0 @crtrott
add p264r2->r3 changes to return type of extents() function
089d12e
@nmm0 @crtrott
add padding_stride member from P2642R2 and add stricter requirements …
b85e676 
…for __is_layout_*_padded_mapping
@nmm0 @crtrott
reword some functions to be clearer and remove dead code
d4245dd
@nmm0 @crtrott
remove inner_extents from layout_padded_right
fcbb54d
@nmm0 @crtrott
remove inner_extents
de2647e
@nmm0 @crtrott
fix missing tests for operator() for padded layouts and remove accide…
c25dce5 
…ntal use of inner_mapping from operator()
@nmm0 @crtrott
add verious minor feedback changes
da845d1
@nmm0 @crtrott
make theinternal padding stride 0 for ranks < 2
2b5239a
@nmm0 @crtrott
cleanup of stride functions
195c8bc
@nmm0 @crtrott
last suggestiosn from review in layout_padded_fwd.hpp
660e96f
@nmm0 @crtrott
more efficient operator() implementation
f61f185
@nmm0 @crtrott
fix references to padding_stride
4b8aa51
@crtrott @dalg24
Apply suggestions from code review
e7d2f5c 
Co-authored-by: Damien L-G <[email protected]>
@crtrott
Address two issues from review: index_type cast, impl prefix
fd09554
@crtrott
Fix static-padded-stride name and stride calculation
44f00d8
@crtrott
Apply more suggestions
34e2735
@crtrott
Cast to rank_type instead of size_t
9896c58
@crtrott
Another cast
63965b5
@crtrott crtrott merged commit ed5f6f6 into kokkos:stable Oct 20, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crtrott crtrott crtrott approved these changes

@nliber nliber nliber left review comments

@mhoemmen mhoemmen mhoemmen approved these changes

@dalg24 dalg24 dalg24 left review comments

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

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