add aligned accessor as part of the library API #446

nmm0 · 2026-01-28T21:23:10Z

Rebase/rework of #422

Closes #412

See https://github.com/rapidsai/raft/blob/branch-25.08/cpp/include/raft/thirdparty/mdspan/include/experimental/__p0009_bits/aligned_accessor.hpp. This implementation was adapted from kokkos/mdspan/blob/stable/examples/aligned_accessor/aligned_accessor.cpp.

Apply suggestions Co-authored-by: Nicolas Morales <nmmoral@sandia.gov>

…cessor

… in test_aligned_accessor.cpp

crtrott

OK I requested some changes.

crtrott · 2026-02-03T00:08:26Z

include/experimental/__p0009_bits/aligned_accessor.hpp

+		"Alignment must be a power of two no less than "
+		"the minimum required alignment of T.");
+  using type = T* MDSPAN_IMPL_ALIGN_VALUE_ATTRIBUTE( Alignment );
+};


I don't think this is necessary is it? If you have a static_assert inside the aligned_accessor you can't get to aligned_accessor::data_handle_type without this being checked or?

The original point of this was that some compiler (I think MSVC?) needed a special attribute marker on pointers to declare overalignment. That's not the Standard C++ way of doing things, but it might be reasonable for pre-C++20 back-ports. If you have C++20, the Standard way to do this is to use assume_aligned.

Thus, I think this is acceptable, as long as it is not exposed to users.

Yeah but we are only using that typedef anyway, so we might as well just declare it exactly this way inside aligned_accessor? I.e. aligned_accessor<T, 4>::data_handle_type is not aligned_pointer<T,4> its aligned_pointer<T,4>::type so we can make that typedef inline in aligned_accessor or?

@crtrott Ah, right, that makes total sense then. At least aligned_pointer_t<T, 4> would be more idiomatic.

crtrott · 2026-02-03T00:17:55Z

examples/aligned_accessor/aligned_accessor.cpp

 namespace {
+using Kokkos::aligned_accessor;
+using Kokkos::detail::aligned_pointer_t;
+using Kokkos::detail::assume_aligned_method;


I am not sure I like that we are adding this. These guys are used only in this example, but we are putting them into the main library (assume_aligned_method and align_attribute_method). I think we should remove them altogether.

+1, we do not need aligned_pointer_t or assume_aligned_method in Standard C++, so we should not expose them to users.

This was mostly to merge Mark's example with the new code. This isn't exposing to the API as they are under detail::

It makes sense that the aligned_accessor example needs to pull in aligned_accessor. However, why does the example need aligned_pointer_t or assume_aligned_method? Wouldn't those just be implementation details of aligned_accessor to help with back-porting it to C++ versions earlier than 20?

tests/test_aligned_accessor.cpp

Copilot

Pull request overview

This PR promotes aligned_accessor (and an is_sufficiently_aligned helper) from an example/raft fork into the core mdspan API, and wires it into tests and the aligned accessor example. It also adds a new, currently unused layout_padded implementation under the __p0009_bits directory.

Changes:

Introduces experimental/__p0009_bits/aligned_accessor.hpp with Kokkos::aligned_accessor and Kokkos::is_sufficiently_aligned, and exposes it via mdspan.hpp.
Refactors examples/aligned_accessor/aligned_accessor.cpp to use the library-provided accessor and alignment utilities instead of its own local implementation.
Adds a new test_aligned_accessor unit test and registers it in CMake, and adds an unused experimental/__p0009_bits/layout_padded.hpp header that duplicates padded layout functionality.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`include/mdspan/mdspan.hpp`	Adds `experimental/__p0009_bits/aligned_accessor.hpp` to the public mdspan umbrella header so `aligned_accessor` and `is_sufficiently_aligned` become part of the main API.
`include/experimental/__p0009_bits/aligned_accessor.hpp`	Defines `Kokkos::aligned_accessor`, `Kokkos::is_sufficiently_aligned`, and supporting alignment utilities/macros for the standard namespace implementation.
`include/experimental/__p0009_bits/layout_padded.hpp`	Introduces an alternate `layout_left_padded` / `layout_right_padded` implementation in the standard namespace that currently isn’t referenced by the rest of the library.
`examples/aligned_accessor/aligned_accessor.cpp`	Switches the example to use `Kokkos::aligned_accessor`, `Kokkos::detail::aligned_pointer_t`, and the shared alignment macros/metadata instead of locally reimplementing them.
`tests/test_aligned_accessor.cpp`	Adds tests for `Kokkos::is_sufficiently_aligned` and for using `Kokkos::aligned_accessor` with `mdspan` and `submdspan`, including conversion back to the default accessor.
`tests/CMakeLists.txt`	Registers the new `test_aligned_accessor` executable with the existing test harness so the new behavior is covered in the test suite.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

include/experimental/__p0009_bits/layout_padded.hpp

tests/test_aligned_accessor.cpp

include/experimental/__p0009_bits/aligned_accessor.hpp

mhoemmen · 2026-02-03T00:23:05Z

examples/aligned_accessor/aligned_accessor.cpp

-template<class ElementType, std::size_t byte_alignment>
-using aligned_pointer_t = typename aligned_pointer<ElementType, byte_alignment>::type;
-
 template<class ElementType, std::size_t byte_alignment>


The Standard C++ way to spell this is std::assume_aligned<byte_alignment>(ptr).

mhoemmen · 2026-02-03T00:26:06Z

include/experimental/__p0009_bits/aligned_accessor.hpp

+		"Alignment must be a power of two no less than "
+		"the minimum required alignment of T.");
+  using type = T* MDSPAN_IMPL_ALIGN_VALUE_ATTRIBUTE( Alignment );
+};


