Applies is_pointer_accessible to mdspan
#6733
Conversation
fbusato
changed the title
is_pointer_accessible to mdspanis_pointer_accessible to mdspan
This comment has been minimized.
This comment has been minimized.
Co-authored-by: David Bayer <[email protected]>
Co-authored-by: David Bayer <[email protected]>
Co-authored-by: David Bayer <[email protected]>
|
@davebayer @miscco I'm starting thinking that we are doing the wrong thing for host/device/managed.
Ideally,
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| static const auto __dev_id = static_cast<int>(::cuda::__driver::__ctxGetDevice()); | ||
| return ::cuda::is_device_accessible(::cuda::std::to_address(__p), ::cuda::device_ref{__dev_id}); |
There was a problem hiding this comment.
This is not correct, we don't want to check for the current device. We should just try to see whether it's accessible from any device here
There was a problem hiding this comment.
you suggested adding device_ref at the time. It makes sense, but it also makes sense to have a relaxed version without it, as I initially proposed.
Co-authored-by: David Bayer <[email protected]>
Co-authored-by: David Bayer <[email protected]>
Co-authored-by: David Bayer <[email protected]>
😬 CI Workflow Results🟥 Finished in 2h 01m: Pass: 92%/90 | Total: 2d 02h | Max: 2h 01m | Hits: 84%/197383See results here. |
| NV_IF_TARGET(NV_IS_DEVICE, (_CCCL_VERIFY(false, "cuda::__host_accessor cannot be used in DEVICE code");)) | ||
| return _Accessor::access(__p, __i); |
There was a problem hiding this comment.
Why are we dropping the check here? that seems like a regression
There was a problem hiding this comment.
yes, this is on purpose. We don't want to call a driver API for each access of a mdspan. This is overkill
There was a problem hiding this comment.
Suggestion: there are different ways to store that information during construction. We could either make it a boolean that is set during construction or turn the pointer into a tagged pointer which would be much more onvolved though
There was a problem hiding this comment.
I don't think we need to store any extra information. Checking the pointer validity during mdspan (not accessor) construction is enough.
| { | ||
| if constexpr (::cuda::std::__has_detect_invalidity<accessor_type>) | ||
| { | ||
| const auto __tmp = mapping(); // workaround for clang with nodiscard |
There was a problem hiding this comment.
Above you are declaring this [[maybe_unused]] I believe we should do that in all places
There was a problem hiding this comment.
yes, [[maybe_unused]] is correct because it is not used in release mode. The code is terrible for doing a trivial check, but I don't have better workaround for the clang warning
4 participants
Description
Replace old code in host/device/managed mdspan with https://nvidia.github.io/cccl/libcudacxx/extended_api/memory/is_pointer_accessible.html