-
Notifications
You must be signed in to change notification settings - Fork 803
[NFC][SYCL] Refactor code around device_image_impl::KernelIDs
#19516
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
fbbf961 to
4e11774
Compare
4e11774 to
c69c3ba
Compare
| "State mismatch between main image and its dependency"); | ||
| DeviceImageImplPtr DepImpl = | ||
| device_image_impl::create(DepImage, Ctx, Devs, DepState, DepKernelIDs, | ||
| device_image_impl::create(DepImage, Ctx, Devs, DepState, std::move(DepKernelIDs), |
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.
An alternative to part of this change would be to initialize DepKernelIDs if it's nullptr here and add an assert in device_image_impl::create to make sure that device_image_impl::KernelIDs is always non-null. @steffenlarsen, what approach would you like better?
c69c3ba to
a4a6feb
Compare
device_image_impl::get_kernel_ids_ptrdevice_image_impl::KernelIDs
a4a6feb to
f7ed339
Compare
* Clarify the reason for `std::shared_ptr` (performance) * Clarify that `get_kernel_ids` is preferred over `get_kernel_ids_ptr` and update some of the existing uses of the latter to use the former * Change `get_kernel_ids` to return `iterator_range` instead of the reference to the underlying container. That way we can also return an empty range when `shared_ptr` isn't initialized, which was possible before but many ctors led to believe that is guaranteed to never happen. * Change those ctors to keep empty `shared_ptr` instead of creating empty `vector`s
f7ed339 to
146abac
Compare
steffenlarsen
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.
Good idea!
|
Arc: Reported in #19305 |
std::shared_ptr(performance,see Improve get_kernel_bundle performance #5496)
get_kernel_idsis preferred overget_kernel_ids_ptrandupdate some of the existing uses of the latter to use the former
get_kernel_idsto returniterator_rangeinstead of thereference to the underlying container. That way we can also return an
empty range when
shared_ptrisn't initialized, which was possiblebefore but many ctors led to believe that is guaranteed to never happen.
shared_ptrinstead of creatingempty
vectors