Skip to content

Conversation

@slawekptak
Copy link
Contributor

@slawekptak slawekptak commented Sep 5, 2025

This change makes the commands submitted to the scheduler unconditionally associated with an event (for both event and event-less APIs), for kernel submission. For other commands, the event can be skipped if the scheduler bypass condition is true (the scheduler bypass itself is not supported for commands other than the kernel submission), if the queue supports discarding the events and the event was not requested.

The reason for this change is, that some commands might already be scheduled and waiting for the submission, so all the kernel submission commands subsequently submitted to the scheduler must return an event, which is then used to order the commands by the in-order type queue and avoid scheduler-bypass flow in such a case.

On the other hand, if the scheduler bypass condition is true for a command other than the kernel submission, the event dependencies are safe for scheduler bypass, so the event is not needed.

Commands submitted to the scheduler need to unconditionally
be associated with an event (for both event and event-less APIs).
This is because some commands might already be scheduled
and waiting for the submission, and a newly submitted command
need to return an event which can be used by the in-order type
queue to properly order the commands.
@jbrodman
Copy link
Contributor

jbrodman commented Sep 5, 2025

Why is an event needed to order an in-order queue? Shouldn't the queue itself just be handling that?

@slawekptak
Copy link
Contributor Author

Why is an event needed to order an in-order queue? Shouldn't the queue itself just be handling that?

The queue on the SYCL level uses those events to do the ordering for the cases where commands are not submitted to UR yet. Ultimately, if user didn't request an event, it is only used internally for the ordering, and not returned. Does it answer the question, or you had something else in mind?

@jbrodman
Copy link
Contributor

jbrodman commented Sep 5, 2025

I'm still not sure I'm seeing why this is needed. Why isn't something submitted? Is this some host task thing?

We need to be creating fewer (ideally zero) events unless they are explicitly asked for. Event creation hurts performance.

@slawekptak
Copy link
Contributor Author

slawekptak commented Sep 5, 2025

Yes, this is about host tasks. As long as they are implemented on the SYCL layer, we need to synchronize here.

By the way - the scheduler-bypass fast path doesn't create events if not needed and this patch doesn't change this. It only affects the scheduler-based path. Currently, we use an event of the previous command to check, if a scheduler-bypass path can be used for the current submission. If no event generated, we assume no dependency, which sometimes is not true. This patch is a fix for those scenarios. To change this entire mechanism, may be beyond the scope of this patch, but for sure we can discuss it.

EDIT: A small clarification - the performance of scheduler-bypass should not be affected when subsequent kernels are submitted using that path, but there might be scenarios with mixed scheduler-bypass and scheduler-based submissions, where in some cases the scheduler-bypass might need an additional event check (event from the previous scheduler-based submission). This check is needed for the ordering to work.

@slawekptak
Copy link
Contributor Author

@sergey-semenov Could you please take a look? Thanks

@slawekptak slawekptak marked this pull request as draft September 17, 2025 09:51
@slawekptak slawekptak marked this pull request as ready for review October 21, 2025 13:54
@slawekptak
Copy link
Contributor Author

@intel/llvm-gatekeepers Please consider merging.

@sarnex sarnex merged commit a276b6e into intel:sycl Nov 3, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants