This is an archive of the discontinued LLVM Phabricator instance.

[libc++] mdspan - implement layout_stride
ClosedPublic

Authored by crtrott on Aug 4 2023, 8:49 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG639a0986f3a3: [libc++] mdspan - implement layout_stride (#69650)
Summary

This implements layout_stride for C++23 and with that completes the implementation of the C++23 mdspan header.
The feature test macro is added, and the status pages updated.

Co-authored-by: Damien L-G <dalg24@gmail.com>

Diff Detail

Event Timeline

crtrott created this revision.Aug 4 2023, 8:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 8:49 PM
crtrott requested review of this revision.Aug 4 2023, 8:49 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 4 2023, 8:49 PM
crtrott updated this revision to Diff 547593.Aug 6 2023, 10:46 AM

Fix up comments for some formatting check.

crtrott updated this revision to Diff 547605.Aug 6 2023, 1:55 PM

More formatting fixes in the tests.

crtrott updated this revision to Diff 547611.Aug 6 2023, 4:01 PM

Fix ADL use warning.

crtrott updated this revision to Diff 547620.Aug 6 2023, 6:07 PM

More fixes for things not triggered in my various MacOS builds.

crtrott updated this revision to Diff 547643.Aug 6 2023, 9:46 PM

Fixup module map

Mordante added inline comments.
libcxx/include/__mdspan/layout_stride.h
19

The file modules/std/mdspan.cppm should be updated; uncomment using std::layout_stride;

crtrott updated this revision to Diff 548285.Aug 8 2023, 11:00 AM

Update mdspan std::module file

crtrott updated this revision to Diff 548341.Aug 8 2023, 1:44 PM

Fix older clang missing a typename in a test

crtrott marked an inline comment as done.EditedAug 9 2023, 10:43 AM

This passed testing except the expected AIX failure since the runners are down.

philnik added inline comments.
libcxx/docs/Status/Cxx23Papers.csv
91–94

These should all be completed with this patch too.

libcxx/include/__mdspan/layout_stride.h
149

Throughout. @var-const Do you have any thoughts here?

164
228

Maybe?

268
291

Test!

312

Where does this come from?

314
libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.conversion.pass.cpp
80

Maybe try to trigger the edge case here? Just ignore the comment if it doesn't work.

84

Possible future work: Add tests to make sure that mdspan and friends work with _BitInt.

libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_array.pass.cpp
53
libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.ctor.extents_span.pass.cpp
54
83

This should be a span!

libcxx/test/std/containers/views/mdspan/CustomTestLayouts.h
278
libcxx/test/std/containers/views/mdspan/layout_stride/comparison.pass.cpp
50–56
template <class E1, class E2>
concept layout_mapping_comparble = requires(E1 e1, E2 e2) { std::layout_stride::mapping<E1>(e1, s1) == std::layout_stride::mapping<E2>(e2, s2); }

should also work, right?

libcxx/test/std/containers/views/mdspan/layout_stride/ctor.default.pass.cpp
38

Please add a test to make sure extents() is const qualified. Also for required_span_size() and strides().

libcxx/test/std/containers/views/mdspan/layout_stride/ctor.extents_array.pass.cpp
2

Note to self: Continue here.

crtrott updated this revision to Diff 550843.Aug 16 2023, 12:43 PM

Address review feedback, and implement missing ctors from layout_stride in layout_left/right

crtrott updated this revision to Diff 550875.Aug 16 2023, 1:51 PM

Fix a formatting thing in tests.

crtrott updated this revision to Diff 551160.Aug 17 2023, 8:55 AM

Fix more warning stuff

crtrott marked 15 inline comments as done.Aug 17 2023, 8:59 AM
crtrott added inline comments.
libcxx/include/__mdspan/layout_stride.h
228

Results in warning: include/c++/v1/__mdspan/layout_stride.h:322:49: error: left operand of comma operator has no effect [-Werror,-Wunused-value]

libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.conversion.pass.cpp
84

Will do this in a future revision for all the relevant pieces starting with extents.

crtrott updated this revision to Diff 551168.Aug 17 2023, 9:06 AM
crtrott marked 2 inline comments as done.

Add missing test on const mapping for required_span_size

crtrott marked an inline comment as done.Aug 17 2023, 9:52 AM
crtrott added inline comments.
libcxx/test/std/containers/views/mdspan/layout_stride/ctor.default.pass.cpp
38

Done in properties.pass.cpp

crtrott updated this revision to Diff 551188.Aug 17 2023, 10:40 AM
crtrott marked an inline comment as done.

Simply disable deduction guide warnings, adding them explicitly ended up either with unused template warning in files where they are not needed, or with a warning from GCC if one adds [[maybe_unused]], will try separately to make this warning go away.

ldionne added inline comments.Aug 28 2023, 12:37 PM
libcxx/include/__mdspan/layout_stride.h
164

I would suggest pulling this into a __bubble_sort helper function and moving the comment there. That way, if we want to change the approach later on (e.g. use std::sort for real), it'll be easier. The code will also get a lot simpler here.

197
231

I think it would make sense to create an __offset function of some kind for https://eel.is/c++draft/mdspan.layout#stride.expo-2. It should handle the case where one of the extents is 0. Also probably needs a test.

Good catch!

285
314

This should be a LWG issue? It should be pretty simple.

326

This is __offset again. Is it wrong here too? If wrong, this is missing a test.

libcxx/test/libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_stride.pass.cpp
14

Can you link to the LLVM issue you created about this? We should have a way to do this with _LIBCPP_CTAD_SUPPORTED_FOR_TYPE.

libcxx/test/std/containers/views/mdspan/layout_right/ctor.layout_stride.pass.cpp
34–37

That way you can't mess up the test by calling it with different arguments.

crtrott marked an inline comment as done.Aug 31 2023, 4:04 PM
crtrott added inline comments.
libcxx/test/libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_stride.pass.cpp
14
crtrott updated this revision to Diff 555190.Aug 31 2023, 4:05 PM
crtrott marked an inline comment as done.

Address more review comments: fix some assertions when extents are 0

crtrott marked 5 inline comments as done.Aug 31 2023, 4:09 PM
crtrott updated this revision to Diff 557718.Oct 16 2023, 3:26 PM

Fix a last few review comments.

crtrott marked an inline comment as done.Oct 16 2023, 3:27 PM

I have a few more comments but this pretty much LGTM assuming CI passes.

libcxx/test/libcxx/containers/views/mdspan/layout_left/assert.ctor.layout_stride.pass.cpp
14

Sorry, I meant add a comment in the test so that we can remove the -Wno-ctad-... at some point.

libcxx/test/libcxx/containers/views/mdspan/layout_right/assert.ctor.layout_stride.pass.cpp
63

Is this what you mean or is the test wrong?

libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.index_operator.pass.cpp
12

Shouldn't these assertions be enabled in the hardened mode too?

libcxx/test/std/containers/views/mdspan/layout_left/ctor.layout_stride.pass.cpp
56

I don't understand why you are performing an assignment here. Aren't we trying to test construction?

Also, the following should be sufficient to check for implicit constructibility:

To dest = src;

this should allow getting rid of test_implicit_conversion. Yes, I left a comment last time to change to the lambda but I guess I had not realized you could simplify this even more altogether.

crtrott updated this revision to Diff 557778.Oct 19 2023, 8:10 AM
crtrott marked an inline comment as done.

Address latest feedback

crtrott marked 3 inline comments as done.Oct 19 2023, 8:10 AM
crtrott added inline comments.
libcxx/test/libcxx/containers/views/mdspan/layout_stride/assert.index_operator.pass.cpp
12

I think we agreed to do it only in debug mode, because for the vast majority of use cases the assertion will be caught inside mdspan already, since most people would use layouts only through mdspan (and later mdarray).

This revision was not accepted when it landed; it landed in state Needs Review.Oct 20 2023, 7:14 AM
This revision was automatically updated to reflect the committed changes.