This is an archive of the discontinued LLVM Phabricator instance.

[libc++] mdspan - implement mdspan class
ClosedPublic

Authored by crtrott on Jul 3 2023, 10:07 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

This adds the mdspan class and its tests.

Diff Detail

Event Timeline

crtrott created this revision.Jul 3 2023, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2023, 10:07 AM
Herald added a subscriber: arphaman. · View Herald Transcript
crtrott requested review of this revision.Jul 3 2023, 10:07 AM
crtrott added reviewers: ldionne, Restricted Project.Jul 3 2023, 10:08 AM
crtrott set the repository for this revision to rG LLVM Github Monorepo.
crtrott added a project: Restricted Project.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJul 3 2023, 10:08 AM
crtrott updated this revision to Diff 536836.Jul 3 2023, 10:27 AM
crtrott retitled this revision from mdspan: add mdspan class and tests to [libc++] mdspan - implement mdspan class.

Rebase on top of main

This implements about 80% of the necessary tests, but its already a lot of code so we can collect review feedback while we do the rest.

philnik added inline comments.
libcxx/include/__mdspan/mdspan.h
62

_Uglification!

116
131–132

libc++ doesn't work with MSVC anyways, so this workaround can be removed.

257

These should be _LIBCPP_NO_UNIQUE_ADDRESS.

275
276
279
282
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestAccessors.h
2

Missing license header and include guard

crtrott updated this revision to Diff 536918.Jul 3 2023, 4:55 PM

Address some review comments, add conversion test.

crtrott marked 8 inline comments as done.Jul 3 2023, 4:57 PM
crtrott updated this revision to Diff 537380.Jul 5 2023, 9:04 AM

Fix module file and GCC build.

crtrott updated this revision to Diff 540073.Jul 13 2023, 9:26 AM

Add assertion tests

ldionne added inline comments.Jul 13 2023, 10:25 AM
libcxx/include/__mdspan/mdspan.h
59–71

This can yield false positives if a type is both a mapping and a layout. It can actually lead to hard compiler errors if instantiating the nested types failed in a non SFINAE friendly way. This is a bit pedantic, but I'd be a lot more comfortable if we only restricted this to catching types that we know are *definitely* mappings, i.e. the mappings we provide. We could do that by defining the __libcxx_is_mapping nested type inside our mappings, and then checking:

template <class _Tp>
concept __is_definitely_mapping = requires { typename _Tp::__libcxx_is_mapping; };

Then we assert !__is_definitely_mapping<_LayoutPolicy> instead of !__is_possibly_mapping<_LayoutPolicy>. If this isn't harsh enough of a diagnostic cause it misses user-defined policies, I'd like to see this as an incremental change later, but not as part of the initial design of this class.

BTW we have a precedent for this with __is_join_view_iterator.

Edit:

Instead, I think we could do this too:

template <class _Layout, class _Extents>
concept __has_invalid_mapping = requires !requires { typename _Layout::template mapping<_Extents>; };

Then from within mdspan, you could do:

static_assert(!__has_invalid_mapping<_LayoutPolicy>,
    "mdspan: did you pass a valid LayoutPolicy to mdspan? A common mistake is passing a mapping instead of a layout policy");

WDYT? This keeps the guard rails for user-defined layouts and mappings as well. And there are no possibilities for false positives AFAICT since _Layout::template mapping<_Extents> *has* to be valid.

crtrott updated this revision to Diff 540572.Jul 14 2023, 3:14 PM

Rebase on main, and adding swap test.

crtrott updated this revision to Diff 540820.Jul 16 2023, 11:20 AM

Add a few more tests checking for explicitness of constructors, and size of mdspan.

crtrott updated this revision to Diff 541033.Jul 17 2023, 7:47 AM

Fir formating.

crtrott updated this revision to Diff 541138.Jul 17 2023, 10:53 AM

Split too long static_assert test, added some more non-constructibility checks

