-
Notifications
You must be signed in to change notification settings - Fork 300
Add internal cuda::__is_device_memory
#6918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
davebayer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version should be internal only, trying to be optimistic - if something fails, let's just return true.
|
Why it should be internal only? It looks consistent and aligned with the other APIs. If we need it, it is very likely that it could be used by other users as well.
I don't understand. The behavior is roughly equivalent to |
|
Because we cannot ask this question in general. If there are 2 devices without peer access enabled, you cannot possibly know the memory living in 1st device is not accessible from the second, so what should the function return in that case? Also, I disagree that it is well aligned with what we do in other apis. All device apis take either explicit device_ref parameter or get the device from stream for example. If you have a device memory resource, you specify which device you want to create the resource for. If we must expose this, is_any_device_accessible(ptr) would be better in my opinion. But I still think that we should make the function internal so we can just return true in case of an error, because I believe we don't want the device accessor to throw a cuda_error if we are just not able to confirm it really is accessible from a device. |
my point is not if it is accessible from GPU A or B. The information that I get is that the input is a device pointer, which is already something valuable.
I meant consistent with other
We can keep it internal if it creates ambiguity, I'm not strongly against it. However, I'm more in favor of exposing it. Peer accessibility looks more an edge case (I could be wrong). |
|
Could we name it |
|
|
|
ok, let keep it internal. I need it to unblock a chain of PRs. |
cuda::is_device_accessiblecuda::__is_device_memory
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 1h 37m: Pass: 100%/91 | Total: 1d 18h | Max: 1h 37m | Hits: 96%/201901See results here. |
| * @return `true` if the pointer is a device pointer, `false` otherwise. | ||
| */ | ||
| [[nodiscard]] | ||
| _CCCL_HOST_API inline bool __is_device_memory(const void* __p) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's counterintuitive that __is_device_memory includes managed memory as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really open to suggestions for a better name. The idea of the function is to provide a weak guarantee about device memory access when the device id is not available. __is_device_accessible has been discarded because of peer access
Description
PR #6733 shows that we also need a relaxed
cuda::is_device_accessiblewhich doesn't require a device reference. The overall idea is to just check if a pointer is device allocated.