Uh oh!
There was an error while loading. Please reload this page.
Support H2D/D2D/D2H SMV copy - #1367
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
8dad27c to
a22f2f6Compare| if hasattr(_tls, "strided_copy_include_dir"): | ||
| return _tls.strided_copy_include_dir |
There was a problem hiding this comment.
| ifhasattr(_tls, "strided_copy_include_dir"): | |
| return_tls.strided_copy_include_dir | |
| if(strided_copy_include_dir :=getattr(_tls, "strided_copy_include_dir", None)) isnotNone: | |
| return strided_copy_include_dir |
This is one attribute access versus two (one inside hasattr, and then the attribute access itself).
Feel free to not use the := walrus operator if you don't like it.
| if hasattr(_tls, "strided_copy_include_dir"): | ||
| return _tls.strided_copy_include_dir | ||
| cdef str current_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| cdef str copy_kernel_dir = os.path.normpath(os.path.join(current_dir, "..", "include", "strided_copy")) |
There was a problem hiding this comment.
| cdef str copy_kernel_dir = os.path.normpath(os.path.join(current_dir, "..", "include", "strided_copy")) | |
| cdef str copy_kernel_dir = os.path.normpath(os.path.join(current_dir, os.pardir, "include", "strided_copy")) |
Probably not strictly necessary, but avoids a hardcoded string.
Uh oh!
There was an error while loading. Please reload this page.
| return True | ||
| cdef inline bint _base_equal_strides(BaseLayout& a, BaseLayout& b) noexcept nogil: |
There was a problem hiding this comment.
Why is one underscore-prefixed (_base_equal_strides) and the other not (base_equal_shapes)?
There was a problem hiding this comment.
It's because the shapes check the ndim equality first, while strides don't. I needed the shape equality for the copy, while strides not really. Maybe for completness it makes sense to add "public" version with ndim check too. :D
Uh oh!
There was an error while loading. Please reload this page.
| return 0 | ||
| cdef inline copy_into( |
There was a problem hiding this comment.
inline-ing here seems a bit pointless and since many of these would be transitively inline'd (because many of the methods/functions are themselves inline), the code will be bigger.
Do we know if inline actually helps anything here? My preference would be to leave inline off unless it can be demonstrated that it actually meaningfully helps performance and doesn't bloat shared objects. It's very low risk to leave it off.
There was a problem hiding this comment.
Yeah, here it's probably just unnecessary. Leaving definitions in layout.pxd file for basic shape/strides utilities actually made a perf difference, but for a function definied in the same pyx file I would guess compiler doesn't care if the inline is there. Removed it.
cpcloud
left a comment
There was a problem hiding this comment.
Mostly just nits, slight preference to avoid so much inlining unless we can demonstrate its effectiveness in a realistic benchmark.
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
leofang
commented
Dec 13, 2025
/ok to test c761437 |
|
…Device.memory_resource in custom allocator tests Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
oleksandr-pavlyk
commented
Dec 15, 2025
/ok to test 5ceea4d |
stiepan
commented
Dec 15, 2025
Looks like some (transient?) connectivity issues |
| { | ||
| template <typename T, typename _coords_t> | ||
| struct array_view |
There was a problem hiding this comment.
I wouldn't overload the name array_view since the std::array_view was the historical name for std::span. Given that this is implemented multi-dimensions I'd call it something like mdspan which would allow us to ultimately remove the implementation after the std::mdspan is usable in our code base.
https://en.cppreference.com/w/cpp/container/mdspan.html
| }; | ||
| template <bool B, class T = void> | ||
| struct enable_if |
There was a problem hiding this comment.
What is the benefit or reimplementing these core meta programming primitives? https://en.cppreference.com/w/cpp/types/enable_if.html
| template <> | ||
| struct min_val<int32_t> | ||
| { | ||
| static constexpr int32_t value = -2147483648; |
There was a problem hiding this comment.
Why is std::numeric_limits not being used instead?
https://en.cppreference.com/w/cpp/types/numeric_limits.html
auto min_val = std::numeric_limits<int32_t>::min();
leofang
commented
Dec 15, 2025
As per #674 (comment) let us close this PR. Thanks @stiepan for all the works, and sorry that we decided to not accept it in cuda.core. |
Removed preview folders for the following PRs: - PR #1367
Description
This PR implements copy_from/copy_to methods for StridedMemoryView.
Time-permittion or as a follow-up:
PinnedMemoryResourceandManagedMemoryResource#1364 is merged)closes
Checklist