ldionne requested changes to this revision.Jul 18 2023, 12:18 PM
ldionne added inline comments.
libcxx/include/__mdspan/mdspan.h
66

Do you test that the default layout policy is layout_right and the default accessor policy is default_accessor<_ElementType>? You can do that with

static_assert(std::is_same_v<mdspan<T, E>, mdspan<T, E, layout_right, default_accessor<T>>>);

Should probably be in types.pass.cpp if not already there.

131–132

Not sure what this comment refers to, but the nvhpc comment should be removed.

137

This raises an interesting question. The spec has preconditions (https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-2), but we can't check them if we make this = default. The = default is not required by the standard but it's arguably a nice property to have. This allows making mdspan trivially default constructible if the data_handle_type, mapping_type and accessor_type are trivially default constructible, right?

Actually, considering this a bit more, per https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-3 the effects are supposed to be: Value-initializes ptr_, map_, and acc_.. This is not the case if we make this = default and we have a data_handle_type, mapping_type or accessor_type that's an aggregate (not all of those might be possible but you get the point). So I think this is non-conforming. For example, if you have an aggregate accessor_type with a dummy int member, that int will never be value-initialized (zeroed-out) with your current implementation, but it should be according to the Standard. This needs a test.

138–139

Do you have tests that those are trivial operations if the members are trivially copy/move constructible?

158

There's a few instances of that.

159

These requirements are wrong AFAICT. They should be using const& like the ones for array as described in https://eel.is/c++draft/views.multidim#mdspan.mdspan.cons-8.1.

I think what you want is is_convertible_v<const _OtherIndexType&, index_type> && is_nothrow_constructible_v<index_type, const _OtherIndexType&>.

Those need tests. If you have an index type where index_type(_OtherIndexType&&) works but index_type(_OtherIndexType const&) doesn't, then the requires before my comment will be satisfied but you will get a hard compiler error while trying to compile __map_(extents_type(__exts)) below (I think). Please also add the same test for the std::array constructor above: if I go and remove const& from the equivalent requires above, a test should fail.

164

If you forget to std::move here (or in any of the similar constructors), which test fails? If there's none, please make sure to add coverage.

167

This needs a test, which should be possible if you have an extents_type where mapping_type(extents_type&&) works but mapping_type(extents_type const&) doesn't. Then the requires before my comment would enable this mdspan constructor, but you'd get a hard compiler error while trying to compile __map_(__exts) below.

178–179

Needs tests.

185–186
191
193–194
205–206

Same question about testing for triviality.

209

These "stable" names seem to originate from a previous version of the standard. https://eel.is/c++draft/views.multidim#mdspan doesn't seem to have the same names.

214

Just for consistency with the spec.

216

We have an open patch to categorize those.

222

Needs a test. The span version has the same problem.

224

Is there any reason why we're doing this here instead of following the standard wording more closely? Is it because you want to avoid calling the variadic version in case you have a large number of dimensions?

I would inline all the uses of __idx_fold into their calling functions and use a lambda instead, that's easier to read. Otherwise the implementation of the function is basically 100+ lines above the function itself.

230

Note to self: I have not actually reviewed the implementation of these idx_fold operations yet.

libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_array.pass.cpp
49–52

I don't think that works. The usual pattern we use here is

template <class T>
void accept(T);

template <class MDS, class Exts>
concept check_mdspan_ctor_implicit = requires(typename MDS::data_handle_type h, const Exts& exts) {
  accept<MDS>({h, exts});
};

You'll want to make all of your explicit tests consistent.

libcxx/test/std/containers/views/mdspan/mdspan/properties.pass.cpp
99

Those static functions should be assert(...)ed instead. Then we call those from a constexpr and a non-constexpr context.

Right now the following implementation passes your tests:

static constexpr rank_type rank() noexcept {
  if (std::is_constant_evaluated()) return extents_type::rank();
  else return 0;
}

I think all the "properties" are affected.

