From 8ba9fb4762c9d1dc3139c2d7dd25a6dda6a610e3 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Mon, 16 Jun 2025 19:02:29 +0000 Subject: [PATCH 1/4] [SYCL] Fix handling of discarded events in preview lib The queue implementation assumed that handler.finalize() will return either a valid (non-discarded) event or nullptr (that's why parseEvent is nop in preview mode). However, that was not always true. It was possible for scheduler to only mark an event as discarded after handler.finalize() completed (during actual command execution). This resulted in discarded event being stored in LastEventPtr in the queue and calls to wait() on that discarded event (which is not allowed) in MT scenarios. Fix this, by modyfing addCG() to mark the event as discarded immediately. This allows handler to return nullptr for preview mode. --- sycl/source/detail/scheduler/commands.cpp | 19 +++++++------------ sycl/source/detail/scheduler/scheduler.cpp | 3 +++ sycl/source/detail/scheduler/scheduler.hpp | 3 ++- sycl/source/handler.cpp | 11 +++++++++-- 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp index 946ad402de46a..020e5b0743c22 100644 --- a/sycl/source/detail/scheduler/commands.cpp +++ b/sycl/source/detail/scheduler/commands.cpp @@ -3132,22 +3132,17 @@ ur_result_t ExecCGCommand::enqueueImpQueue() { auto RawEvents = getUrEvents(EventImpls); flushCrossQueueDeps(EventImpls, MWorkerQueue); - // We can omit creating a UR event and create a "discarded" event if the - // command has been explicitly marked as not needing an event, e.g. if the - // user did not ask for one, and there are no requirements. - bool DiscardUrEvent = MQueue && !MEventNeeded && - MQueue->supportsDiscardingPiEvents() && - MCommandGroup->getRequirements().size() == 0; - ur_event_handle_t UREvent = nullptr; - ur_event_handle_t *Event = DiscardUrEvent ? nullptr : &UREvent; - detail::event_impl *EventImpl = DiscardUrEvent ? nullptr : MEvent.get(); + ur_event_handle_t *Event = !MEventNeeded ? nullptr : &UREvent; + detail::event_impl *EventImpl = !MEventNeeded ? nullptr : MEvent.get(); auto SetEventHandleOrDiscard = [&]() { - if (Event) + if (!MEventNeeded) { + assert(MEvent->isDiscarded()); + } else { + assert(!MEvent->isDiscarded()); MEvent->setHandle(*Event); - else - MEvent->setStateDiscarded(); + } }; switch (MCommandGroup->getType()) { diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index b84500dc65a96..dceaaec3fa37b 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -133,6 +133,9 @@ EventImplPtr Scheduler::addCG( } NewEvent = NewCmd->getEvent(); NewEvent->setSubmissionTime(); + if (!EventNeeded) { + NewEvent->setStateDiscarded(); + } } enqueueCommandForCG(NewEvent, AuxiliaryCmds); diff --git a/sycl/source/detail/scheduler/scheduler.hpp b/sycl/source/detail/scheduler/scheduler.hpp index a43e7546e9a6b..5ac668878bb9b 100644 --- a/sycl/source/detail/scheduler/scheduler.hpp +++ b/sycl/source/detail/scheduler/scheduler.hpp @@ -376,7 +376,8 @@ class Scheduler { /// directly to the queue. /// \param Dependencies Optional list of dependency /// sync points when enqueuing to a command buffer. - /// \return an event object to wait on for command group completion. + /// \return an event object to wait on for command group completion. It can + /// be a discarded event. EventImplPtr addCG( std::unique_ptr CommandGroup, const QueueImplPtr &Queue, bool EventNeeded, ur_exp_command_buffer_handle_t CommandBuffer = nullptr, diff --git a/sycl/source/handler.cpp b/sycl/source/handler.cpp index a28f21c408f80..18fdb457ec91c 100644 --- a/sycl/source/handler.cpp +++ b/sycl/source/handler.cpp @@ -895,11 +895,18 @@ event handler::finalize() { #endif } + bool DiscardEvent = !impl->MEventNeeded && Queue && + Queue->supportsDiscardingPiEvents() && + CommandGroup->getRequirements().size() == 0; + detail::EventImplPtr Event = detail::Scheduler::getInstance().addCG( - std::move(CommandGroup), Queue->shared_from_this(), impl->MEventNeeded); + std::move(CommandGroup), Queue->shared_from_this(), !DiscardEvent); #ifdef __INTEL_PREVIEW_BREAKING_CHANGES - MLastEvent = Event; + // For preview mode, handler.finalize() is expected to return nullptr + // if the event is discarded. + MLastEvent = DiscardEvent ? nullptr : Event; + assert(MLastEvent || !MLastEvent->isDiscarded()); #else MLastEvent = detail::createSyclObjFromImpl(Event); #endif From ffd33aa14bf0fa9cb2503684385c0a2860d5e6ae Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Mon, 16 Jun 2025 22:07:55 +0000 Subject: [PATCH 2/4] add comment --- sycl/source/detail/scheduler/scheduler.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index dceaaec3fa37b..d4f6fb6b93515 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -133,6 +133,11 @@ EventImplPtr Scheduler::addCG( } NewEvent = NewCmd->getEvent(); NewEvent->setSubmissionTime(); + + // This is the last moment we can mark the event as discarded. + // Doing this during command execution would could to incorrect + // event handling (as event would change it's state from non-discarded + // to discarded). if (!EventNeeded) { NewEvent->setStateDiscarded(); } From 0555ed3a8d56d5953b465e78c1afad10afb57167 Mon Sep 17 00:00:00 2001 From: Igor Chorazewicz Date: Tue, 17 Jun 2025 00:32:30 +0000 Subject: [PATCH 3/4] Move assert to queue --- sycl/include/sycl/handler.hpp | 3 +++ sycl/source/detail/queue_impl.hpp | 6 +++++- sycl/source/handler.cpp | 3 --- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/sycl/include/sycl/handler.hpp b/sycl/include/sycl/handler.hpp index 7c50701c5af0e..3cf26b5432298 100644 --- a/sycl/include/sycl/handler.hpp +++ b/sycl/include/sycl/handler.hpp @@ -571,6 +571,9 @@ class __SYCL_EXPORT handler { /// object destruction. /// /// \return a SYCL event object representing the command group + /// + /// Note: in preview mode, handler.finalize() is expected to return + /// nullptr if the event is not needed (discarded). #ifdef __INTEL_PREVIEW_BREAKING_CHANGES detail::EventImplPtr finalize(); #else diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 34982e99ead92..d897bfc91b5ab 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -709,7 +709,11 @@ class queue_impl : public std::enable_shared_from_this { } #ifdef __INTEL_PREVIEW_BREAKING_CHANGES -#define parseEvent(arg) (arg) + inline const detail::EventImplPtr & + parseEvent(const detail::EventImplPtr &Event) { + assert(!Event || !Event->isDiscarded()); + return Event; + } #else inline detail::EventImplPtr parseEvent(const event &Event) { const detail::EventImplPtr &EventImpl = getSyclObjImpl(Event); diff --git a/sycl/source/handler.cpp b/sycl/source/handler.cpp index 18fdb457ec91c..6377d457ae063 100644 --- a/sycl/source/handler.cpp +++ b/sycl/source/handler.cpp @@ -903,10 +903,7 @@ event handler::finalize() { std::move(CommandGroup), Queue->shared_from_this(), !DiscardEvent); #ifdef __INTEL_PREVIEW_BREAKING_CHANGES - // For preview mode, handler.finalize() is expected to return nullptr - // if the event is discarded. MLastEvent = DiscardEvent ? nullptr : Event; - assert(MLastEvent || !MLastEvent->isDiscarded()); #else MLastEvent = detail::createSyclObjFromImpl(Event); #endif From 45faf365b02d505670bbfe6c371080b30edf98e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20Chor=C4=85=C5=BCewicz?= Date: Mon, 16 Jun 2025 16:41:39 -0700 Subject: [PATCH 4/4] Update sycl/source/detail/scheduler/scheduler.cpp Co-authored-by: aelovikov-intel --- sycl/source/detail/scheduler/scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/scheduler/scheduler.cpp b/sycl/source/detail/scheduler/scheduler.cpp index d4f6fb6b93515..fd5b2af0b338b 100644 --- a/sycl/source/detail/scheduler/scheduler.cpp +++ b/sycl/source/detail/scheduler/scheduler.cpp @@ -135,7 +135,7 @@ EventImplPtr Scheduler::addCG( NewEvent->setSubmissionTime(); // This is the last moment we can mark the event as discarded. - // Doing this during command execution would could to incorrect + // Doing this during command execution would lead to incorrect // event handling (as event would change it's state from non-discarded // to discarded). if (!EventNeeded) {