Add sycl ext intel kernel queries extension #16834

kurapov-peter · 2025-01-29T18:10:45Z

This is a draft for the extension and its implementation. In a nutshell, the extension adds compiled kernel traits for the intel backend. The traits can be queried using the usual kernel::get_info() calls (and get_kernel_info). A new device aspect tells whether the query is supported on the device.

llvm/include/llvm/SYCLLowerIR/DeviceConfigFile.td

sycl/cmake/modules/FetchUnifiedRuntime.cmake

sycl/doc/extensions/proposed/sycl_ext_intel_kernel_queries.asciidoc

kurapov-peter · 2025-01-29T18:16:40Z

sycl/source/detail/kernel_info.hpp

+inline ext::intel::info::kernel::spill_mem_size::return_type
+get_kernel_device_specific_info<ext::intel::info::kernel::spill_mem_size>(
+    ur_kernel_handle_t Kernel, ur_device_handle_t Device,
+    const AdapterPtr &Adapter) {


Is it possible to check for the aspect here as well?

kurapov-peter · 2025-01-29T18:21:01Z

sycl/source/detail/kernel_info.hpp

+  // Retrieve the associated device list
+  size_t URDevicesSize = 0;
+  Adapter->call<UrApiKind::urProgramGetInfo>(Program, UR_PROGRAM_INFO_DEVICES,
+                                             0, nullptr, &URDevicesSize);
+
+  std::vector<ur_device_handle_t> URDevices(URDevicesSize /
+                                            sizeof(ur_device_handle_t));
+  Adapter->call<UrApiKind::urProgramGetInfo>(Program, UR_PROGRAM_INFO_DEVICES,
+                                             URDevicesSize, URDevices.data(),
+                                             nullptr);
+  assert(Result.size() == URDevices.size());
+
+  // Map the result back to the program devices
+  for (size_t idx = 0; idx < URDevices.size(); ++idx) {
+    if (URDevices[idx] == Device)
+      return Result[idx];
+  }


The mapping to devices relies on UR providing the same order within a single application (see oneapi-src/unified-runtime#2614 for details). There was a problem inside UR implementation that demanded the API to return an array of values for spills instead of a single value (due to device pointer unavailability). This implementation hides that inconvenience from SYCL users.

kurapov-peter · 2025-01-29T18:21:26Z

sycl/source/detail/kernel_info.hpp

+  throw exception(make_error_code(errc::runtime),
+                  "ext::intel::info::kernel::spill_mem_size failed to retrieve "
+                  "the requested value");


Is this the right type of exception?

kurapov-peter · 2025-01-29T18:22:39Z

sycl/test-e2e/Basic/kernel_info.cpp

There might be a better way to test the thing. This would just return zero. Any suggestions?

Here is my proposal for how the specification should be.

gmlueck · 2025-01-31T18:55:33Z

Hi @kurapov-peter, I hope you don't mind, but I pushed a commit with my proposal for how the specification should look in b027f87. Feel free to revert this commit if you are opposed, or to add comments in this PR.

Here is my rationale for some of the changes:

Since this PR is implementing the extension, it can't be "proposed". It needs to be either "experimental" or "supported". I thought the extension seemed simple enough to go straight to "supported", but we can do "experimental" if you think the API might change.
I reworded the Overview to be a bit more general, allowing us to add more queries in the future.
I think each query needs to have its own matching aspect. This is similar to what we did in sycl_ext_intel_device_info.
I changed the names to spell out "memory" because the newer APIs in SYCL tend to prefer whole words rather than abbreviations.
I changed the return type of the query to size_t because that seemed more appropriate for a C++ API.
I changed the namespace of the info descriptor to kernel_device_specific because this is the correct namespace for kernel info descriptors that are used with the get_info(const device&) overload.
I updated the general style of the specification to use a newer format that we are moving to.

Note that I did not update the implementation, so that will need to be changed to match the spec.

kurapov-peter · 2025-02-03T13:10:28Z

All the changes look reasonable to me. I'll update the implementation.

kurapov-peter · 2025-02-03T13:12:21Z

sycl/doc/extensions/supported/sycl_ext_intel_kernel_queries.asciidoc

+=== Spill memory size
+
+This query returns the kernel's spill memory size that is allocated by the
+compiler, as reported by Level Zero.


Is L0 important here?

Normally, we would not define a SYCL API in terms of a particular backend like this. However, it seemed to me that "spill memory size" is too vaguely defined. The reality is that SYCL is just returning whatever value comes from Level Zero. If people have questions about what it means, I'd rather direct them to Level Zero.

fair enough

kurapov-peter · 2025-02-04T16:06:34Z

#16810 is merged, so this has no dependencies. Putting it to ready for review.

AlexeySachkov · 2025-02-04T16:53:41Z

llvm/test/Instrumentation/MemorySanitizer/SPIRV/instrument_global_address_space.ll

@@ -128,6 +128,7 @@ attributes #2 = { nounwind }
 !77 = !{!"ext_oneapi_bindless_images_sample_2d_usm", i32 79}
 !78 = !{!"ext_oneapi_atomic16", i32 80}
 !79 = !{!"ext_oneapi_virtual_functions", i32 81}
+!79 = !{!"ext_intel_spill_memory_size", i32 82}


I doubt that this line has any effect on the test, so you may as well drop it. Moreover, I'm not sure what are the rules of redefining metadata, it may not be a valid change at all

AlexeySachkov · 2025-02-04T16:58:13Z

sycl/test-e2e/Basic/kernel_info.cpp

+  const size_t spillMemSz =
+      krn.get_info<ext::intel::info::kernel_device_specific::spill_memory_size>(
+          dev);
+  assert(spillMemSz >= 0);


This should be hidden under an if, the query is not guaranteed to be supported everywhere. The same goes for other change in this file.

gmlueck · 2025-02-04T21:34:02Z

@Pennycook could you review this for @intel/dpcpp-specification-reviewers ? I'm the author of the spec in this PR.

kurapov-peter added 3 commits January 29, 2025 18:02

Add extension doc draft

d6d4342

Specify UR version to inlude upcoming changes

a7fa67b

Add a draft implementation for the extension

3ed995a

kurapov-peter commented Jan 29, 2025

View reviewed changes

Merge branch 'sycl' into sycl_ext_intel_kernel_queries

c277979

kurapov-peter had a problem deploying to WindowsCILock January 30, 2025 11:51 — with GitHub Actions Failure

kurapov-peter requested a review from gmlueck January 31, 2025 15:42

Update specification

b027f87

Here is my proposal for how the specification should be.

kurapov-peter commented Feb 3, 2025

View reviewed changes

kurapov-peter added 4 commits February 3, 2025 14:20

Change names to spell out memory

6bcdddd

Change the return type to size_t

2f04373

Move spill_memory_size to kernel_device_specific

0adff86

Merge branch 'sycl' into sycl_ext_intel_kernel_queries

9e06d8b

kurapov-peter had a problem deploying to WindowsCILock February 3, 2025 15:52 — with GitHub Actions Failure

Update UR tag

5b4533e

kurapov-peter had a problem deploying to WindowsCILock February 4, 2025 15:23 — with GitHub Actions Error

fixup! Update UR tag

6d91045

kurapov-peter had a problem deploying to WindowsCILock February 4, 2025 15:29 — with GitHub Actions Error

fix formatting

5221dce

kurapov-peter had a problem deploying to WindowsCILock February 4, 2025 15:38 — with GitHub Actions Failure

kurapov-peter marked this pull request as ready for review February 4, 2025 16:05

kurapov-peter requested review from a team as code owners February 4, 2025 16:05

kurapov-peter requested a review from aelovikov-intel February 4, 2025 16:05

AlexeySachkov approved these changes Feb 4, 2025

View reviewed changes

kurapov-peter had a problem deploying to WindowsCILock February 4, 2025 17:23 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sycl ext intel kernel queries extension #16834

Add sycl ext intel kernel queries extension #16834

kurapov-peter commented Jan 29, 2025

kurapov-peter Jan 29, 2025

kurapov-peter Jan 29, 2025

kurapov-peter Jan 29, 2025

kurapov-peter Jan 29, 2025

gmlueck commented Jan 31, 2025 •

edited

Loading

kurapov-peter commented Feb 3, 2025

kurapov-peter Feb 3, 2025

gmlueck Feb 3, 2025

kurapov-peter Feb 3, 2025

kurapov-peter commented Feb 4, 2025

AlexeySachkov Feb 4, 2025

AlexeySachkov Feb 4, 2025

gmlueck commented Feb 4, 2025

Add sycl ext intel kernel queries extension #16834

Are you sure you want to change the base?

Add sycl ext intel kernel queries extension #16834

Conversation

kurapov-peter commented Jan 29, 2025

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

gmlueck commented Jan 31, 2025 • edited Loading

kurapov-peter commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurapov-peter commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlueck commented Feb 4, 2025

gmlueck commented Jan 31, 2025 •

edited

Loading