[L0 v2] add support for images #2640
Conversation
github-actions
bot
added
conformance
igchor
force-pushed
the
images
branch
2 times, most recently
from
e6a621c to
37af7a4
Compare
| // NULL is a valid value | ||
| zePtr = pending.hMem->getDevicePtr(hDevice, pending.mode, 0, | ||
| pending.hMem->getSize(), migrate); | ||
| if (!pending.hMem->isImage()) { |
There was a problem hiding this comment.
looks like we should have an abstraction getPtr() from pending.hMem
There was a problem hiding this comment.
Maybe, but it's not a great abstraction - we would need to pass 'migrate' which doesn't make any sense for images.
| @@ -95,7 +100,7 @@ ur_result_t urSamplerCreate( | |||
| } | |||
| } | |||
|
|
|||
| ZE2UR_CALL(zeSamplerCreate, (Context->ZeContext, Device->ZeDevice, | |||
There was a problem hiding this comment.
What is the status of this issue on the L0 side? #1463
Are you still planning to replace zeSamplerCreate with zeImageCreate which would allow urSamplerCreate (which is not officially in bindless-images UR spec) to be removed from bindless-image dpc++ calls,
or do we need to add a new version of the urSamplerCreate API that takes a device argument, so that we avoid setting a different device here than the one that the user passes to create_image here https://github.com/intel/llvm/blob/b0e975c88531125d077687549a2e40a78612507f/sycl/source/detail/bindless_images.cpp#L376
?
There was a problem hiding this comment.
That's a good question. I don't actually know what is the status, perhaps @przemektmalon or @wenju-he can comment?
In any case, this PR doesn't really change any logic related to the sampler - it only enables it for v2. Any change should probably be done in a separate PR.
There was a problem hiding this comment.
There was a problem hiding this comment.
What is the status of this issue on the L0 side? #1463
Remaining work of #1463 should be done in UR for all adapters at the same time. urSamplerCreate should not be called for bindless image. So #1463 isn't done yet.
Any change should probably be done in a separate PR.
Yes I think so.
So there is no blocker on the l0 side to removing the call to urSamplerCreate: because you've presumably either either replaced fully the functionality of zuSamplerCreate by zeImageCreate (as indicated by the comment in #1463) or otherwise?
Is the below now possible?
"We plan to update Level-zero bindless image spec to allow a sampled image to be created with a single zeImageCreate call with additional sampler descriptor information. Then a single 64-bit handle works for a Level-zero sampled image. "
There was a problem hiding this comment.
So there is no blocker on the l0 side to removing the call to
urSamplerCreate: because you've presumably either either replaced fully the functionality ofzuSamplerCreatebyzeImageCreate(as indicated by the comment in #1463) or otherwise?
There is no blocker, but replaced fully the functionality of zuSamplerCreate by zeImageCreate isn't done yet. I think it should be done for all adapters at the same time before we can remove the call to urSamplerCreate at https://github.com/intel/llvm/blob/d3568afe1d7142b6eab54403f8f1db8b8e1dcd27/sycl/source/detail/bindless_images.cpp#L416
urSamplerCreate in L0 adapter does two things: one is setting up sampler descriptor and another is creating a sampler object from the descriptor. The first part is still needed for bindless image.
urSamplerCreate in cuda and hip adapters only set up sampler descriptor.
My understanding is that the functionality of translating ur sampler description to backend sampler description can be moved into a utility function, which should be called in both urSamplerCreate and urBindlessImagesSampledImageCreateExp.
Is the below now possible?
"We plan to update Level-zero bindless image spec to allow a sampled image to be created with a single zeImageCreate call with additional sampler descriptor information. Then a single 64-bit handle works for a Level-zero sampled image. "
Yes, this is already implemented in L0 adapter.
There was a problem hiding this comment.
My understanding is that the functionality of translating ur sampler description to backend sampler description can be moved into a utility function, which should be called in both urSamplerCreate and urBindlessImagesSampledImageCreateExp.
That's correct.
The change to the Bindless Images UR spec would be to replace the ur_sampler_handle_t argument with a ur_sampler_desc_t, in the call to urBindlessImagesSampledImageCreateExp.
Then we could remove the call to urSamplerCreate from the SYCL RT. This is already possible, as you point out, on all of our backends, including L0 where currently we create an unnecessary ur_sampler_handle_t.
And as you note, the utility function to set up the sampler description would be made available to both urBindlessImagesSampledImageCreateExp and urSamplerCreate.
We still have the internal tracker for this, and I agree that this should be done as a separate PR. Our internal priority on this issue is still low for now, however, we are in the process of discussing planned priorities for all of our issues, and since this issue has existed for a while, with the fix being quite straightforward, we may be able to bump it.
Move common functionality to helpers/image_helpers.*
No description provided.