-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL][Doc] Add spec to get device index #20007
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
Conversation
Add a proposed extension specification that allows applications to easily get the index of a `device` object and to get a `device` object from its index.
class device { | ||
// ... | ||
int ext_oneapi_to_index() const; | ||
static device ext_oneapi_from_index(int index); |
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.
Personally, I think ext_oneapi_from_index(i)
is a little unnecessary. It is effectively just a "shorthand" for device::get_devices()[i]
, but not currently actually shorter:
device::ext_oneapi_from_index(i)
device::get_devices()[i]
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 agree with this, but it seemed better to add a pair of APIs to convert back and forth between index and device
, rather than just adding a single API to do a one-way conversion. In addition, it is a bit wasteful to construct a std::vector
if the user just wants a single element from that vector.
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 agree with this, but it seemed better to add a pair of APIs to convert back and forth between index and
device
, rather than just adding a single API to do a one-way conversion.
Since the API is already talking about the return value being an index into the vector of devices, I am not personally too much against just having the one-way API added here. It is a minor thing, but it is also an additional API to maintain. I suppose we could maybe just reword it to say "Same as calling device::get_devices()[i]
" or something along those lines.
In addition, it is a bit wasteful to construct a
std::vector
if the user just wants a single element from that vector.
That is a good point. I would be tempted to say we would want to do that internally as well, simply to make sure the sets of devices don't suddenly change due to the adapters not making any ordering guarantees, but maybe some implementations could avoid that.
That said, even if we did keep an internal version for book-keeping, the API for get_devices()
would return a copy of it, so you are right that it would be wasteful.
@intel/llvm-gatekeepers please consider merging |
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.
LGTM!
Yes. @intel/llvm-gatekeepers, I think this is ready to mege. |
Add a proposed extension specification that allows applications to easily get the index of a
device
object and to get adevice
object from its index.