This revision now requires changes to proceed.Jul 18 2023, 12:18 PM
crtrott marked 23 inline comments as done.Jul 19 2023, 7:37 PM

Addressed all the comments, uploading patch now.

libcxx/include/__mdspan/mdspan.h
66

Added.

137

Doesn't the {} make it so its value initialized?

_LIBCPP_NO_UNIQUE_ADDRESS data_handle_type __ptr_{};
_LIBCPP_NO_UNIQUE_ADDRESS mapping_type __map_{};
_LIBCPP_NO_UNIQUE_ADDRESS accessor_type __acc_{};

I believe this makes it so that the three members are value initialized for the defaulted default constructor doesn't it?

Which means the only thing we are not doing is the uncheckable precondition.

138–139

Adding it.

159

index_type is integral so I don't think we can write a test for that using the move thing? I.e. the only thing you a move into int is an int. However, we could test that we have an OtherIndexType where the conversion operator to integral is non-const and/or missing no-except. Turns out that this was already wrong in extents itself, which I need to fix in order to test mdspan, because otherwise it can go through the constructor taking extents ...

164

Added coverage via move_counted_handle i.e. a handle type which increments a static variable during move.

167

Fixed and added test by modifying the layout_wrapped_integral depending on value of the _Wrap it now has a constructor from extents_type.&& Note that construction from extents and construction from array/span/integers has different requirements for the mapping. We are now testing both.

178–179

Added a whole bunch of testing, and made sure (i.e. tried) that any combination of missing const& both for the requires and the explicit will result in failure.

224

Yeah, we can't avoid having an expansion happening in this operator via idx_fold (or now changed via a lambda) but we don't have to instantiate another variadic function here. We have ongoing issues with inlining depth in our big applications, and so we make judgement calls regarding delegation.

