Skip to content

[Bindless][SYCL][UR] Create a sampled image with a single UR API #18384

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

Open
wants to merge 12 commits into
base: sycl
Choose a base branch
from

Conversation

GeorgeWeb
Copy link
Contributor

@GeorgeWeb GeorgeWeb commented May 9, 2025

We no longer need to create a UR sampler object in order to create a sampled image. The backends can simply use the sampler description to create the sampled image.

Note this is an ABI break in the currently still experimental bindless images extension for Unified Runtime.

Addresses the following issue: oneapi-src/unified-runtime#1463
Relevant discussion from oneapi-src/unified-runtime#2640 (comment)

Additionally, avoiding the allocation of a sampler object itself fixes memory leak(s) in all adapters where the UR sampler handle was never released which was caused by the use of the API in the SYCL RT only calling urSamplerCreate and not Release for the sampler associated with the image. Also, while HIP and Cuda do not further create backend specific sampler objects in the respective adapters, Level Zero was creating one that never got released upon freeing the image memory which lead to leaking the Level Zero sampler memory as well. Now that Level Zero can create a sampled image from a descriptor only, the sampler object is not needed altogether, allowing to rid of the above mentioned leaking memory allocations and simplify the runtime.

We no longer need to create a UR sampler object in order to create a sampled image.
The backends can simply use the sampler description to create the sampled image.
@GeorgeWeb GeorgeWeb force-pushed the georgi/create-sampled-image-without-sampler branch from 544bf40 to 6693ade Compare May 9, 2025 11:37
@GeorgeWeb GeorgeWeb temporarily deployed to WindowsCILock May 9, 2025 11:37 — with GitHub Actions Inactive
@GeorgeWeb GeorgeWeb marked this pull request as ready for review May 9, 2025 12:20
@GeorgeWeb GeorgeWeb requested review from a team as code owners May 9, 2025 12:20
@GeorgeWeb GeorgeWeb assigned wenju-he and unassigned wenju-he May 9, 2025
@GeorgeWeb GeorgeWeb requested a review from wenju-he May 9, 2025 12:24
@GeorgeWeb GeorgeWeb temporarily deployed to WindowsCILock May 9, 2025 12:50 — with GitHub Actions Inactive
@GeorgeWeb GeorgeWeb temporarily deployed to WindowsCILock May 9, 2025 12:50 — with GitHub Actions Inactive
Copy link
Contributor

@aarongreig aarongreig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UR + CL LGTM

Copy link
Contributor

@przemektmalon przemektmalon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments and one missing error check, otherwise LGTM.

@GeorgeWeb
Copy link
Contributor Author

GeorgeWeb commented May 12, 2025

The following CI failures for Intel Arc A Series GPUs are unrelated.
(https://github.com/intel/llvm/actions/runs/14972970580/job/42059340348)

Failed Tests (4):
  SYCL :: KernelAndProgram/cache_env_vars.cpp
  SYCL :: KernelAndProgram/cache_env_vars_lin.cpp
  SYCL :: KernelCompiler/opencl.cpp
  SYCL :: KernelCompiler/sycl_cache_pm.cpp

These appeared after rebasing.
See issue #18416.

Copy link
Contributor

@wenju-he wenju-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@GeorgeWeb
Copy link
Contributor Author

GeorgeWeb commented May 15, 2025

The following CI failures for Intel Arc A Series GPUs are unrelated.
(https://github.com/intel/llvm/actions/runs/15024003764/job/42222242149?pr=18384)

Failed Tests (3):
  SYCL :: Adapters/level_zero/interop-buffer-ownership.cpp
  SYCL :: Adapters/level_zero/usm_device_read_only.cpp
  SYCL :: Basic/buffer/buffer_create.cpp

See issue #18463.

@GeorgeWeb
Copy link
Contributor Author

@intel/llvm-gatekeepers This PR is ready to merge.
The CI failures are unrelated, so I'll leave it to you if you'd prefer waiting to get the CI green once a fix for those issues is merged before merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants