[SYCL][Bindless][UR][E2E] Add image memory and handle support queries #17865

przemektmalon · 2025-04-04T17:15:01Z

This patch introduces two new SYCL queries for Bindless Images.

get_image_memory_support returns a vector of supported image backing memory types for a device given an `image_descriptor.

get_image_handle_supported returns a boolean indicating whether the device supports creation of either a sampled_image_handle or unsampled_image_handle, given an image_descriptor and image backing memory type.

Some tests are updated to use these queries to filter out unsupported image properties, e.g. allocating unorm channel types on the LevelZero backend.

This patch introduces two new SYCL queries for Bindless Images. `get_image_memory_support` returns a vector of supported image backing memory types for a device given an `image_descriptor. `get_image_handle_supported` returns a boolean indicating whether the device supports creation of either a `sampled_image_handle` or `unsampled_image_handle`, given an `image_descriptor` and image backing memory type. Some tests are updated to use these queries to filter out unsupported image properties, e.g. allocating `unorm` channel types on the LevelZero backend.

hvdijk

The NativeCPU part looks fine.

Seanst98 · 2025-04-07T09:56:23Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_bindless_images.asciidoc

+
+template <typename ImageHandleType>
+bool
+get_image_handle_supported(const image_descriptor &imageDescriptor,


The naming is a little inconsistent here, or is that purposeful to highlight that one returns a vector of support and the other is a bool? get_image_memory_support VS get_image_handle_supported.

or is that purposeful to highlight that one returns a vector of support and the other is a bool?

That's correct.

sycl/test-e2e/bindless_images/helpers/common.hpp

Seanst98 · 2025-04-07T10:04:41Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_bindless_images.asciidoc

+enum class image_memory_handle_type : /* unspecified */ {
+  usm_pointer,
+  opaque_handle
+};


Is there ever going to be more than two options here?

It feels odd that this first query returns a vector for only two elements that are queried whereas the next query also only has two options to query yet that chooses to return a bool based on the templated handle type. Why aren't both sets of queries consistent in this regard? It feels awfully fiddly for users.

It's plausible to consider that in future the extension might provide support for more than two types of memory backing, e.g. non-usm pointer for host-task only. It's less plausible that we will have more than two types of image handles (sampled & unsampled).

We may also consider splitting usm_pointer into distinct host_, device_, and shared_ variants, which would allow backends to provide finer grained allocation support queries that depend on all the information in an image_descriptor, as opposed to the current true/false device aspects.

These decisions are not set in stone yet, and the API can change in the future, as this is still an experimental extension.

Seanst98 · 2025-04-07T10:10:32Z

sycl/source/detail/bindless_images.cpp

+  bool supportsPointerAllocation = false;
+  Adapter->call<
+      sycl::errc::runtime,
+      sycl::detail::UrApiKind::urBindlessImagesGetImageMemoryPointerSupportExp>(


Does there really need to be two distinct funcs for this query? Why can't you have one func and pass in an enum for what you're specifically querying for as is done in other UR query funcs?

Looking further into this PR, it appears you end up calling into one massive function anyway. Why not do this at a higher level and reduce the number of UR funcs you have defined?

I weighed this against the addition of two new UR enum types that would not be used outside of passing values to these two queries and thought it simpler to just provide two functions. We could also use a boolean to indicate the memory/image handle types, however, I thought that would make these UR functions less clear.

Additionally, merging these UR functions might also make them less maintainable, as you already mentioned below, these queries tend to grow quite large in order for each backend to verify the properties in a given image descriptor are valid for every image type, while taking into account the type of backing memory.

The current was the most direct and simple solution to the problem as it currently stands. But I can see how passing an enum to the UR function is also valid and might be desirable in some circumstances.

Seanst98 · 2025-04-07T10:12:03Z

sycl/source/detail/bindless_images.cpp

+  bool supportsUnsampledHandle = false;
+  Adapter->call<sycl::errc::runtime,
+                sycl::detail::UrApiKind::
+                    urBindlessImagesGetImageUnsampledHandleSupportExp>(


Same here for the two different funcs which appear to be identical in signature other than the name.

Seanst98 · 2025-04-07T10:26:12Z

unified-runtime/source/adapters/hip/device.cpp

@@ -248,94 +248,46 @@ UR_APIEXPORT ur_result_t UR_APICALL urDeviceGetInfo(ur_device_handle_t hDevice,
    return ReturnValue(128u);
  }
  case UR_DEVICE_INFO_IMAGE2D_MAX_HEIGHT: {
-    // Take the smaller of maximum surface and maximum texture height.


You've made changes to the existing queries in the HIP adapter but there's no mention of this in your PR description. Shouldn't these changes come in as a separate commit specific to the fixes made here?

Some of these changes just remove redundant calls to hipDeviceGetAttribute that I've fixed, alongside the queries for MAX_IMAGE_LINEAR_<DIM> which are currently required for the HIP UR backend image verification to work.

It could be done as a separate commit, but I don't think fixing the HIP device queries that pertain to supported image properties strays too far away from the spirit of this PR.

sycl/test/abi/sycl_symbols_windows.dump

Seanst98 · 2025-04-07T10:38:00Z

unified-runtime/source/adapters/cuda/image.cpp

@@ -1006,6 +1006,347 @@ UR_APIEXPORT ur_result_t UR_APICALL urBindlessImagesImageGetInfoExp(
  }
 }

+bool verifyCommonImagePropertiesSupport(const ur_device_handle_t hDevice,


This function looks awfully unmaintainable. Is there a better way to do this?

I'll need to come back later to go over this.

It could be split into several functions that verify a single image type each, however that could be up to 5 extra helper functions.

Seanst98 · 2025-04-07T10:39:26Z

sycl/test/abi/sycl_symbols_windows.dump

@@ -44,6 +44,10 @@
 ??$get_backend_info@Uversion@platform@info@_V1@sycl@@@kernel@_V1@sycl@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
 ??$get_backend_info@Uversion@platform@info@_V1@sycl@@@platform@_V1@sycl@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
 ??$get_backend_info@Uversion@platform@info@_V1@sycl@@@queue@_V1@sycl@@QEBA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@XZ
+??$get_image_handle_supported@Usampled_image_handle@experimental@oneapi@ext@_V1@sycl@@@experimental@oneapi@ext@_V1@sycl@@YA_NAEBUimage_descriptor@01234@W4image_memory_handle_type@01234@AEBVdevice@34@AEBVcontext@34@@Z
+??$get_image_handle_supported@Usampled_image_handle@experimental@oneapi@ext@_V1@sycl@@@experimental@oneapi@ext@_V1@sycl@@YA_NAEBUimage_descriptor@01234@W4image_memory_handle_type@01234@AEBVqueue@34@@Z


Again, unrelated to the line here: How does these changes mix with the existing image_descriptor::verify() method. Would it not be a good idea to incorporate the two somehow?

Would it not be a good idea to incorporate the two somehow?

I think we need to have another look at the verify function. I'm not sure whether it's still completely applicable, at least in its current state, as it cannot take into account the backend used or the type of image backing memory the descriptor is going to be used to allocate.

The queries I've introduced in this PR do far more than the verify function can without taking sycl::device/context/queue parameters, and an indication of the backing memory type.

The queries introduced here also cover creation of sampled/unsampled image handles, should those be incorporated into the verify function?

As it stands the verify function acts as a very simple sanity check, and there is specific wording in the documentation as to what it checks.

Going forward, I think the best option may be to get rid of the verify function in favor of the new queries introduced in this PR, as they are far more rigorous, by taking into the backend used (each backend has some differences in what it provides support for) and the image backing memory type. A general verify function seems too broad to accommodate verification of an image_descriptor, as the allocation of the descriptor depends on the properties I mention above.

Seanst98 · 2025-04-07T10:43:37Z

sycl/test-e2e/bindless_images/vulkan_interop/sampled_images.cpp

-  sycl::device dev;
-  sycl::queue q(dev);
-  auto ctxt = q.get_context();
+  auto dev = syclQueue.get_device();
+  auto ctxt = syclQueue.get_context();


nit: To make this PR easier to review what changes are necessary vs what are not necessary, any NFC changes should be left to a different PR.

This change is not purely NFC.

In order to query the device for image support in run_test I needed access to the sycl::device and sycl::context, which I did by creating a sycl::queue.

Then, in order to ensure I'm operating on the same queue/device here in run_sycl, I passed that queue as a parameter here, instead of creating a whole new queue object.

aarongreig · 2025-04-08T09:02:21Z

unified-runtime/scripts/core/exp-bindless-images.yml

+    - type: bool
+      name: isOpaqueAllocation
+      desc: "[in] indicates whether the image memory would be backed by an opaque handle allocation"
+    - type: bool*


can you change these to use the $x_bool_t typedef

Updated, now using $x_bool_t

aarongreig

UR + CL adapter lgtm

przemektmalon requested review from a team as code owners April 4, 2025 17:15

przemektmalon requested review from ldrumm and slawekptak April 4, 2025 17:15

przemektmalon temporarily deployed to WindowsCILock April 4, 2025 17:15 — with GitHub Actions Inactive

przemektmalon temporarily deployed to WindowsCILock April 4, 2025 17:28 — with GitHub Actions Inactive

przemektmalon had a problem deploying to WindowsCILock April 4, 2025 17:28 — with GitHub Actions Failure

Merge branch 'sycl' into przemek/image-support-queries

4581989

przemektmalon had a problem deploying to WindowsCILock April 7, 2025 10:19 — with GitHub Actions Error

hvdijk approved these changes Apr 7, 2025

View reviewed changes

Fix include

e7a57b0

przemektmalon temporarily deployed to WindowsCILock April 7, 2025 10:37 — with GitHub Actions Inactive

Seanst98 requested changes Apr 7, 2025

View reviewed changes

przemektmalon had a problem deploying to WindowsCILock April 7, 2025 10:51 — with GitHub Actions Error

Pass image_descriptor by ref

b054575

przemektmalon had a problem deploying to WindowsCILock April 7, 2025 11:19 — with GitHub Actions Error

Fix return value for skipping read_2D test

1824593

przemektmalon temporarily deployed to WindowsCILock April 7, 2025 11:29 — with GitHub Actions Inactive

przemektmalon had a problem deploying to WindowsCILock April 7, 2025 11:44 — with GitHub Actions Failure

przemektmalon temporarily deployed to WindowsCILock April 7, 2025 11:44 — with GitHub Actions Inactive

przemektmalon changed the title ~~Add image memory and handle support queries~~ [SYCL][Bindless][UR}Add image memory and handle support queries Apr 7, 2025

przemektmalon changed the title ~~[SYCL][Bindless][UR}Add image memory and handle support queries~~ [SYCL][Bindless][UR] Add image memory and handle support queries Apr 7, 2025

przemektmalon changed the title ~~[SYCL][Bindless][UR] Add image memory and handle support queries~~ [SYCL][Bindless][UR][E2E] Add image memory and handle support queries Apr 7, 2025

aarongreig reviewed Apr 8, 2025

View reviewed changes

przemektmalon added 3 commits April 8, 2025 11:46

Use ur_bool_t

e0fc137

Merge branch 'sycl' into przemek/image-support-queries

630ebb4

Update use of ur_bool_t

65cb5b4

przemektmalon temporarily deployed to WindowsCILock April 8, 2025 11:23 — with GitHub Actions Inactive

przemektmalon had a problem deploying to WindowsCILock April 8, 2025 11:58 — with GitHub Actions Failure

przemektmalon temporarily deployed to WindowsCILock April 8, 2025 11:58 — with GitHub Actions Inactive

aarongreig approved these changes Apr 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Bindless][UR][E2E] Add image memory and handle support queries #17865

[SYCL][Bindless][UR][E2E] Add image memory and handle support queries #17865

przemektmalon commented Apr 4, 2025

hvdijk left a comment

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025 •

edited

Loading

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025

Seanst98 Apr 7, 2025

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025 •

edited

Loading

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025

Seanst98 Apr 7, 2025

przemektmalon Apr 7, 2025 •

edited

Loading

aarongreig Apr 8, 2025

przemektmalon Apr 8, 2025

aarongreig left a comment

[SYCL][Bindless][UR][E2E] Add image memory and handle support queries #17865

Are you sure you want to change the base?

[SYCL][Bindless][UR][E2E] Add image memory and handle support queries #17865

Conversation

przemektmalon commented Apr 4, 2025

hvdijk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemektmalon Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemektmalon Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemektmalon Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongreig left a comment

Choose a reason for hiding this comment

przemektmalon Apr 7, 2025 •

edited

Loading

przemektmalon Apr 7, 2025 •

edited

Loading

przemektmalon Apr 7, 2025 •

edited

Loading