[SYCL] Deprecate discard_events queue prop, make impl no-op #18059

JackAKirk · 2025-04-16T14:15:09Z

Deprecates sycl_ext_oneapi_discard_queue_events in favour of the sycl_ext_oneapi_enqueue_functions extension.
In order to simplify/allow further simplifications to the handler/scheduler code the implementation of the discard_events queue property is also made a no-op immediately.

The main consequence of this is that users of discard_events will have to ensure they use the new submit_without_event / nd_launch etc APIs from https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_enqueue_functions.asciidoc
sycl::event is very lightweight already in the level_zero v2 adapter and discard_events already doesn't have a very big performance advantage for this case. Therefore the biggest consequence of this PR from a performance point of view is in the cuda/hip backends. Users of these backends need to be aware of the performance implications of making the discard_events no-op, and should ensure that they switch to using the sycl_ext_oneapi_enqueue_functions.

This PR leaves all other functionality working correctly.

Signed-off-by: JackAKirk <[email protected]>

jbrodman · 2025-04-16T15:57:07Z

sycl/include/sycl/properties/queue_properties.def

@@ -9,8 +9,6 @@
 __SYCL_DATA_LESS_PROP(property::queue, in_order, InOrder)
 __SYCL_DATA_LESS_PROP(property::queue, enable_profiling, QueueEnableProfiling)

