Skip to content

[SYCL] Remove redundant creation of discarded events#21571

Open
sergey-semenov wants to merge 4 commits intointel:syclfrom
sergey-semenov:dropdiscardedevents
Open

[SYCL] Remove redundant creation of discarded events#21571
sergey-semenov wants to merge 4 commits intointel:syclfrom
sergey-semenov:dropdiscardedevents

Conversation

@sergey-semenov
Copy link
Contributor

Creation of discarded events introduces significant overhead, which is why the corresponding extension has been dropped in favor of sycl_ext_oneapi_enqueue_functions.

Creation of discarded events introduces significant overhead, which is
why the corresponding extension has been dropped in favor of
sycl_ext_oneapi_enqueue_functions.
@sergey-semenov sergey-semenov requested a review from a team as a code owner March 19, 2026 17:33
@sergey-semenov sergey-semenov added the performance Performance related issues label Mar 19, 2026
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 removes redundant creation of “discarded” sycl::event objects (previously used when callers did not need a usable event), reducing overhead and aligning the implementation with sycl_ext_oneapi_enqueue_functions.

Changes:

  • Refactor USM memory ops in queue_impl to return EventImplPtr (nullable when the caller doesn’t need an event), and wrap into sycl::event at the queue API boundary.
  • Update internal submission helpers to avoid manufacturing discarded events for in-order submissions.
  • Remove event_impl::create_discarded_event().

Reviewed changes

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

File Description
sycl/source/queue.cpp Adds a small helper to wrap EventImplPtr into sycl::event for public queue memory-op APIs.
sycl/source/detail/queue_impl.hpp Changes memory-op and helper submission APIs to return EventImplPtr instead of sycl::event.
sycl/source/detail/queue_impl.cpp Implements nullable EventImplPtr returns and removes discarded-event creation in helper submission paths.
sycl/source/detail/event_impl.hpp Removes the discarded-event factory function.

Comment on lines 563 to +567
/// \param CallerNeedsEvent specifies if the caller expects a usable event.
/// \return an event representing fill operation.
event memset(void *Ptr, int Value, size_t Count,
const std::vector<event> &DepEvents, bool CallerNeedsEvent);
EventImplPtr memset(void *Ptr, int Value, size_t Count,
const std::vector<event> &DepEvents,
bool CallerNeedsEvent);
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The doxygen for memset/memcpy/mem_advise still reads as if a (non-null) event is always returned, but these functions now return EventImplPtr and can return nullptr when CallerNeedsEvent is false. Please update the "\return"/API comment to explicitly document the nullptr behavior so internal callers know they must handle it.

Copilot uses AI. Check for mistakes.
Comment on lines +385 to +386
// TODO Avoid event creation in the first place if it's not needed
return CallerNeedsEvent ? EventImpl : nullptr;
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

In queue_impl::submit_impl(), the new "return CallerNeedsEvent ? EventImpl : nullptr" only applies to the main path; the earlier noLastEventPath branch returns finalizeHandlerInOrderNoEventsUnlocked(Handler) directly, which can still yield a non-null EventImplPtr even when CallerNeedsEvent=false. This can violate the CallerNeedsEvent contract and can trip the assert in submitWithHandler() (ReturnEvent==false but submit_impl returns a non-null pointer). Consider routing the noLastEventPath through the same tail return logic (or at least applying the same CallerNeedsEvent filtering there).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants