[FEA] Add support for mdspan - #1227
Conversation
Adds the new tensor[i, j] syntax using MatX's existing indexing code. The new syntax is only included when supported by the compiler with C++23. It does not break the C++20 tensor(i, j) syntax.
Allow make_tensor to create a MatX tensor from a cuda::std::mdspan without copying or owning its data. Keep the same dimensions and strides and add tests for different layouts, sizes, and shared memory.
Explain how to create a MatX tensor from an mdspan and show the supported memory layouts. Show some examples of the new tensor[i, j] syntax available with C++23
Greptile SummaryThis PR adds zero-copy construction of MatX tensors from supported
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
M["cuda::std::mdspan"] --> V["Validate layout, accessor, extents, and strides"]
V --> D["Create MatX shape/stride descriptor"]
D --> T["Non-owning MatX tensor"]
M -. "shared data_handle()" .-> T
Reviews (2): Last reviewed commit: "Only allow supported mdspan layouts" | Re-trigger Greptile |
Limit make_tensor to layout_right, layout_left, and layout_stride to prevent incorrect results from unsupported custom layouts
|
Hi @cliffburdick, I addressed the issue Greptile pointed out. Please let me know if I need to change anything else. Thank you for your time! |
| .. doxygenfunction:: make_tensor( TensorType &tensor, const index_t (&shape)[TensorType::Rank()], Allocator&& alloc) | ||
| .. doxygenfunction:: make_tensor( TensorType &tensor, ShapeType &&shape, Allocator&& alloc) | ||
|
|
||
| cuda::std::mdspan Support |
There was a problem hiding this comment.
I think we should support both cuda::std::mdspan for those on older c++ versions, then an include guard for the same ones for std::mdspan. Maybe there's a type trait you can use to simplify the code.
| std::is_same_v<LayoutPolicy, cuda::std::layout_right> || | ||
| std::is_same_v<LayoutPolicy, cuda::std::layout_left> || | ||
| std::is_same_v<LayoutPolicy, cuda::std::layout_stride>) | ||
| auto make_tensor( |
There was a problem hiding this comment.
I think if we want to support std::mdspan also we should add a type trait that's true for cuda::std::mdspan and std::mdspan. Have that be the only requires clause, then do separate checks inside? It could be a bit ugly but at least it keeps it in one function
There was a problem hiding this comment.
Sounds like a good idea. I am working on it!
What this does
Adds two features:
make_tensorcan now create a MatX tensor from acuda::std::mdspantensor[i, j]with C++23When the compiler supports C++23 multidimensional indexing, tensors can use:
Testing
Added tests for mdspan and operator[]
Built and ran the tensor tests with:
cmake --build build \ --target test_00_tensor_MakeTensorTests \ --parallel 2 ./build/test/test_00_tensor_MakeTensorTests \ --gtest_filter='MakeTensorTests.*Mdspan*' ./build/test/test_00_tensor_MakeTensorTestsTesting was done using:
The mdspan changes and the full
MakeTensorTestssuite passed with C++20The new
operator[]feature requires C++23 multidimensional subscript support,which is not available with CUDA 12.8. I could not update the CUDA environment
due to administrative permission restrictions
I would really appreciate it if the C++23 tests could be run in a compatible
environment during review.
Documentation
Updated the documentation with:
tensor[i, j]syntaxtensor(i, j)in C++20 and C++23Related issue
Closes #582