Extract `submdspan_canonicalize_slices` as standalone function (from PR #408) by Copilot · Pull Request #451 · kokkos/mdspan

Copilot · 2026-02-24T17:40:56Z

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>

crtrott · 2026-02-24T18:03:46Z

+template<size_t k, class Slice, class IndexType, size_t... Extents>
+MDSPAN_INLINE_FUNCTION
+constexpr auto submdspan_canonicalize_one_slice(
+  [[maybe_unused]] const extents<IndexType, Extents...>& exts,
+  [[maybe_unused]] Slice s)
+{


Neither exts, nor Extents, nor k is actually used in this function. Can we not simply template it on IndexType and Slice, and only have that single Slice argument passed in?

Done in 68134d1. Simplified to template<class IndexType, class Slice> with a single Slice s argument — the unused k, Extents, and exts are all removed.

crtrott · 2026-02-24T18:08:25Z

+  const extents<IndexType, Extents...>& exts,
+  Slices... slices)
+{
+  (check_submdspan_slice_mandate<Inds>(exts, get_kth_in_pack<Inds>(slices...)), ...);


Doesn't this work?

Suggested change

(check_submdspan_slice_mandate<Inds>(exts, get_kth_in_pack<Inds>(slices...)), ...);

(check_submdspan_slice_mandate<Inds>(exts, slices), ...);

Done in 68134d1. Updated to (check_submdspan_slice_mandate<Inds>(exts, slices), ...);

crtrott

Couple things there I think we could do it differently, which I think would eliminate the need for the pack extraction function.

…_in_pack Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>

nmm0

A bunch of comments, I think it mostly looks good but I wanna do another pass after some discussion/changes

nmm0 · 2026-02-24T21:12:10Z

+{
+  // Mandate checks (static_asserts only, no computation).
+  // Separated from canonicalization for clarity.
+  check_submdspan_slice_mandates(


I assume this was an ai-generated change -- I'm not sure it's clearer, and it may have some impact on compile time as you have to generate additional templates just for checking.

Since you are already processing one slice at a time I prefer the way it used to be with the mandate checking done on the slice

nmm0 · 2026-02-24T21:29:15Z

+    return cw<static_cast<IndexType>(index_cast<IndexType>(S::value))>;
+  }
+  else {
+    return static_cast<IndexType>(index_cast<IndexType>(s));


Another slight difference with the standard, should this be std::move(s)? I'm not 100% what the purpose of that is unless there is an index type that is movable...

nmm0 · 2026-02-24T21:33:00Z

+#endif
+  else {
+    // General pair-like case: structured binding into [first, last)
+    auto [s_k0, s_k1] = s;


Another std::move(s) from the standard

Per https://eel.is/c++draft/mdspan.sub#helpers-12

Suggested change

auto [s_k0, s_k1] = s;

auto [s_k0, s_k1] = std::move(s);

nmm0 · 2026-02-24T21:38:24Z

+constexpr check_static_bounds_result check_static_bounds(
+  const extents<IndexType, Exts...>&)
+{
+#if defined(__cpp_pack_indexing)


TBH I would rather just define this using index sequence while obvious pack indexing is much preferable it's C++26 so in most cases we will have to look at the alternate code.

This is especially true since we are using an index sequence to generate this anyway, so imo lets just pass down the indices

nmm0 · 2026-02-24T21:48:55Z

+  }
+  else if constexpr (std::is_convertible_v<S_k, IndexType>) {
+    if constexpr (is_integral_constant_like_v<S_k>) {
+      if constexpr (de_ice(S_k{}) < 0) {


Is this needed? I thought you could compare integral constants to integers

I think you still need it. there were weird scenarios.

Ok I will take your word for it :P

integral-constant-like does require convertibility to integers, but it's not clear to me that the compiler will always be able to figure out that it needs to convert the value to int.

Signed-off-by: Christian Trott <crtrott@sandia.gov>

nmm0

Last couple of comments I think things are mostly good just one or two minor issues

nmm0 · 2026-06-08T08:55:30Z

+  }
+  else if constexpr (std::is_convertible_v<S_k, IndexType>) {
+    if constexpr (is_integral_constant_like_v<S_k>) {
+      if constexpr (de_ice(S_k{}) < 0) {


Ok I will take your word for it :P

nmm0 · 2026-06-08T09:00:17Z

+#endif
+  else {
+    // General pair-like case: structured binding into [first, last)
+    auto [s_k0, s_k1] = s;


Per https://eel.is/c++draft/mdspan.sub#helpers-12

Suggested change

auto [s_k0, s_k1] = s;

auto [s_k0, s_k1] = std::move(s);

nmm0 · 2026-06-08T09:01:39Z

+{
+  // Mandate checks (static_asserts only, no computation).
+  // Separated from canonicalization for clarity.
+  (void)(check_submdspan_slice_mandate<typename Extents::index_type, Extents::static_extent(Inds)>(slices) && ... && true);


Why make check_submdspan_slice_mandate return bool if its result is not used?

because I need some statically expandable fold expression which doesn't warn about ignored expression.
I.e. , operator warns about "left thing is getting ignored" and thus I decided to go with a && expansion.

@nmm0 It's a good thought! We could just make check_submdspan_slice_mandate return bool without a static_assert, and static_assert on the whole fold expression. The idea here is to make it easier to figure out which extent static_asserted.

Signed-off-by: Christian Trott <crtrott@sandia.gov>

mhoemmen

Thanks Christian! : - ) I posted some suggestions in the comments.

mhoemmen · 2026-06-26T18:07:24Z

+  using extent_t = std::integral_constant<int, 4>;
+  using stride_t = std::integral_constant<int, 2>;
+  const auto slice0 =
+    MDSPAN_IMPL_STANDARD_NAMESPACE::strided_slice<offset_t, extent_t, stride_t>{};


This is called extent_slice now. (I imagine the plan is to change that later, though. I'm just remarking on it to remind myself.)

yeah exactly.

mhoemmen · 2026-06-26T18:16:17Z

+  test_canonicalize_slices(tuple{}, MDSPAN_IMPL_STANDARD_NAMESPACE::extents<size_t>{});
+}
+
+TEST(CanonicalizeSlices, Rank1_full) {


Should we consider adding a test for a slice type that's convertible to full_extent_t, but not the same as full_extent_t?

mhoemmen · 2026-06-26T18:18:37Z

+} // namespace detail
+
+// ============================================================
+// submdspan_canonicalize_slices: public API


Suggested change

// submdspan_canonicalize_slices: public API

// canonical_slices: public API

mhoemmen · 2026-06-26T18:21:51Z

+{
+  // Mandate checks (static_asserts only, no computation).
+  // Separated from canonicalization for clarity.
+  (void)(check_submdspan_slice_mandate<typename Extents::index_type, Extents::static_extent(Inds)>(slices) && ... && true);


@nmm0 It's a good thought! We could just make check_submdspan_slice_mandate return bool without a static_assert, and static_assert on the whole fold expression. The idea here is to make it easier to figure out which extent static_asserted.

mhoemmen · 2026-06-26T18:22:59Z

+    // TODO: Later introduce canonical-range-slice
+    return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{


Suggested change

// TODO: Later introduce canonical-range-slice

return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{

// TODO: Rename to extent_slice

return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{

mhoemmen · 2026-06-26T18:23:26Z

+    // TODO: Later introduce canonical-range-slice
+    return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{


Suggested change

// TODO: Later introduce canonical-range-slice

return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{

// TODO: Rename to extent_slice

return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{

mhoemmen · 2026-06-26T18:34:46Z

+  }
+  else if constexpr (std::is_convertible_v<S_k, IndexType>) {
+    if constexpr (is_integral_constant_like_v<S_k>) {
+      if constexpr (de_ice(S_k{}) < 0) {


integral-constant-like does require convertibility to integers, but it's not clear to me that the compiler will always be able to figure out that it needs to convert the value to int.

mhoemmen · 2026-06-26T18:42:10Z

+MDSPAN_INLINE_FUNCTION
+constexpr auto subtract_ice([[maybe_unused]] X x, [[maybe_unused]] Y y) {
+  if constexpr (
+    is_integral_constant_like_v<remove_cvref_t<X>> &&


Have we fixed the definition of this to match the Standard concept? Before, it would only be true for std::integral_constant and Kokkos::integral_constant specializations.

Initial plan

e2a6083

Copilot AI assigned Copilot and crtrott Feb 24, 2026

Copilot started work on behalf of crtrott February 24, 2026 17:41 View session

Add submdspan_canonicalize_slices function with design improvements

ad567e7

Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Extract submdspan_canonicalize_slices function into standalone PR~~ Extract submdspan_canonicalize_slices as standalone function (from PR #408) Feb 24, 2026

Copilot AI requested a review from crtrott February 24, 2026 17:58

Copilot finished work on behalf of crtrott February 24, 2026 17:58

crtrott reviewed Feb 24, 2026

View reviewed changes

Comment thread include/experimental/__p2630_bits/submdspan_canonicalize_slices.hpp

crtrott reviewed Feb 24, 2026

View reviewed changes

crtrott requested changes Feb 24, 2026

View reviewed changes

Copilot started work on behalf of crtrott February 24, 2026 18:09 View session

Simplify canonicalize_slices: remove unused params, eliminate get_kth…

68134d1

…_in_pack Co-authored-by: crtrott <9490481+crtrott@users.noreply.github.com>

Copilot AI requested a review from crtrott February 24, 2026 18:12

Copilot finished work on behalf of crtrott February 24, 2026 18:12

nmm0 reviewed Feb 24, 2026

View reviewed changes

crtrott added 2 commits June 3, 2026 13:30

Rename submdspan_canonicalize_slices to align with ISO C++

594eabc

Signed-off-by: Christian Trott <crtrott@sandia.gov>

Remvoe complex support in canonical slice

b46f00f

Signed-off-by: Christian Trott <crtrott@sandia.gov>

crtrott marked this pull request as ready for review June 3, 2026 20:16

crtrott force-pushed the copilot/extract-canonicalize-slices-function branch from 7601c60 to 38a26c7 Compare June 3, 2026 20:21

More cleanup

3643cca

Signed-off-by: Christian Trott <crtrott@sandia.gov>

crtrott force-pushed the copilot/extract-canonicalize-slices-function branch from 38a26c7 to 3643cca Compare June 3, 2026 20:23

crtrott approved these changes Jun 3, 2026

View reviewed changes

nmm0 reviewed Jun 8, 2026

View reviewed changes

Add one more move

309f700

Signed-off-by: Christian Trott <crtrott@sandia.gov>

mhoemmen approved these changes Jun 26, 2026

View reviewed changes

	(check_submdspan_slice_mandate<Inds>(exts, get_kth_in_pack<Inds>(slices...)), ...);
	(check_submdspan_slice_mandate<Inds>(exts, slices), ...);

	// submdspan_canonicalize_slices: public API
	// canonical_slices: public API

		// TODO: Later introduce canonical-range-slice
		return strided_slice<decltype(offset), decltype(extent), decltype(stride)>{

Uh oh!

Conversation

Copilot AI commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

crtrott left a comment

Choose a reason for hiding this comment

Uh oh!

nmm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nmm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhoemmen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Feb 24, 2026 •

edited

Loading