[NFCI][SYCL] Copy `handler::MQueue` to `handler_impl` #18830

aelovikov-intel · 2025-06-05T15:33:32Z

Ideally, it should be just moved there, but doing the whole move in one PR would be harder to review as it would require multiple internal API changes to accept queue_impl by raw ptr/ref instead of std::shared_ptr<queue_impl> value/ref. As such, this PR does the following:

Store std::variant of queue/graph instead of just graph as two are mutually exclusive
Store them by reference (through std::reference_wrapper as 'std::variant' can't store raw references directly) instead of std::shared_ptr<graph_impl>. The reason for that is that the object handler is submitted to is guaranteed to be alive for the whole lifetime of the handler and we don't need to extend the lifetime of queue/graph. Corresponding changes for handler::MQueue have been implemented recently already (although via std::shared_ptr<queue_impl> & and not raw reference) but are limited to prevew-only. Do the same for graphs here, essentially.
Update all uses of the old handler_impl::MGraph data member as it needs to go through new getters accessing the std::variant described above.
Update some of the direct usages of handler::MQueue that don't require APIs refactoring elsewhere. The remaining uses are left to the subsequent PR(s).
We'll probably need to keep the handler::MQueue initialized properly even after the move is complete and all internal SYCL RT accesses are through handler_impl as some direct handler::MQueue accesses might have been inlined into the users' applications (I'd be especially worried about reductions).

Ideally, it should be just moved there, but doing the whole move in one PR would be harder to review as it would require multiple internal API changes to accept `queue_impl` by raw ptr/ref instead of `std::shared_ptr<queue_impl>` value/ref. As such, this PR does the following: * Store `std::variant` of queue/graph instead of just graph as two are mutually exclusive * Store them by reference (through `std::reference_wrapper` as 'std::variant' can't store raw references directly) instead of `std::shared_ptr<graph_impl>`. The reason for that is that the object `handler` is submitted to is guaranteed to be alive for the whole lifetime of the `handler` and we don't need to extend the lifetime of queue/graph. Corresponding changes for `handler::MQueue` have been implemented recently already (although via `std::shared_ptr<queue_impl> &` and not raw reference) but are limited to prevew-only. Do the same for graphs here, essentially. * Update all uses of the old `handler_impl::MGraph` data member as it needs to go through new getters accessing the `std::variant` described above. * Update some of the direct usages of `handler::MQueue` that don't require APIs refactoring elsewhere. The remaining uses are left to the subsequent PR(s). * We'll probably need to keep the `handler::MQueue` initialized properly even after the move is complete and all internal SYCL RT accesses are through `handler_impl` as some direct `handler::MQueue` accesses might have been inlined into the users' applications (I'd be especially worried about reductions).

aelovikov-intel · 2025-06-05T15:34:17Z

sycl/source/accessor.cpp

@@ -14,15 +14,6 @@
 namespace sycl {
 inline namespace _V1 {
 namespace detail {
-device getDeviceFromHandler(handler &cgh) {


This is exported, I've moved it to handler.cpp.

aelovikov-intel · 2025-06-05T17:10:37Z

Just for a bit of extra context, #18767 is a draft/wip/poc showing something close to the targeted state.

maarquitos14

Overall, it looks good to me. Just a couple of nits, and a couple of questions:

What is the motivation behind these changes?
Is this not breaking ABI?

maarquitos14 · 2025-06-06T08:20:01Z

sycl/source/handler.cpp

@@ -43,13 +43,15 @@ inline namespace _V1 {

 namespace detail {

+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
+// TODO: Check if two ABI exports below are still necessary.


Should we address this here or in the future?

IMO, during the ABI breaking window would be most effective, hence the preview guards around the comment.

The question was not very precise, sorry. Let me clarify: should we try to address this within the guards already, so it's ready and tested for the next ABI breaking window?

I don't think it's necessary and I'd rather focus on more complicated things (like changes in this PR) first. Having single cleanup for trivial cases like this later, after I do most of the hard refactoring, is my current plan.

maarquitos14 · 2025-06-06T08:24:26Z

sycl/source/handler.cpp

  range<2> ItemLimit = Dev.get_info<info::device::max_work_item_sizes<2>>() *
                       Dev.get_info<info::device::max_compute_units>();
  return id<2>{std::min(ItemLimit[0], Height), std::min(ItemLimit[1], Width)};
 }

+// TODO: do we need this still?


Should we address this here or in the future?

aelovikov-intel · 2025-06-06T14:26:22Z

What is the motivation behind these changes?

Looks like a much clear design to me. If properties are mutually exclusive, then it should be an std::variant. Having all the data in impl is also just the right thing to do. Also, the ownership bit for the Graph from the description comment.

Is this not breaking ABI?

Don't see how would it...

aelovikov-intel · 2025-06-06T14:55:08Z

@intel/sycl-graphs-reviewers , could you take a look before EOD in your TZ? I was hoping I'd be able to proceed with subsequent PRs during my working hours today.

maarquitos14 · 2025-06-06T15:01:00Z

Looks like a much clear design to me. If properties are mutually exclusive, then it should be an std::variant. Having all the data in impl is also just the right thing to do. Also, the ownership bit for the Graph from the description comment.

I agree. Just wanted to know if this was also expected to have any performance implications, as we are doing some efforts on that direction.

Is this not breaking ABI?

Don't see how would it...

Sorry, I mixed up the title of this PR with the draft PR you linked, so in my head this was already effectively moving, not copying.

aelovikov-intel · 2025-06-06T15:10:21Z

Just wanted to know if this was also expected to have any performance implications, as we are doing some efforts on that direction.

We've had multiple PRs that were eliminating unnecessary shared_ptr copies on hot paths. With my refactoring I'm trying to approach this issue on the design/architecture level by using raw ptr/ref by default and only doing explicit ref count increments via shared_from_this. In that sense I believe it should be neutral/slightly positive. The only extra cost I see is two memory accesses (via extra impl indirection) vs one, but a) it's very cheap; b) that's the price of the pImpl idiom and ABI compatibility well justifies that kind of overhead.

EwanC

One meaningful comment about handler_impl(ext::oneapi::experimental::detail::graph_impl &Graph) usage with __INTEL_PREVIEW_BREAKING_CHANGES. All other comments are superficial nitpicks.

sycl/source/handler.cpp

sycl/source/detail/handler_impl.hpp

EwanC · 2025-06-06T15:49:09Z

sycl/source/detail/handler_impl.hpp

+  handler_impl(ext::oneapi::experimental::detail::graph_impl &Graph)
+      : MQueueOrGraph{Graph} {}


I don't think we have any %if preview-breaking-changes-supported Graph E2E tests to catch this. When we call this constructor with under __INTEL_PREVIEW_BREAKING_CHANGES
https://github.com/intel/llvm/blob/sycl/sycl/source/detail/graph_impl.cpp#L507 it will pass a std::shared_ptr<ext::oneapi::experimental::detail::graph_impl> from shared_from_this() can that compile after this change?

source/detail is compiled unconditionally in two modes, because we provide two variants of libsycl.so/libsycl-preview.so. All existing E2E tests are run in preview mode in this nightly task as well:

llvm/.github/workflows/sycl-nightly.yml

Lines 111 to 115 in 57cc5fe

- name: Preview mode on SPR/PVC

runner: '["Linux", "pvc"]'

image_options: -u 1001 --device=/dev/dri -v /dev/dri/by-path:/dev/dri/by-path --privileged --cap-add SYS_ADMIN

target_devices: level_zero:gpu

extra_lit_opts: --param test-preview-mode=True

That said, I'll take another look locally to be 100% sure, but I wouldn't expect any failures here.

cool, in that case I've approved the PR, but if you could double check locally that would be great.

This is already modified as part of this PR - see changes in the above file (graph_impl.cpp).

Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748

Initially started in #18830 Subsequent PRs before this final one: #18794 #18834 #18748

aelovikov-intel requested review from a team as code owners June 5, 2025 15:33

aelovikov-intel requested review from Bensuo and maarquitos14 June 5, 2025 15:33

aelovikov-intel had a problem deploying to WindowsCILock June 5, 2025 15:33 — with GitHub Actions Error

aelovikov-intel force-pushed the copy-MQueue branch from 3ea60a9 to 5281e90 Compare June 5, 2025 15:39

aelovikov-intel temporarily deployed to WindowsCILock June 5, 2025 15:40 — with GitHub Actions Inactive

aelovikov-intel commented Jun 5, 2025

View reviewed changes

aelovikov-intel mentioned this pull request Jun 5, 2025

[SYCL] Pass handler & instead of queue across ABI for reduction utils #18834

Merged

aelovikov-intel temporarily deployed to WindowsCILock June 5, 2025 19:56 — with GitHub Actions Inactive

maarquitos14 approved these changes Jun 6, 2025

View reviewed changes

EwanC reviewed Jun 6, 2025

View reviewed changes

EwanC approved these changes Jun 6, 2025

View reviewed changes

Naming convention fixes

b597294

aelovikov-intel temporarily deployed to WindowsCILock June 6, 2025 16:07 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 6, 2025 16:37 — with GitHub Actions Inactive

aelovikov-intel merged commit 0dff0ff into intel:sycl Jun 6, 2025
24 checks passed

aelovikov-intel deleted the copy-MQueue branch June 6, 2025 16:58

aelovikov-intel mentioned this pull request Jun 6, 2025

[NFCI][SYCL] Complete handler::MQueue->handler_impl move #18767

Merged

aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jun 6, 2025

[NFCI][SYCL] Complete handler::MQueue->handler_impl move

fd4300f

Initially started in intel#18830 Subsequent PRs before this final one: intel#18794 intel#18834 intel#18748

aelovikov-intel added a commit that referenced this pull request Jun 9, 2025

[NFCI][SYCL] Complete handler::MQueue->handler_impl move (#18767)

9457ac2

Initially started in #18830 Subsequent PRs before this final one: #18794 #18834 #18748

		handler_impl(ext::oneapi::experimental::detail::graph_impl &Graph)
		: MQueueOrGraph{Graph} {}

	- name: Preview mode on SPR/PVC
	runner: '["Linux", "pvc"]'
	image_options: -u 1001 --device=/dev/dri -v /dev/dri/by-path:/dev/dri/by-path --privileged --cap-add SYS_ADMIN
	target_devices: level_zero:gpu
	extra_lit_opts: --param test-preview-mode=True

[NFCI][SYCL] Copy handler::MQueue to handler_impl #18830

[NFCI][SYCL] Copy handler::MQueue to handler_impl #18830

Uh oh!

Conversation

aelovikov-intel commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Jun 5, 2025

Uh oh!

maarquitos14 left a comment

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!

maarquitos14 Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aelovikov-intel commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel commented Jun 6, 2025

Uh oh!

maarquitos14 commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel commented Jun 6, 2025

Uh oh!

EwanC left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

[NFCI][SYCL] Copy `handler::MQueue` to `handler_impl` #18830

[NFCI][SYCL] Copy `handler::MQueue` to `handler_impl` #18830

maarquitos14 Jun 6, 2025 •

edited

Loading

aelovikov-intel commented Jun 6, 2025 •

edited

Loading

maarquitos14 commented Jun 6, 2025 •

edited

Loading