Repository navigation
Provide make_tma_descriptor, DLPack -> CUtensorMap - #6237
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
fbusato
changed the title
make_tma_descriptor, DLPack -> CUtensorMapmake_tma_descriptor, DLPack -> CUtensorMap
Co-authored-by: Bernhard Manfred Gruber <[email protected]>
Co-authored-by: Bernhard Manfred Gruber <[email protected]>
|
My original example created DLTensor for square matrix with C contiguous layout, data type When passing Replacing 57 with 58 also trips it, but 56 works: Passing All tests appear to use shape values that are a whole multiple of itemsize. While this may be what is expected by the driver, I feel we need to provide an example/suggestion of how to handle layouts were the stride in GMEM is not a multiple of 16 bytes. |
The |
|
The behavior seems correct. Note the message
This is not possible. 16 bytes is a constraint of the HW |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pciolkosz
left a comment
pciolkosz
left a comment
There was a problem hiding this comment.
+1 for the device_ref changes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 6h 54m: Pass: 100%/261 | Total: 8d 19h | Max: 5h 52m | Hits: 83%/379023See results here. |
|
@miscco this PR is blocked by you. Can you please re-review and unblock? Thx! |
| // auto __current_device = ::cuda::__driver::__deviceGet(__tensor.device.device_id); | ||
| // auto __compute_capability = | ||
| // ::cuda::__driver::__deviceGetAttribute(::CU_DEVICE_ATTRIBUTE_COMPUTE_CAPABILITY_MAJOR, __current_device); | ||
| //_CCCL_VERIFY(__compute_capability >= 9, "Compute capability 9.0 or higher is required"); |
| // auto __compute_capability = | ||
| // ::cuda::__driver::__deviceGetAttribute(::CU_DEVICE_ATTRIBUTE_COMPUTE_CAPABILITY_MAJOR, __current_device); | ||
| //_CCCL_VERIFY(__compute_capability >= 9, "Compute capability 9.0 or higher is required"); | ||
| if (__compute_capability >= 10) |
There was a problem hiding this comment.
previously __compute_capability was 90. now this is 10 are we missing a zero or did something change
There was a problem hiding this comment.
it was automatically merged. Let me open a new PR
| tma_interleave_layout __interleave_layout = tma_interleave_layout::none, | ||
| tma_swizzle __swizzle = tma_swizzle::none, | ||
| tma_l2_fetch_size __l2_fetch_size = tma_l2_fetch_size::none, | ||
| tma_oob_fill __oobfill = tma_oob_fill::none) noexcept |
There was a problem hiding this comment.
Q: I don't know anything about tma, but if I want to specify only l2 fetch size, then I have to type make_tma_descriptor(..., tma_interleave_layout::none, tma_swizzle::none, tma_l2_fetch_size::some_size)?
If yes, my question is whether it wouldn't be better to implement this as a separate struct tma_descriptor_params (or some other name) and in C++20 you could do: make_tma_descriptor(..., {.l2_fetch_size = tma_l2_fetch_size::some_size})
There was a problem hiding this comment.
Actually, with all compilers (except for msvc) it works even in C++17, see https://godbolt.org/z/jh3vT4fc4
There was a problem hiding this comment.
this is a very nice idea. Unfortunate, C++20 is not really adopted in the API context, so the modification would make the API less user-friendly. We can think about it in the future.
There was a problem hiding this comment.
But as I said, nvcc has no problem with the code, nor do any of the other compilers with exception of msvc. So most of the time, you can just use it in C++17 as well. And if not, you just have to fallback to:
make_tma_descriptor(..., {tma_interleave_layout::none, tma_swizzle::none, tma_l2_fetch_size::some_size});which is basically the same thing.
Please, reconsider this
Address #6172
Description
Provide
make_tma_descriptorto convertDLTensorandDLManagedTensortoCUtensorMap.Aspects discussed with the JAX team:
Aspects to discuss:
CUtensorMapopaque type is enough, or do we need a higher-level object type.Todo: