[SYCL][Bindless] Device 'image_mem_handle' to 'image_mem_handle' Sub-Region Copy #15579
Conversation
…Region Copy Add support for device 'image_mem_handle' to 'image_mem_handle' sub-region copies and implement tests
|
Friendly ping @intel/llvm-reviewers-runtime @cperkinsintel |
| const ext::oneapi::experimental::image_descriptor &ImageDesc); | ||
|
|
||
| // Device to device copy with offsets and extent | ||
| void ext_oneapi_copy( |
There was a problem hiding this comment.
are we sure about this name? For something so generic like ext_oneapi_copy I would expect something that handles any type of memory (not just image data) and maybe across all address spaces (host, device). Much like queue.copy() is now.
But, the comments on this routine suggest its capability is much more focused. If so, I think it's name should express that. maybe something like ext_oneapi_d2d_img_copy or ext_d2d_img_copy ?
There was a problem hiding this comment.
We already have a lot of overloads that perform copies. The reason we chose this originally is we just intended to extend the existing copy functionality to support new bindless images. Also, our overloads are not just images to images but also memory to memory copies.
There was a problem hiding this comment.
There are other new overloads planned to be added. Such as host to host and device usm to device image_mem_handle.
There was a problem hiding this comment.
I have been thinking that perhaps the amount of ext_oneapi_copy overloads we have is getting a bit unwieldy. Especially considering a number additional ones need to be added to support more device to device copy variants.
I don't think it would be a good idea to start making changes involving other copy functions here. But something to be looked into later.
There was a problem hiding this comment.
sorry for not responding to your comments earlier. If there are other overloads that handle other data types, it wouldn't hurt to mention that in the comment. Even just "this overload of ext_copy is for bindless images".
There was a problem hiding this comment.
No problem, thanks for the approval! On your suggestion, it would likely be best to apply a comment like you suggest to all the overloads at the same time which is a bit out of scope it feels for this PR. As there are 50~ of them (Have to triple them in queue.hpp...). queue.hpp and handler.hpp need a bit of a refactor which is very tentatively planned early next year. Double checking the order of the functions and their corresponding comments. Comments stating they are overloads for bindless images can be added then.
…gDesc' to align with existing code
DBDuncan
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
|
@cperkinsintel Friendly ping |
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
DBDuncan
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers Please merge, thanks! |
5 participants
Add support for device 'image_mem_handle' to 'image_mem_handle' sub-region copies and implement tests