Add sycl ext intel kernel queries extension #16834

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

This adds a sycl_ext_intel_kernel_queries extension for target-specific low-level information retrieval. 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). Each query gets its own aspect to tell whether the query is supported on the device. This PR adds ext_intel_spill_memory_size aspect and an implementation for querying the spilled memory size as reported by L0.

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.

Fixed in UR v1 adapter

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?

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 b027f870d67e035e6476dc0a84bc3f5dfddbf99f. 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

Yep, I agree with @AlexeySachkov on behalf of sanitizer reviewer.

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.

Pennycook

Specification looks good, but a meta-comment: if we're going to have to end up exposing all the Level Zero kernel queries, it might be a good idea to just provide a single way to query things using the Level Zero enums and/or backend interop.

aelovikov-intel · 2025-02-10T15:00:38Z

it might be a good idea to just provide a single way to query things using the Level Zero enums and/or backend interop

Why don't we push for that right now?

This is a draft for the extension and its implementation.

This needs an update, I assume

…el_queries

kurapov-peter commented Jan 29, 2025

View reviewed changes

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

kurapov-peter commented Feb 3, 2025

View reviewed changes

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

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

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

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

Pennycook approved these changes Feb 5, 2025

View reviewed changes

kurapov-peter temporarily deployed to WindowsCILock February 6, 2025 12:51 — with GitHub Actions Inactive

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

kurapov-peter mentioned this pull request Feb 10, 2025

[L0 SDK][Win] Improve installation user experience intel/intel-xpu-backend-for-triton#3386

Open

kurapov-peter temporarily deployed to WindowsCILock February 12, 2025 14:00 — with GitHub Actions Inactive

kurapov-peter had a problem deploying to WindowsCILock February 12, 2025 14:48 — with GitHub Actions Failure

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

kurapov-peter temporarily deployed to WindowsCILock February 12, 2025 18:13 — with GitHub Actions Inactive

kurapov-peter requested review from a team as code owners March 5, 2025 15:01

kurapov-peter requested review from reble and frasercrmck March 5, 2025 15:01

Merge remote-tracking branch 'upstream/sycl' into sycl_ext_intel_kern…

7402507

…el_queries

kurapov-peter force-pushed the sycl_ext_intel_kernel_queries branch from ed41dfc to 7402507 Compare March 5, 2025 15:05

kurapov-peter removed request for a team, frasercrmck and reble March 5, 2025 15:06

kurapov-peter temporarily deployed to WindowsCILock March 5, 2025 15:06 — with GitHub Actions Inactive

kurapov-peter temporarily deployed to WindowsCILock March 5, 2025 16:09 — with GitHub Actions Inactive

Merge remote-tracking branch 'upstream/sycl' into sycl_ext_intel_kern…

d918044

…el_queries

kurapov-peter temporarily deployed to WindowsCILock March 6, 2025 13:55 — with GitHub Actions Inactive

kurapov-peter temporarily deployed to WindowsCILock March 6, 2025 15:04 — with GitHub Actions Inactive

sarnex merged commit 65498a6 into intel:sycl Mar 6, 2025
33 checks passed

bader mentioned this pull request Mar 11, 2025

[SYCL] Extend -fsycl-device-obj to dump assembly #17390

Merged

kurapov-peter mentioned this pull request Mar 23, 2025

[SYCL] Add spill_memory_size standalone test #17594

Closed

Add sycl ext intel kernel queries extension #16834

Add sycl ext intel kernel queries extension #16834

Uh oh!

Conversation

kurapov-peter commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlueck commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurapov-peter commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurapov-peter commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlueck commented Feb 4, 2025

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

kurapov-peter commented Jan 29, 2025 •

edited

Loading

gmlueck commented Jan 31, 2025 •

edited

Loading