The original point of this was that some compiler (I think MSVC?) needed a special attribute marker on pointers to declare overalignment. That's not the Standard C++ way of doing things, but it might be reasonable for pre-C++20 back-ports. If you have C++20, the Standard way to do this is to use assume_aligned.

include/experimental/__p0009_bits/aligned_accessor.hpp

mhoemmen · 2026-02-03T00:28:45Z

include/experimental/__p0009_bits/aligned_accessor.hpp

+		"Alignment must be a power of two no less than "
+		"the minimum required alignment of T.");
+  using type = T* MDSPAN_IMPL_ALIGN_VALUE_ATTRIBUTE( Alignment );
+};


Thus, I think this is acceptable, as long as it is not exposed to users.

include/experimental/__p0009_bits/layout_padded.hpp

mhoemmen · 2026-02-03T15:49:22Z

examples/aligned_accessor/aligned_accessor.cpp

-
 template<class ElementType, std::size_t byte_alignment>
 aligned_pointer_t<ElementType, byte_alignment>
 bless(ElementType* ptr, std::integral_constant<std::size_t, byte_alignment> /* ba */ )


Should we consider just calling this assume_aligned? More importantly, why does the example need this? In general, we should favor the example looking as much like Standard C++ as possible. (Yes, I know I wrote the example! but it still could use revision : - ) .)

Yeah I tried to make as minimum possible changes to the example as possible, but now I'm reconsidering that since, well, the example is supposed to be useful to learn how to use the API. So I might rewrite a lot of it (and possible just make it require C++20)

Thanks for considering this! There's nothing wrong with back-porting, but I think I would want both the example and the aligned_accessor implementation to look as much like Standard C++ as possible. The latter could be achieved just by changing some names.

Alright, will do then, I should have a new version tomorrow with all the changes

include/experimental/__p0009_bits/aligned_accessor.hpp

mhoemmen · 2026-02-03T15:56:30Z

include/experimental/__p0009_bits/aligned_accessor.hpp

+#endif
+
+constexpr bool
+is_nonzero_power_of_two(const std::size_t x)


Should we consider just calling this has_single_bit? That would make the implementation look as much like Standard C++ as possible.

mhoemmen · 2026-02-03T15:59:27Z

tests/test_aligned_accessor.cpp

+
+TEST(TestAlignedAccessor, IsSufficientlyAligned) {
+  ASSERT_TRUE(
+      Kokkos::is_sufficiently_aligned<1>(reinterpret_cast<char *>(0x12345678)));


This is technically UB, because we're forming a likely invalid pointer. Another way we could test alignment of 1 would be to construct a array of char aligned to (say) 4 or 8 bytes at least, and then take the address of element 1.

vitor1001 and others added 7 commits January 28, 2026 13:49

Apply suggestions from code review

8d3bd1b

Apply suggestions Co-authored-by: Nicolas Morales <nmmoral@sandia.gov>

kokkos#412: add tests for aligned accessor

78a2c40

kokkos#412: fix some compiler warnings

8814113

kokkos#412: update aligned accessor example to use library aligned ac…

ded7894

…cessor

kokkos#412: add missing include for <cstdlib> to obtain aligned_alloc…

49161ee

… in test_aligned_accessor.cpp

kokkos#412: use MSVCs _aligned_Malloc

b73c7f5

crtrott requested changes Feb 3, 2026

View reviewed changes

crtrott requested a review from Copilot February 3, 2026 00:20

Copilot started reviewing on behalf of crtrott February 3, 2026 00:20 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

mhoemmen requested changes Feb 3, 2026

View reviewed changes

kokkos#412: remove accidental rebase artifact

35da413

mhoemmen reviewed Feb 3, 2026

View reviewed changes

include/experimental/__p0009_bits/aligned_accessor.hpp Show resolved Hide resolved

mhoemmen reviewed Feb 3, 2026

View reviewed changes

add aligned accessor as part of the library API #446

Are you sure you want to change the base?

add aligned accessor as part of the library API #446

Conversation

nmm0 commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crtrott 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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhoemmen Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nmm0 commented Jan 28, 2026 •

edited

Loading

mhoemmen Feb 3, 2026 •

edited

Loading