This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Adding mdspan layout_left
ClosedPublic

Authored by crtrott on Jun 26 2023, 9:50 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGb4ff893877ff: [libc++][mdspan] Implement layout_left
Summary

This adds the layout_left layout mapping for mdspan, and its associated tests.
The tests are copy paste from layout_right, with more or less a search replace of left for right..

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

Diff Detail

Event Timeline

crtrott created this revision.Jun 26 2023, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 9:50 AM
Herald added a subscriber: arphaman. · View Herald Transcript
crtrott requested review of this revision.Jun 26 2023, 9:50 AM

I think the next step would be rebasing this properly on top of layouts_right so the diff is fixed, but this pretty much LGTM.

libcxx/include/__mdspan/layout_left.h
73
98

Here and probably elsewhere.

126

I think we should take a look at the codegen here for small but especially for large numbers of dimensions to make sure the compiler doesn't create an actual array.

libcxx/test/std/containers/views/mdspan/layout_left/ctor.default.pass.cpp
33–34

Either address or remove.

libcxx/test/std/containers/views/mdspan/layout_right/assert.ctor.layout_left.pass.cpp
42

I would probably add this and call it a day:

// we can't actually test the representability requirement on `required_span_size()` because it triggers the assertion above in
// the extents constructor
libcxx/test/std/containers/views/mdspan/layout_right/ctor.layout_left.pass.cpp
24

I don't think this is needed.

107–108

I would add !std::is_constructible_v<lr_mapping_t<int, 1, 1>, ll_mapping_t<int, 1, 1>>

125

Oops!

ldionne set the repository for this revision to rG LLVM Github Monorepo.Jun 26 2023, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 10:22 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
crtrott updated this revision to Diff 534756.Jun 26 2023, 2:54 PM
crtrott edited the summary of this revision. (Show Details)

Rebased on top of layout_right revision

crtrott updated this revision to Diff 534761.Jun 26 2023, 3:03 PM
crtrott marked an inline comment as done.

Address review comments.

crtrott marked 6 inline comments as done.Jun 26 2023, 3:09 PM
crtrott updated this revision to Diff 535030.Jun 27 2023, 9:47 AM

Fix test which exceeds default consteval step limit, and fix formatting of fwd file.

crtrott updated this revision to Diff 535066.Jun 27 2023, 11:06 AM

Fix some copy paste name issues in required_span_size test
Fix whitespace in comments in tests

crtrott updated this revision to Diff 535218.Jun 27 2023, 8:03 PM
crtrott retitled this revision from Adding mdspan layout_left to [libc++] Adding mdspan layout_left.

Fix missing include for module build

ldionne accepted this revision.Jun 28 2023, 12:20 PM

LGTM, I'll make a local check for the diff between the tests for layout_right and layout_left just in case.

libcxx/test/std/containers/views/mdspan/layout_right/ctor.layout_left.pass.cpp
25

You're not using any concepts, I think this can go.

This revision is now accepted and ready to land.Jun 28 2023, 12:20 PM
crtrott updated this revision to Diff 535545.Jun 28 2023, 3:02 PM

Remove unused include in test

crtrott marked 2 inline comments as done.Jun 28 2023, 3:03 PM
crtrott added inline comments.
libcxx/include/__mdspan/layout_left.h
126

We decided to do this after the fact, lets us play on godbolt etc.

crtrott updated this revision to Diff 535899.Jun 29 2023, 10:49 AM
crtrott marked an inline comment as done.

Apply post-review changes from layout_right to layout_left

This revision was landed with ongoing or failed builds.Jun 29 2023, 1:04 PM
This revision was automatically updated to reflect the committed changes.