[Mirror] View with mdspan Arguments - #1
Conversation
This supports instantiating View as View<ElementType, ExtentsType, LayoutType, AccessorType> Limitation: all four arguments must be provided and they must be compatible with BasicView, e.g. AccessorType must be one of Kokkos's accessortype for Views such as SpaceAwareAccessor, and you can't arbitrarily mix dynamic and static extents. Also: the memory_traits typedef will come out wrong for now. Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Summary of ChangesHello @csiefer2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant update to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces initial support for Kokkos::View with mdspan-style template arguments, which is a significant step towards aligning with the C++ standard and enabling more flexible view configurations. The implementation strategy of using BasicViewFromTraits as a dispatcher to handle both old and new argument styles is well-executed. The changes to accessors to correctly propagate memory traits and the addition of reverse layout mappings are necessary and correctly implemented. The new compile-time tests are a great addition to ensure the equivalence between the two ways of instantiating views. I have one suggestion for improving code clarity and robustness in Kokkos_MDSpan_Accessor.hpp.
| return MemoryTraits<(mt::is_random_access ? RandomAccess : 0) | | ||
| (mt::is_atomic ? Atomic : 0) | | ||
| (mt::is_restrict ? Restrict : 0) | | ||
| (mt::is_aligned ? Kokkos::Aligned : 0)>(); |
There was a problem hiding this comment.
This implementation for deriving memory traits is correct, but it can be simplified and made more robust. Instead of reconstructing the memory traits from individual flags, you can use bitwise operations on the impl_value of the nested memory traits to remove the Unmanaged flag. This is more concise and will automatically handle any new memory trait flags that might be added in the future.
return MemoryTraits<mt::impl_value & ~Unmanaged>();2 participants
Automated mirror of upstream PR kokkos#8852 Initial support for
Viewwithmdspantemplate arguments.This supports instantiating View as
Why we are doing this
std::mdspansize_tleading to smaller size of theViewobject, and potentially higher performance in particular for high rankViewswhen using 32bit indexingView(i.e. > 8) (not in this PR though)Current Limitation
All four arguments must be provided and they must be compatible with BasicView, e.g. AccessorType must be one of Kokkos's accessor types for Views such as
SpaceAwareAccessor.Status of this PR
This is a draft to gather implementation strategy feedback.
The changes to the Develop test just illustrate that this produces the intended behavior.
Where I want to end up is the following
Tis an element typeEsome instance ofextentsLanmdspancompatible layoutSpaceis a execution or memory space orKokkos::DeviceMemTraitsisKokkos::MemoryTraits<...>with some argsAis a potentially user provided accessorMust have:
Kokkos::View<T, E, L, A>Must have:
Kokkos::View<T, E, L, Kokkos::Accessor<T, Space, MemTraits>>Very Likely want:
Kokkos::View<T, E, L>andKokkos::View<T, E, Kokkos::Accessor<T, Space, MemTraits>>Maybe want:
Kokkos::View<T, E, L, Kokkos::AccessorPolicy<Space, MemTraits>>andView<T, E, Kokkos::AccessorPolicy<Space, MemTraits>>The
AccessorPolicyworks like layouts inmdspani.e.AccessorPolicy<Space, MemTraits>::accessor<T>isAccessor<T, Space, MemTraits>.I would start out both in Experimental.
Issues
MemoryTraitsis not preserving:std::is_same_v<View<T, E, L, M>::memory_traits, M>might be false. Only values in memory traits that are implemented by the accessors actually are preserved right now.