This is the lambda version body of the function (a bit more than the fold expression but ok:

return __acc_.access(__ptr_, [&]<size_t ... _Idxs>(index_sequence<_Idxs...>) {
  return __map_(__indices[_Idxs]...);
}(make_index_sequence<rank()>()));

This is the forwarding version:

return [&, self = this]<size_t ... _Idxs>(index_sequence<_Idxs...>) -> reference {
   return (*self)[__indices[_Idxs]...];
}(make_index_sequence<rank()>());

I personally don't see that as better, we need to explicitly capture this, and need to define the return type (otherwise it returns a value not a reference.

230

Removed it.

libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_array.pass.cpp
49–52

I don't think that works, because it's multi argument explicitlness. I.e. this doesn't even compile:

class Bar {
  constexpr Bar(int, int) {}
};

template<class T>
constexpr bool accept(T) { return true; }

static_assert(accept({1,1}));

My understanding is that for multi argument constructors the ONLY difference in whether you have an explicit or not is whether

T a = {....};

works.

It doesn't make any difference for function calls.

libcxx/test/std/containers/views/mdspan/mdspan/properties.pass.cpp
99

Yeah done.

crtrott updated this revision to Diff 542278.Jul 19 2023, 7:39 PM
crtrott marked 11 inline comments as done.

This addresses Louis review comments. Most notably fixing a lot of constraints regarding const& vs non-const arguments and adding testing for that.

crtrott updated this revision to Diff 542523.Jul 20 2023, 7:51 AM

Fix another warning from GCC

crtrott updated this revision to Diff 542647.Jul 20 2023, 1:13 PM

Another CI fix and add missing static assert verification.

ldionne added inline comments.Jul 20 2023, 1:37 PM
libcxx/include/CMakeLists.txt
479

Let's add a release note and let's update the paper list.

libcxx/include/__mdspan/mdspan.h
137

You're right, sorry I missed that.

libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h
39
50

Also no uglification in test code, since that makes the test code (pedantically) ill-formed for using reserved names.

52

Would it make sense to use layout_right::mapping under the hood to implement this? It would reduce the complexity of this test class quite a bit, no?

56–57

This shouldn't be using any libc++ internal utilities since this is test code.

66

We don't use these ABI macros in test code.

libcxx/test/std/containers/views/mdspan/mdspan/assert.conversion.pass.cpp
11

For the assertion tests, we now use // UNSUPPORTED: !libcpp-has-hardened-mode && !libcpp-has-debug-mode, and you can drop ADDITIONAL_COMPILE_FLAGS.

libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp
110

This is a nitpick, but we usually just use test();, not (void)test();. Consider doing for consistency with the numerous other tests where we have that pattern.

libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp
195

This is crazy. If there was a test thoroughness award, you'd get it. Thanks!

235

Is there a reason why you're not checking the conv_test_accessor_c versions at compile-time?

libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp
52

Whenever I see this kind of logic in tests, I feel like it would be too easy to make a mistake in the logic of the test and render the test useless. Instead, what would you think of passing whether you expect H, M and A to be constructible as bool template arguments to this function? Then this would be hardcoded at the point of call instead of figured out "automatically" by the test. This applies in a bunch of places.

libcxx/test/std/containers/views/mdspan/mdspan/index_operator.pass.cpp
43

Let's use TEST_CLANG_VER, it'll be easier to find in the future and remove the workaround. Also below (and maybe elsewhere, please check).

libcxx/test/std/containers/views/mdspan/mdspan/types.pass.cpp
65

Nice!

crtrott updated this revision to Diff 542724.Jul 20 2023, 5:05 PM
crtrott marked 11 inline comments as done.

Added release notes, rebased. Addressed Louis comments.

crtrott updated this revision to Diff 542745.Jul 20 2023, 7:27 PM

Fix formatting, add more triviality testing

crtrott added inline comments.Jul 20 2023, 7:53 PM
libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h
52

I looked at it, because we wanted a layout which is runtime conditionally non-unique and not-strides, almost all the functions are different. So nothing to be saved really.

libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp
52

OK done.

52

I did it for the ones where it seemed straight forward, and did not do it for conversion, where its kinda combinatorial what happens. That thing has more local checks to assure that we test what we believe we test.

crtrott updated this revision to Diff 542750.Jul 20 2023, 8:06 PM

Fix warning on GCC

crtrott updated this revision to Diff 542753.Jul 20 2023, 8:31 PM

Fix apple clang build ... TEST_CLANG_VER is not defined on apple clang ...

crtrott added inline comments.Jul 20 2023, 8:32 PM
libcxx/test/std/containers/views/mdspan/mdspan/index_operator.pass.cpp
43

TEST_CLANG_VER is not defined on apple clang so we would need to check for TEST_CLANG_VER and TEST_APPLECLANG_VER, clang-major works universally here.

crtrott updated this revision to Diff 542928.Jul 21 2023, 8:17 AM

Fix 32bit builds and fix size assertion.

crtrott updated this revision to Diff 542988.Jul 21 2023, 10:49 AM

Trigger all mdspan assertions in hardened mode: all of them are related in one form or another to bounds checking. Either directly, or in order to not screw up the bounds during construction/assignment/conversion etc. which would violate the invariants of mdspan, and thus could lead to bounds checks not triggering when they should.

ldionne accepted this revision.Jul 21 2023, 2:30 PM

LGTM with passing CI. Thank you for all the effort you've put into this feature!

libcxx/test/std/containers/views/mdspan/extents/ctor_from_array.pass.cpp
86

There's a few instances of this typo.

libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h
32

Please explain how the actual layout differs from layout_right and the other ones.

190

exposition-only is relevant in the standard, not in our tests. I would remove this.

322

Same here for exposition-only.

libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp
13

This synopsis is wrong, please go through them to make sure.

35–40

I think you can get rid of this by using a = default copy assignment operator. In all cases I don't understand why we're basically not testing anything in that case.

This revision is now accepted and ready to land.Jul 21 2023, 2:30 PM
ldionne added inline comments.
libcxx/include/__mdspan/extents.h
178

I would keep this _LIBCPP_ASSERT_UNCATEGORIZED and then @var-const can categorize them appropriately.

var-const added inline comments.Jul 21 2023, 6:01 PM
libcxx/include/__mdspan/extents.h
210

Can you please double-check if this assertion is tested?

libcxx/test/std/containers/views/mdspan/layout_right/assert.conversion.pass.cpp
1 ↗(On Diff #542988)

Nit: I think tests that check assertions usually are under libcxx/test/libcxx, not libcxx/test/std since they test libc++-specific behavior. This can be done in a follow-up.

crtrott marked 7 inline comments as done.Jul 22 2023, 10:21 AM
crtrott added inline comments.
libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp
13

I think this is right?

35–40

part of the point of checked accessor is to NOT be trivially copy assignable, so that's why it has a user provided one.

crtrott updated this revision to Diff 543204.Jul 22 2023, 10:21 AM
crtrott marked 2 inline comments as done.

Address last comments: make all assertions UNCATEGORIZED

crtrott added inline comments.Jul 22 2023, 10:24 AM
libcxx/test/std/containers/views/mdspan/mdspan/assign.pass.cpp
13

sorry accidentally published that above comment: it was wrong (copy ctor instead of copy assign) and I fixed it

crtrott marked an inline comment as done.Jul 22 2023, 12:03 PM
crtrott added inline comments.
libcxx/include/__mdspan/extents.h
210

Checked in libcxx/test/std/containers/views/mdspan/extents/assert.obs.pass.cpp

crtrott updated this revision to Diff 543306.Jul 23 2023, 10:44 AM
crtrott marked an inline comment as done.

Attempt to fix mingw failure.

crtrott updated this revision to Diff 543322.Jul 23 2023, 1:09 PM

I investigated the failure on MinGW and tracked it down to a reproducer on godbolt, which indicates this is a bug in the compiler when using --target=x86_64-w64-windows-gnu
https://godbolt.org/z/Y6Mb7sMMc

Shortish version, you have something like this:

template<class T, class M, class A>
class mdspan {
    public:
  mdspan() = default;
  mdspan(const mdspan&) = default;
  mdspan(T* ptr_, M map_, A acc_):ptr(ptr_),map(map_),acc(acc_) {}
  template<class OT, class OM, class OA>
  mdspan(const mdspan<OT, OM, OA>& other):ptr(other.ptr), map(other.map), acc(other.acc) {}

  [[no_unique_address]] T* ptr;
  [[no_unique_address]] M map;
  [[no_unique_address]] A acc;
};

If you now do mdspan<const T, M2, A2>(mdspan<T, M1, A1>); where M1, A1 and A2 are empty, but M2 is not, and A1->A2 happens via a conversion operator in A1, not a converting constructor in A2 the last byte of ptr gets zeroed out.

The only standard conformant way to fix this is to not have [[no_unique_address]].
However this is a rather esoteric corner case, where you would need to have two accessors where A2 is constructible from A1, but doesn't have a constructor accepting const A1&.

So couple options:
a) I just don't test that combo on MinGW, and users could run into this behavior which we will report as a bug, if they do something weird.
b) we remove [[no_unique_address]] for now on mdspan for a MinGW build, and break ABI later if and when this bug gets fixed
c) I don't land mdspan today, and we hope the bug is fixed in MinGW with clang 17.

I personally would opt for a) since the chances users will run into this are virtually zero, and one can work around this bug.

crtrott updated this revision to Diff 543570.Jul 24 2023, 8:37 AM

Work around MinGW issue by disabling test, not removing no-unique-address.

Mordante added inline comments.
libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp
241

Please add a link to the bug you filed.

crtrott updated this revision to Diff 543775.Jul 24 2023, 6:25 PM

Uploaded the wrong patch file last time around, where I guarded the wrong two tests on MinGW :-(

crtrott marked an inline comment as done.Jul 24 2023, 8:51 PM