[SYCL][Bindless] Replace 'image_channel_order' field in 'image_descriptor' with number of channels #13745
Conversation
…, sampling and writing to images within a kernel This commit adds support to set image channel order when fetching, sampling and writing. Removed 'get_channel_order' function from 'image_mem' as it is redundant because images no longer have or need a notion of channel order. Only channel size.
| By default, no reordering will occur. Only when specified by the user. The | ||
| following different template argument layouts are valid for `fetch_image`, | ||
| `sample_image`, `sample_mipmap`, `fetch_image_array`, `fetch_cubemap` and | ||
| `sample_cubemap`: |
There was a problem hiding this comment.
This seems confusing.
So if my image is rgba ordered and I want to read a pixel with fetch_image but get that pixel in argb order, I can do that with the template arg. And, similarly if I have a argb order pixel and want to write it to this image, I can do that with the template arg. Got it.
But what if my original image data is argb order? Well, since we are removing image_channel_order from the image_descriptor then it seems like that knowledge is no longer stored in the image. OK. And if I want matching argb order pixels I should just fetch and write without template args, right? Ok. But what if I want to convert the pixel to rgba order? What conversion template arg do I use? Not rgba, because that's the no-op right? And not argb because that'll convert my data to barg, wouldn't it?
I think there is something I'm not appreciating.
There was a problem hiding this comment.
If you had an image who's data is in argb order, to get it back into rgba order, you would have to read it again via argb template arg.
The problem is that Cuda has no notion of channel order in surfaces and textures. Note that even though image_channel_order was used to construct image_descriptor, it was only used to get the number of channels Because Cuda has no notion of channel order, there are only two ways in which image channel reordering could work.
- Add an
image_channel_orderfield tounsampled_image_handleandsampled_image_handle. With this, it would always be known what channel order the image is in and what to convert do. - Treat all images as being in
rgbaorder and convert from there to the passed channel order when reading/writing.
We first were going with option 1, but we became concerned about tacking in extra information into the image handle structs from a performance perspective due to higher register use. So option 2 was chosen as it left the image handles as lightweight as possible.
Every read and write treats the data as already being in rgba order and converts from there to the passed template arg.
One additional thing I want to note, L0 and OpenCL do have notions of channel order, but they can only be set on a per image basis and not per read/write. (I am not sure if this info is important)
There was a problem hiding this comment.
If you had an image who's data is in argb order, to get it back into rgba order, you would have to read it again via argb template arg.
Are you sure? Can you make a test that demonstrates these "same" and "cross" cases, especially where the original data isn't rgba?
When you say "read it again" you mean using the fetch_image<T> routine? You are saying that fetch_image<argb> when applied to a rgba order image will fetch that pixel in argb order. But that same call fetch_image<argb> when applied to a argb order image will fetch that pixel in rgba order? That's seems confusing. And magical. In the first case it changes 0123 to 3012 and in the second 0123 to 1230.
I'm sorry to be obtuse - I think there's something I'm not getting.
There was a problem hiding this comment.
No problem. I got my swaps completely mixed up and what I suggested does not work.
I did not fully appreciate how awkward this is. Problem is all conversions are treated as converting from rgba to the supplied channel order. We have had some discussions internally and we will be changing this current design. Either fetch_image will know what the channel order the pixel is converting from as well as what channel order to convert to. Something like:
fetch_image<int, argb/*FromCOrder*/, rgba/*ToCOrder*/>I have done some templating experimentation and it should work. Development is a little awkward as we really want to limit the number of extra template functions needed to get the templating working nicely from a use perspective.
Or introduce a separate channel conversion utility function. Something like:
template <typename DataT, image_channel_order FromCOrder, image_channel_order ToCOrder>
DataT swap_channels(DataT data)One thing that we do want to merge soonish from this PR is removing the image_channel_order field from image_descriptor and replacing it with the number of channels. As I mentioned before, it is only used to know the number of channels an image has and can't be used for anything else when considering cuda's limitation of having no notion of channel order and not wanting to package any extra data in the image handle structs.
I am going to draft this PR for now and remove the channel swapping code but keep the changes to image_descriptor. Adding channel swapping support can be done later and is not the highest priority with the 2025 release coming up.
…nel_order' from 'image_descriptor'
DBDuncan
changed the title
DBDuncan
changed the title
|
@cperkinsintel I have removed the image channel reordering changes and kept replacing |
|
Pinging @intel/bindless-images-reviewers @intel/unified-runtime-reviewers |
|
Friendly ping @dm-vodopyanov |
|
@cperkinsintel Hey. Plan is to revisit image channel reordering later. Want to make you aware that I am going to move to get this merged with the image channel reordering code removed. |
|
@intel/llvm-gatekeepers This should be ready to merge. |
'image_channel_order' field in 'image_descriptor' is and can only be used for the number of channels due to CUDA having no notion of image channel order. Replaced with the number of channels instead.
Also deprecate 'get_channel_order' function from 'image_mem' as it is redundant because images no longer have or need a notion of channel order. Only channel size.