-__SYCL_DATA_LESS_PROP(ext::oneapi::property::queue, discard_events,


So I think we'd like to still have the property exist to avoid a breaking change, but when it's seen, it's just ignored.

OK sure, I'll fix this. Thanks

I've now kept the property but made it a no-op. I've updated the PR description accordingly.
I've kept all discard events tests other than those which test that the underlying sycl::event is nullptr, this this PR makes those tests obsolete.
I've also updated to docs to mark sycl_ext_oneapi_discard_queue_events deprecated.

igchor · 2025-04-16T16:50:58Z

@JackAKirk can you also cleanup event_impl? I still see some references to discard events there. Or is this still needed for some paths?

JackAKirk · 2025-04-17T08:19:13Z

@JackAKirk can you also cleanup event_impl? I still see some references to discard events there. Or is this still needed for some paths?

When we use the submit_without_event path the handler_impl::MEventNeeded is still set false which leads to event_impl::isDiscarded being called which is currently used in queue::ext_oneapi_empty. So I think most of the "discard" in event_impl is still needed until events are fully removed from the submit_without_event path, which sounds like it is going to happen since apparently there is still a 1-2% overhead due to that.

But I think you might be right that I can still remove some of the event_impl "discard" stuff already here, since maybe the "discard" in event_impl::wait is only used with the discard_events property.
I'll check this. Thanks for the idea.

Signed-off-by: JackAKirk <[email protected]>

JackAKirk · 2025-04-17T12:01:06Z

@JackAKirk can you also cleanup event_impl? I still see some references to discard events there. Or is this still needed for some paths?

I've removed one case that is definitely safe to remove. waitInternal could also potentially be cleaned up, although this depends on the way forward with host_task; in general I think that all of the internal discard event checks could be removed following on from #18018 . Apart from host_task the only other reason for keeping LastEvent is to keep this little potential optimization: https://github.com/intel/llvm/pull/18018/files#diff-f412476f167a41ca486ea41ab76bc7e861df86e8cf0542d4931b93947f41e6edL687

However as discussed this doesn't seem worth it.
You do still have a LastHostTaskEvent in the #18018 draft, so maybe it makes sense to wait until this PR is finalized/merged to finish the internal discard_event cleanup.
Could you get rid of LastHostTaskEvent completely, and instead submit a empty barrier to the native queue after the host_task is completed? This would guarantee that the host_task is synced with the in-order queue, and if users want to use multiple in order queues they can submit_with_event the host_task, and sync manually?
This would mean that sycl::queue::host_task matches exactly the semantics of cudaLaunchHostFunc(cudaStream_t, ...), see
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#host-functions-callbacks

- Make the prop no-op - add back full testing initially - deprecate discard_events extension Signed-off-by: JackAKirk <[email protected]>

Signed-off-by: JackAKirk <[email protected]>

igchor · 2025-04-17T16:53:37Z

@JackAKirk can you also cleanup event_impl? I still see some references to discard events there. Or is this still needed for some paths?

I've removed one case that is definitely safe to remove. waitInternal could also potentially be cleaned up, although this depends on the way forward with host_task; in general I think that all of the internal discard event checks could be removed following on from #18018 . Apart from host_task the only other reason for keeping LastEvent is to keep this little potential optimization: https://github.com/intel/llvm/pull/18018/files#diff-f412476f167a41ca486ea41ab76bc7e861df86e8cf0542d4931b93947f41e6edL687

However as discussed this doesn't seem worth it. You do still have a LastHostTaskEvent in the #18018 draft, so maybe it makes sense to wait until this PR is finalized/merged to finish the internal discard_event cleanup. Could you get rid of LastHostTaskEvent completely, and instead submit a empty barrier to the native queue after the host_task is completed? This would guarantee that the host_task is synced with the in-order queue, and if users want to use multiple in order queues they can submit_with_event the host_task, and sync manually? This would mean that sycl::queue::host_task matches exactly the semantics of cudaLaunchHostFunc(cudaStream_t, ...), see https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#host-functions-callbacks

I believe @KseniyaTikhomirova is working on a similar approach (having host task signal an event that we can pass to next enqueue function). However, there are some issues related to memory residence with this approach so this will probably happen as a next step.

ProGTX · 2025-04-30T13:53:58Z

Looks like there's a conflict to resolve?

Conflicts resolved. The CI seems a bit flaky, Windows testing passed after a few reruns.

Also friendly ping @intel/unified-runtime-reviewers-level-zero

igchor · 2025-04-30T15:25:34Z

sycl/source/handler.cpp

-      bool DiscardEvent = (MQueue->MDiscardEvents || !impl->MEventNeeded) &&
-                          MQueue->supportsDiscardingPiEvents();
+      bool DiscardEvent =
+          !impl->MEventNeeded && MQueue->supportsDiscardingPiEvents();


MQueue->supportsDiscardingPiEvents() doesn't seem needed anymore

Thanks, yeah I see this now: sycl_ext_oneapi_enqueue_functions.asciidoc doesn't require an in-order queue.
Fixed

Although note that this change is technically out of the scope of just removing discard_events queue property functionality: The change on this line now makes more discarding of events in the impl for submit_without_event on a sycl::queue without the in-order property. This seems to make some tests fail, despite it being allowed by the spec doc as I mention above. I will investigate.

So it turns out that currently supportsDiscardingPiEvents is still needed: see #18415
I've added it back.

vinser52 · 2025-05-05T20:19:38Z

@JackAKirk what's next with this PR?

Signed-off-by: JackAKirk <[email protected]>

Failure due to deprecation warning, duplicate test case not necessary Signed-off-by: JackAKirk <[email protected]>

Turns out EnqueueFunctions submit_without_event still creates events for oo queue, and things break if supportsDiscardingPiEvents isn't used Signed-off-by: JackAKirk <[email protected]>

JackAKirk · 2025-05-12T13:13:17Z

@JackAKirk what's next with this PR?

This should be ready to merge once tests pass.

JackAKirk · 2025-05-12T14:06:53Z

Test failures appear unrelated and occur in other PRs: see #18416

JackAKirk · 2025-05-12T15:51:23Z

@intel/llvm-gatekeepers this is ready to be merged.

Thanks

Remove discard_events queue prop

ce08eaa

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock April 16, 2025 14:15 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock April 16, 2025 14:46 — with GitHub Actions Error

Fix test fail

7541349

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock April 16, 2025 15:02 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock April 16, 2025 15:32 — with GitHub Actions Error

Fix failure

8333401

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock April 16, 2025 15:38 — with GitHub Actions Inactive

jbrodman reviewed Apr 16, 2025

View reviewed changes

JackAKirk temporarily deployed to WindowsCILock April 16, 2025 16:13 — with GitHub Actions Inactive

JackAKirk temporarily deployed to WindowsCILock April 16, 2025 16:31 — with GitHub Actions Inactive

Remove dead code in event_impl

23b2991

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock April 17, 2025 11:46 — with GitHub Actions Inactive

JackAKirk temporarily deployed to WindowsCILock April 17, 2025 12:17 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock April 17, 2025 12:30 — with GitHub Actions Failure

Add back discard_event queue prop

df73429

- Make the prop no-op - add back full testing initially - deprecate discard_events extension Signed-off-by: JackAKirk <[email protected]>

JackAKirk had a problem deploying to WindowsCILock April 17, 2025 14:02 — with GitHub Actions Error

Add back missed change

18fe7b7

Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock April 17, 2025 14:07 — with GitHub Actions Inactive

JackAKirk temporarily deployed to WindowsCILock April 17, 2025 14:44 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock April 17, 2025 14:44 — with GitHub Actions Failure

JackAKirk had a problem deploying to WindowsCILock April 17, 2025 14:56 — with GitHub Actions Failure

ProGTX temporarily deployed to WindowsCILock April 29, 2025 19:16 — with GitHub Actions Inactive

ProGTX had a problem deploying to WindowsCILock April 29, 2025 19:27 — with GitHub Actions Failure

ProGTX temporarily deployed to WindowsCILock April 29, 2025 19:27 — with GitHub Actions Inactive

ProGTX had a problem deploying to WindowsCILock April 30, 2025 05:58 — with GitHub Actions Failure

ProGTX temporarily deployed to WindowsCILock April 30, 2025 07:44 — with GitHub Actions Inactive

igchor approved these changes Apr 30, 2025

View reviewed changes

igchor mentioned this pull request May 6, 2025

[SYCL] Do not store last event for in-order queues #18277

Merged

JackAKirk added 2 commits May 12, 2025 09:59

Merge branch 'sycl' into discard-event-queue-prop-remove

e840da6

supportsDiscardingPiEvents isn't needed

bdb05ac

Signed-off-by: JackAKirk <[email protected]>

JackAKirk had a problem deploying to WindowsCILock May 12, 2025 09:00 — with GitHub Actions Error

Fix format

e24eec2

Signed-off-by: JackAKirk <[email protected]>

JackAKirk had a problem deploying to WindowsCILock May 12, 2025 09:11 — with GitHub Actions Failure

Fix mistake in merge

4af7981

Signed-off-by: JackAKirk <[email protected]>

JackAKirk had a problem deploying to WindowsCILock May 12, 2025 09:33 — with GitHub Actions Error

Remove discard_events prop from test

9b4a27b

Failure due to deprecation warning, duplicate test case not necessary Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock May 12, 2025 09:59 — with GitHub Actions Inactive

JackAKirk had a problem deploying to WindowsCILock May 12, 2025 10:21 — with GitHub Actions Failure

Add back supportsDiscardingPiEvents

4e9ecc6

Turns out EnqueueFunctions submit_without_event still creates events for oo queue, and things break if supportsDiscardingPiEvents isn't used Signed-off-by: JackAKirk <[email protected]>

JackAKirk temporarily deployed to WindowsCILock May 12, 2025 12:56 — with GitHub Actions Inactive

JackAKirk mentioned this pull request May 12, 2025

submit_without_event impl still creates events for ooo queue #18415

Open

JackAKirk temporarily deployed to WindowsCILock May 12, 2025 13:28 — with GitHub Actions Inactive

sarnex merged commit 300bba0 into intel:sycl May 12, 2025
23 of 24 checks passed

JackAKirk mentioned this pull request May 14, 2025

Remove all discard_queue_events tests for deprecated oneapi extension KhronosGroup/SYCL-CTS#1096

Merged

[SYCL] Deprecate discard_events queue prop, make impl no-op #18059

[SYCL] Deprecate discard_events queue prop, make impl no-op #18059

Uh oh!

Conversation

JackAKirk commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbrodman Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

JackAKirk Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

JackAKirk Apr 21, 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!

igchor commented Apr 16, 2025

Uh oh!

JackAKirk commented Apr 17, 2025

Uh oh!

JackAKirk commented Apr 17, 2025

Uh oh!

igchor commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ProGTX commented Apr 30, 2025

Uh oh!

igchor Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

JackAKirk May 12, 2025

Choose a reason for hiding this comment

Uh oh!

JackAKirk May 12, 2025

Choose a reason for hiding this comment

Uh oh!

JackAKirk May 12, 2025

Choose a reason for hiding this comment

Uh oh!

vinser52 commented May 5, 2025

Uh oh!

JackAKirk commented May 12, 2025

Uh oh!

JackAKirk commented May 12, 2025

Uh oh!

JackAKirk commented May 12, 2025

Uh oh!

Uh oh!

Uh oh!

JackAKirk commented Apr 16, 2025 •

edited

Loading

JackAKirk Apr 21, 2025 •

edited

Loading

igchor commented Apr 17, 2025 •

edited

Loading