Skip to content

[SYCL][NFC] Avoid unnecessary calls to event-related functions #20163

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

Merged
merged 1 commit into from
Sep 25, 2025
Merged

[SYCL][NFC] Avoid unnecessary calls to event-related functions #20163

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions sycl/source/handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,11 +502,15 @@ event handler::finalize() {
detail::queue_impl *Queue = impl->get_queue_or_null();
ext::oneapi::experimental::detail::graph_impl *Graph =
impl->get_graph_or_null();

// TODO checking the size of the events vector and avoiding the call is more
// efficient here at this point
Comment on lines +506 to +507
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these extra checks only avoid a function call, since the functions themselves already return early if the vector is empty, the benefit here seems minimal (just call-overhead savings). So I am not sure if you mean something additional by inefficiency of those calls?
Considering that, are you sure this TODO comments are actually needed? It feels like these TODOs might never be realistically addressed unless you have some ideas how to do it. If so, probably they should be just removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally have way too many function calls to do anything in the runtime. Eliminating unnecessary ones seems like a good goal. Calls into UR especially are not inlined due to the dispatching into plugins and all that, so avoiding unnecessary calls into UR definitely seems like a win.

const bool KernelFastPath =
(Queue && !Graph && !impl->MSubgraphNode && !Queue->hasCommandGraph() &&
!impl->CGData.MRequirements.size() && !MStreamStorage.size() &&
detail::Scheduler::areEventsSafeForSchedulerBypass(
impl->CGData.MEvents, Queue->getContextImpl()));
(impl->CGData.MEvents.size() == 0 ||
detail::Scheduler::areEventsSafeForSchedulerBypass(
impl->CGData.MEvents, Queue->getContextImpl())));

// Extract arguments from the kernel lambda, if required.
// Skipping this is currently limited to simple kernels on the fast path.
Expand Down Expand Up @@ -628,8 +632,13 @@ event handler::finalize() {
// the graph is not changed, then this faster path is used to submit
// kernel bypassing scheduler and avoiding CommandGroup, Command objects
// creation.
std::vector<ur_event_handle_t> RawEvents = detail::Command::getUrEvents(
impl->CGData.MEvents, impl->get_queue_or_null(), false);
std::vector<ur_event_handle_t> RawEvents;
// TODO checking the size of the events vector and avoiding the call is
// more efficient here at this point
if (impl->CGData.MEvents.size() > 0) {
RawEvents = detail::Command::getUrEvents(
impl->CGData.MEvents, impl->get_queue_or_null(), false);
}

bool DiscardEvent =
!impl->MEventNeeded && impl->get_queue().supportsDiscardingPiEvents();
Expand Down
Loading