Skip to content

[SYCL] Add no-handler barrier submission path for ext_oneapi_submit_barrier #21099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
uditagarwal97 wants to merge 5 commits into
base: sycl
Choose a base branch
from
Open

[SYCL] Add no-handler barrier submission path for ext_oneapi_submit_barrier #21099

uditagarwal97 wants to merge 5 commits into from
+98 −11

Conversation

@uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Jan 22, 2026

No description provided.

@uditagarwal97 uditagarwal97 self-assigned this Jan 22, 2026
Copilot AI reviewed Jan 22, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a no-handler barrier submission path for ext_oneapi_submit_barrier to optimize barrier operations by bypassing the handler layer when command graphs are not involved.

Changes:

  • Modified ext_oneapi_submit_barrier to use a direct submission path when no command graph is present
  • Added submit_barrier_direct_impl and submit_barrier_direct_with_event methods to queue_impl for direct UR barrier submission
  • Updated test expectations to reflect the new behavior where non-empty event lists now trigger UR calls

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
sycl/source/queue.cpp Routes barrier submissions to direct path when command graphs aren't used
sycl/source/detail/queue_impl.hpp Declares new direct barrier submission methods
sycl/source/detail/queue_impl.cpp Implements direct barrier submission with UR event handling and dependency management
sycl/unittests/Extensions/ExtOneapiBarrierOpt.cpp Updates test to verify UR calls are made for non-empty event dependencies

@uditagarwal97 uditagarwal97 changed the title [SYCL] Add no-handler barrier submission path for ext_oneapi_submit_barrier [SYCL] Add no-handler barrier submission path for ext_oneapi_submit_barrier Jan 22, 2026
slawekptak
slawekptak reviewed Jan 22, 2026
View reviewed changes
@uditagarwal97 uditagarwal97 marked this pull request as ready for review January 22, 2026 19:52
@uditagarwal97 uditagarwal97 requested a review from a team as a code owner January 22, 2026 19:52
sergey-semenov
sergey-semenov requested changes Jan 26, 2026
View reviewed changes
sycl/source/detail/queue_impl.cpp Outdated
Comment on lines 486 to 524
// This is equivalent to handler::depends_on() and is required
// to handle events associated with graphs correctly.
if (Event->isHost()) {
detail::registerEventDependency(
Event, CGData.MEvents, this, this->getContextImpl(),
this->getDeviceImpl(), getCommandGraph().get(),
CGType::BarrierWaitlist);
continue;
}

// Throwaway events created with empty constructor will not have a context
// (which is set lazily) calling getContextImpl() would set that
// context, which we wish to avoid as it is expensive.
// Skip NOP events also.
if (Event->isDefaultConstructed() || Event->isNOP())
continue;

// If command has not been enqueued then we have to enqueue it.
// It may happen if async enqueue in a host task is involved.
// Interoperability events are special cases and they are not enqueued, as
// they don't have an associated queue and command.
if (!Event->isInterop() && !Event->isEnqueued()) {
if (!Event->getCommand() || !Event->getCommand()->producesPiEvent())
continue;
std::vector<Command *> AuxCmds;
Scheduler::getInstance().enqueueCommandForCG(*Event, AuxCmds, BLOCKING);
}

// Do not add redundant event dependencies for in-order queues.
// At this stage dependency is definitely UR task and need to check if
// current one is a host task. In this case we should not skip UR event
// due to different sync mechanisms for different task types on in-order
// queue. If the resulting event is supposed to have a specific event
// mode, redundant events may still differ from the resulting event, so
// they are kept.
if (Event->getWorkerQueue().get() == this && this->isInOrder())
continue;

RetUrEvents.push_back(Event->getHandle());
Copy link
Contributor

@sergey-semenov sergey-semenov Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the code from Command::getUrEventsBlocking. Can we move it out into a separate function?

Copy link
Contributor

@sergey-semenov sergey-semenov Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the duplication is still there.

Copy link
Contributor Author

@uditagarwal97 uditagarwal97 Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function wasn't really a duplicate of Command::getUrEventsBlocking since we were also registering host event dependencies in this function. However, I have now split that part out and re-used Command::getUrEventsBlocking, let me know if that looks better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slawekptak slawekptak slawekptak left review comments

Copilot code review Copilot Copilot left review comments

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov

Requested changes must be addressed to merge this pull request.

Assignees

@uditagarwal97 uditagarwal97

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@uditagarwal97 @slawekptak @sergey-semenov