From eff9b17a311b887c9c788b2c8defb98b35803cf8 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 25 May 2022 15:04:42 -0700 Subject: [PATCH 01/13] initial commit of events that default to the default context, but lazily Signed-off-by: Chris Perkins --- sycl/source/detail/event_impl.cpp | 59 +++++++++++++++++++++---------- sycl/source/detail/event_impl.hpp | 20 ++++++----- 2 files changed, 52 insertions(+), 27 deletions(-) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 32476dcf926a4..2683dd165c9d6 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include +#include #include #include #include @@ -31,11 +32,25 @@ namespace detail { extern xpti::trace_event_data_t *GSYCLGraphEvent; #endif -// Threat all devices that don't support interoperability as host devices to -// avoid attempts to call method get on such events. -bool event_impl::is_host() const { return MHostEvent || !MOpenCLInterop; } +// if we do not yet have a context, use the default one. +void event_impl::ensureContextInitialized() { + if (!MIsContextInitialized) { + const device &SyclDevice = default_selector().select_device(); + this->setContextImpl(detail::queue_impl::getDefaultOrNew( + detail::getSyclObjImpl(SyclDevice))); + } +} + +bool event_impl::is_host() { + // we'll need a context before we can answer is_host question. + // setting it may adjust the values of MHostEvent and MOpenCLInterop + ensureContextInitialized(); + // Treat all devices that don't support interoperability as host devices to + // avoid attempts to call method get on such events. + return MHostEvent || !MOpenCLInterop; +} -cl_event event_impl::get() const { +cl_event event_impl::get() { if (!MOpenCLInterop) { throw invalid_object_error( "This instance of event doesn't support OpenCL interoperability.", @@ -50,7 +65,7 @@ event_impl::~event_impl() { getPlugin().call(MEvent); } -void event_impl::waitInternal() const { +void event_impl::waitInternal() { if (!MHostEvent && MEvent) { getPlugin().call(1, &MEvent); return; @@ -86,14 +101,21 @@ void event_impl::setComplete() { const RT::PiEvent &event_impl::getHandleRef() const { return MEvent; } RT::PiEvent &event_impl::getHandleRef() { return MEvent; } -const ContextImplPtr &event_impl::getContextImpl() { return MContext; } +const ContextImplPtr &event_impl::getContextImpl() { + ensureContextInitialized(); + return MContext; +} -const plugin &event_impl::getPlugin() const { return MContext->getPlugin(); } +const plugin &event_impl::getPlugin() { + ensureContextInitialized(); + return MContext->getPlugin(); +} void event_impl::setContextImpl(const ContextImplPtr &Context) { MHostEvent = Context->is_host(); MOpenCLInterop = !MHostEvent; MContext = Context; + MIsContextInitialized = true; MState = HES_NotComplete; } @@ -102,9 +124,9 @@ event_impl::event_impl(HostEventState State) : MIsInitialized(false), MIsFlushed(true), MState(State) {} event_impl::event_impl(RT::PiEvent Event, const context &SyclContext) - : MEvent(Event), MContext(detail::getSyclObjImpl(SyclContext)), - MOpenCLInterop(true), MHostEvent(false), MIsFlushed(true), - MState(HES_Complete) { + : MIsContextInitialized(true), MEvent(Event), + MContext(detail::getSyclObjImpl(SyclContext)), MOpenCLInterop(true), + MHostEvent(false), MIsFlushed(true), MState(HES_Complete) { if (MContext->is_host()) { throw cl::sycl::invalid_parameter_error( @@ -190,8 +212,7 @@ void event_impl::instrumentationEpilog(void *TelemetryEvent, #endif } -void event_impl::wait( - std::shared_ptr Self) const { +void event_impl::wait(std::shared_ptr Self) { if (MState == HES_Discarded) throw sycl::exception(make_error_code(errc::invalid), "wait method cannot be used for a discarded event."); @@ -258,7 +279,7 @@ void event_impl::checkProfilingPreconditions() const { template <> cl_ulong -event_impl::get_profiling_info() const { +event_impl::get_profiling_info() { checkProfilingPreconditions(); if (!MHostEvent) { if (MEvent) @@ -275,7 +296,7 @@ event_impl::get_profiling_info() const { template <> cl_ulong -event_impl::get_profiling_info() const { +event_impl::get_profiling_info() { checkProfilingPreconditions(); if (!MHostEvent) { if (MEvent) @@ -291,8 +312,7 @@ event_impl::get_profiling_info() const { } template <> -cl_ulong -event_impl::get_profiling_info() const { +cl_ulong event_impl::get_profiling_info() { checkProfilingPreconditions(); if (!MHostEvent) { if (MEvent) @@ -306,7 +326,7 @@ event_impl::get_profiling_info() const { return MHostProfilingInfo->getEndTime(); } -template <> cl_uint event_impl::get_info() const { +template <> cl_uint event_impl::get_info() { if (!MHostEvent && MEvent) { return get_event_info::get( this->getHandleRef(), this->getPlugin()); @@ -316,7 +336,7 @@ template <> cl_uint event_impl::get_info() const { template <> info::event_command_status -event_impl::get_info() const { +event_impl::get_info() { if (MState == HES_Discarded) return info::event_command_status::ext_oneapi_unknown; @@ -339,10 +359,11 @@ void HostProfilingInfo::start() { StartTime = getTimestamp(); } void HostProfilingInfo::end() { EndTime = getTimestamp(); } -pi_native_handle event_impl::getNative() const { +pi_native_handle event_impl::getNative() { if (!MContext) { static context SyclContext; MContext = getSyclObjImpl(SyclContext); + MIsContextInitialized = true; MHostEvent = MContext->is_host(); MOpenCLInterop = !MHostEvent; } diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index 505543143ed5f..b254767df8998 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -58,19 +58,19 @@ class event_impl { /// host device to avoid attempts to call method get on such events. // /// \return true if this event is a SYCL host event. - bool is_host() const; + bool is_host(); /// Returns a valid OpenCL event interoperability handle. /// /// \return a valid instance of OpenCL cl_event. - cl_event get() const; + cl_event get(); /// Waits for the event. /// /// Self is needed in order to pass shared_ptr to Scheduler. /// /// \param Self is a pointer to this event. - void wait(std::shared_ptr Self) const; + void wait(std::shared_ptr Self); /// Waits for the event. /// @@ -101,18 +101,18 @@ class event_impl { /// \return depends on template parameter. template typename info::param_traits::return_type - get_profiling_info() const; + get_profiling_info(); /// Queries this SYCL event for information. /// /// \return depends on the information being requested. template - typename info::param_traits::return_type get_info() const; + typename info::param_traits::return_type get_info(); ~event_impl(); /// Waits for the event with respect to device type. - void waitInternal() const; + void waitInternal(); /// Marks this event as completed. void setComplete(); @@ -135,7 +135,7 @@ class event_impl { /// \return the Plugin associated with the context of this event. /// Should be called when this is not a Host Event. - const plugin &getPlugin() const; + const plugin &getPlugin(); /// Associate event with the context. /// @@ -167,7 +167,7 @@ class event_impl { /// Gets the native handle of the SYCL event. /// /// \return a native handle. - pi_native_handle getNative() const; + pi_native_handle getNative(); /// Returns vector of event dependencies. /// @@ -218,7 +218,11 @@ class event_impl { void instrumentationEpilog(void *TelementryEvent, const std::string &Name, int32_t StreamID, uint64_t IId) const; void checkProfilingPreconditions() const; + // events constructed without a context will lazily use the default context + // when needed. + void ensureContextInitialized(); mutable bool MIsInitialized = true; + mutable bool MIsContextInitialized = false; mutable RT::PiEvent MEvent = nullptr; mutable ContextImplPtr MContext; mutable bool MOpenCLInterop = false; From be21dff1a4aaf49865cec9697532839993dd8a3d Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 1 Jun 2022 14:51:50 -0700 Subject: [PATCH 02/13] event(queue) updated --- sycl/source/detail/event_impl.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 2683dd165c9d6..a44173c36a9f8 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -150,6 +150,9 @@ event_impl::event_impl(RT::PiEvent Event, const context &SyclContext) event_impl::event_impl(const QueueImplPtr &Queue) : MQueue{Queue}, MIsProfilingEnabled{Queue->is_host() || Queue->MIsProfilingEnabled} { + //CP + this->setContextImpl(Queue->getContextImplPtr()); + if (Queue->is_host()) { MState.store(HES_NotComplete); From 177cd18a23a1bf753d813b12c1cca34cdf1584d9 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 1 Jun 2022 16:49:02 -0700 Subject: [PATCH 03/13] lack of Context no longer a shorthand for lack of native event. Signed-off-by: Chris Perkins --- sycl/source/detail/helpers.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sycl/source/detail/helpers.cpp b/sycl/source/detail/helpers.cpp index 73ed626761904..6bfccfe830017 100644 --- a/sycl/source/detail/helpers.cpp +++ b/sycl/source/detail/helpers.cpp @@ -23,6 +23,8 @@ std::vector getOrWaitEvents(std::vector DepEvents, std::vector Events; for (auto SyclEvent : DepEvents) { auto SyclEventImplPtr = detail::getSyclObjImpl(SyclEvent); + if(SyclEventImplPtr->getHandleRef() == nullptr) + continue; if (SyclEventImplPtr->is_host() || SyclEventImplPtr->getContextImpl() != Context) { SyclEventImplPtr->waitInternal(); From 20486feea38db7a780c983c3848066412f96964c Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 2 Jun 2022 09:51:47 -0700 Subject: [PATCH 04/13] did someone say clang-format? --- sycl/source/detail/helpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/helpers.cpp b/sycl/source/detail/helpers.cpp index 6bfccfe830017..cddaa4d25be44 100644 --- a/sycl/source/detail/helpers.cpp +++ b/sycl/source/detail/helpers.cpp @@ -23,7 +23,7 @@ std::vector getOrWaitEvents(std::vector DepEvents, std::vector Events; for (auto SyclEvent : DepEvents) { auto SyclEventImplPtr = detail::getSyclObjImpl(SyclEvent); - if(SyclEventImplPtr->getHandleRef() == nullptr) + if (SyclEventImplPtr->getHandleRef() == nullptr) continue; if (SyclEventImplPtr->is_host() || SyclEventImplPtr->getContextImpl() != Context) { From 639879dedb90de0b6dfce28d1ae21224eb58ae13 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 2 Jun 2022 10:04:20 -0700 Subject: [PATCH 05/13] thanks clang-format. dribbling out the errors one at a time is SO great. can't wait for your next insight. --- sycl/source/detail/event_impl.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index a44173c36a9f8..80f7c3fce625f 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -150,7 +150,6 @@ event_impl::event_impl(RT::PiEvent Event, const context &SyclContext) event_impl::event_impl(const QueueImplPtr &Queue) : MQueue{Queue}, MIsProfilingEnabled{Queue->is_host() || Queue->MIsProfilingEnabled} { - //CP this->setContextImpl(Queue->getContextImplPtr()); if (Queue->is_host()) { From 934392ca44ce203072fbfa687773aa5f5bd1fe43 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 10 Jun 2022 10:18:55 -0700 Subject: [PATCH 06/13] someone has a theory --- sycl/source/detail/event_impl.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index c25a39a65c7a2..625d9642903a1 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -36,8 +36,10 @@ extern xpti::trace_event_data_t *GSYCLGraphEvent; void event_impl::ensureContextInitialized() { if (!MIsContextInitialized) { const device &SyclDevice = default_selector().select_device(); + auto tempState = MState; // setContextImpl changes MState for some reason. We don't want that. this->setContextImpl(detail::queue_impl::getDefaultOrNew( detail::getSyclObjImpl(SyclDevice))); + MState = tempState; //restore } } From 4b80dbd66edbc60eb849d8a109e6ef553a7451af Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 10 Jun 2022 16:28:17 -0700 Subject: [PATCH 07/13] setContextImpl was not only setting the context, but also changing the atomic event state. This is undesirable now that we lazily set the context to be the default one (if none is provided). It is also undesirable from a one-function-does-one-thing standpoint. I am introducing a new routine on the impl to change that event state and am adding explicit calls to it where before we were only calling event->setContex I don't like leaking impl state in this way, but it's clean. Right now all the places that are calling setContextImpl are intentionally setting up the event_impl, so having them explicitly set the state is in line with the surrounding activities. Better to have this intention explicit and known rather than hidden and ill understood. Signed-off-by: Chris Perkins --- sycl/source/detail/event_impl.cpp | 6 ++---- sycl/source/detail/event_impl.hpp | 3 +++ sycl/source/detail/queue_impl.cpp | 1 + sycl/source/detail/scheduler/commands.cpp | 11 ++++++++--- sycl/source/handler.cpp | 1 + sycl/unittests/scheduler/QueueFlushing.cpp | 2 ++ 6 files changed, 17 insertions(+), 7 deletions(-) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 625d9642903a1..603b4bb45cb3d 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -36,10 +36,8 @@ extern xpti::trace_event_data_t *GSYCLGraphEvent; void event_impl::ensureContextInitialized() { if (!MIsContextInitialized) { const device &SyclDevice = default_selector().select_device(); - auto tempState = MState; // setContextImpl changes MState for some reason. We don't want that. this->setContextImpl(detail::queue_impl::getDefaultOrNew( detail::getSyclObjImpl(SyclDevice))); - MState = tempState; //restore } } @@ -118,13 +116,13 @@ const plugin &event_impl::getPlugin() { return MContext->getPlugin(); } +void event_impl::setStateIncomplete() { MState = HES_NotComplete; } + void event_impl::setContextImpl(const ContextImplPtr &Context) { MHostEvent = Context->is_host(); MOpenCLInterop = !MHostEvent; MContext = Context; MIsContextInitialized = true; - - MState = HES_NotComplete; } event_impl::event_impl(HostEventState State) diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index 56fc5a3756d3a..f8ba2c218371e 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -146,6 +146,9 @@ class event_impl { /// @param Context is a shared pointer to an instance of valid context_impl. void setContextImpl(const ContextImplPtr &Context); + /// clear the event state + void setStateIncomplete(); + /// Returns command that is associated with the event. /// /// Scheduler mutex must be locked in read mode when this is called. diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 63e4132403ac7..a7d0ba1a2c376 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -48,6 +48,7 @@ prepareUSMEvent(const std::shared_ptr &QueueImpl, auto EventImpl = std::make_shared(QueueImpl); EventImpl->getHandleRef() = NativeEvent; EventImpl->setContextImpl(detail::getSyclObjImpl(QueueImpl->get_context())); + EventImpl->setStateIncomplete(); return detail::createSyclObjFromImpl(EventImpl); } diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp index 24404c9832e3e..ee17eca4d5c2c 100644 --- a/sycl/source/detail/scheduler/commands.cpp +++ b/sycl/source/detail/scheduler/commands.cpp @@ -400,6 +400,7 @@ Command::Command(CommandType Type, QueueImplPtr Queue) MSubmittedQueue = MQueue; MEvent->setCommand(this); MEvent->setContextImpl(MQueue->getContextImplPtr()); + MEvent->setStateIncomplete(); MEnqueueStatus = EnqueueResultT::SyclEnqueueReady; #ifdef XPTI_ENABLE_INSTRUMENTATION @@ -1092,6 +1093,7 @@ cl_int ReleaseCommand::enqueueImp() { EventImplPtr UnmapEventImpl(new event_impl(Queue)); UnmapEventImpl->setContextImpl(Queue->getContextImplPtr()); + UnmapEventImpl->setStateIncomplete(); RT::PiEvent &UnmapEvent = UnmapEventImpl->getHandleRef(); void *Src = CurAllocaIsHost @@ -1294,9 +1296,10 @@ MemCpyCommand::MemCpyCommand(Requirement SrcReq, MSrcQueue(SrcQueue), MSrcReq(std::move(SrcReq)), MSrcAllocaCmd(SrcAllocaCmd), MDstReq(std::move(DstReq)), MDstAllocaCmd(DstAllocaCmd) { - if (!MSrcQueue->is_host()) + if (!MSrcQueue->is_host()) { MEvent->setContextImpl(MSrcQueue->getContextImplPtr()); - + MEvent->setStateIncomplete(); + } emitInstrumentationDataProxy(); } @@ -1476,8 +1479,10 @@ MemCpyCommandHost::MemCpyCommandHost(Requirement SrcReq, : Command(CommandType::COPY_MEMORY, std::move(DstQueue)), MSrcQueue(SrcQueue), MSrcReq(std::move(SrcReq)), MSrcAllocaCmd(SrcAllocaCmd), MDstReq(std::move(DstReq)), MDstPtr(DstPtr) { - if (!MSrcQueue->is_host()) + if (!MSrcQueue->is_host()) { MEvent->setContextImpl(MSrcQueue->getContextImplPtr()); + MEvent->setStateIncomplete(); + } emitInstrumentationDataProxy(); } diff --git a/sycl/source/handler.cpp b/sycl/source/handler.cpp index 3e8421cf7cdbd..e76a97552c2d9 100644 --- a/sycl/source/handler.cpp +++ b/sycl/source/handler.cpp @@ -286,6 +286,7 @@ event handler::finalize() { } else { NewEvent = std::make_shared(MQueue); NewEvent->setContextImpl(MQueue->getContextImplPtr()); + NewEvent->setStateIncomplete(); OutEvent = &NewEvent->getHandleRef(); if (CL_SUCCESS != EnqueueKernel()) diff --git a/sycl/unittests/scheduler/QueueFlushing.cpp b/sycl/unittests/scheduler/QueueFlushing.cpp index 194d7c14fa59a..f216574ac02d5 100644 --- a/sycl/unittests/scheduler/QueueFlushing.cpp +++ b/sycl/unittests/scheduler/QueueFlushing.cpp @@ -208,6 +208,7 @@ TEST_F(SchedulerTest, QueueFlushing) { access::mode::read_write}; detail::EventImplPtr DepEvent{new detail::event_impl(QueueImplB)}; DepEvent->setContextImpl(QueueImplB->getContextImplPtr()); + DepEvent->setStateIncomplete(); DepEvent->getHandleRef() = reinterpret_cast(new int{}); (void)Cmd.addDep(DepEvent, ToCleanUp); MockScheduler::enqueueCommand(&Cmd, Res, detail::NON_BLOCKING); @@ -225,6 +226,7 @@ TEST_F(SchedulerTest, QueueFlushing) { detail::QueueImplPtr TempQueueImpl = detail::getSyclObjImpl(TempQueue); DepEvent.reset(new detail::event_impl(TempQueueImpl)); DepEvent->setContextImpl(TempQueueImpl->getContextImplPtr()); + DepEvent->setStateIncomplete(); DepEvent->getHandleRef() = reinterpret_cast(new int{}); } (void)Cmd.addDep(DepEvent, ToCleanUp); From cd82f8114564dd7ac012456e6b63f8ab962f4c94 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Wed, 15 Jun 2022 12:05:43 -0700 Subject: [PATCH 08/13] reviewer feedback I need too transfer this to another machine for testing. Signed-off-by: Chris Perkins --- sycl/source/detail/event_impl.cpp | 19 ++++++++++--------- sycl/source/detail/event_impl.hpp | 4 ++-- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 603b4bb45cb3d..056ce05e012e1 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -32,7 +32,7 @@ namespace detail { extern xpti::trace_event_data_t *GSYCLGraphEvent; #endif -// if we do not yet have a context, use the default one. +// If we do not yet have a context, use the default one. void event_impl::ensureContextInitialized() { if (!MIsContextInitialized) { const device &SyclDevice = default_selector().select_device(); @@ -42,7 +42,7 @@ void event_impl::ensureContextInitialized() { } bool event_impl::is_host() { - // we'll need a context before we can answer is_host question. + // We'll need a context before we can answer is_host question. // setting it may adjust the values of MHostEvent and MOpenCLInterop ensureContextInitialized(); // Treat all devices that don't support interoperability as host devices to @@ -367,13 +367,14 @@ void HostProfilingInfo::start() { StartTime = getTimestamp(); } void HostProfilingInfo::end() { EndTime = getTimestamp(); } pi_native_handle event_impl::getNative() { - if (!MContext) { - static context SyclContext; - MContext = getSyclObjImpl(SyclContext); - MIsContextInitialized = true; - MHostEvent = MContext->is_host(); - MOpenCLInterop = !MHostEvent; - } + ensureContextInitialized(); + // if (!MContext) { + // static context SyclContext; + // MContext = getSyclObjImpl(SyclContext); + // MIsContextInitialized = true; + // MHostEvent = MContext->is_host(); + // MOpenCLInterop = !MHostEvent; + // } auto Plugin = getPlugin(); if (!MIsInitialized) { MIsInitialized = true; diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index f8ba2c218371e..babfa506a0fb7 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -146,7 +146,7 @@ class event_impl { /// @param Context is a shared pointer to an instance of valid context_impl. void setContextImpl(const ContextImplPtr &Context); - /// clear the event state + /// Clear the event state void setStateIncomplete(); /// Returns command that is associated with the event. @@ -222,7 +222,7 @@ class event_impl { void instrumentationEpilog(void *TelementryEvent, const std::string &Name, int32_t StreamID, uint64_t IId) const; void checkProfilingPreconditions() const; - // events constructed without a context will lazily use the default context + // Events constructed without a context will lazily use the default context // when needed. void ensureContextInitialized(); mutable bool MIsInitialized = true; From 91e588a464bc7aff2062d92013bf480bb4e38647 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 23 Jun 2022 08:14:28 -0700 Subject: [PATCH 09/13] reviewer feedback Signed-off-by: Chris Perkins --- sycl/source/detail/event_impl.cpp | 19 +++++++------------ sycl/source/detail/scheduler/commands.cpp | 2 -- sycl/unittests/scheduler/QueueFlushing.cpp | 1 - 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/sycl/source/detail/event_impl.cpp b/sycl/source/detail/event_impl.cpp index 056ce05e012e1..4ed0b126ae0c5 100644 --- a/sycl/source/detail/event_impl.cpp +++ b/sycl/source/detail/event_impl.cpp @@ -34,11 +34,12 @@ extern xpti::trace_event_data_t *GSYCLGraphEvent; // If we do not yet have a context, use the default one. void event_impl::ensureContextInitialized() { - if (!MIsContextInitialized) { - const device &SyclDevice = default_selector().select_device(); - this->setContextImpl(detail::queue_impl::getDefaultOrNew( - detail::getSyclObjImpl(SyclDevice))); - } + if (MIsContextInitialized) + return; + + const device &SyclDevice = default_selector().select_device(); + this->setContextImpl( + detail::queue_impl::getDefaultOrNew(detail::getSyclObjImpl(SyclDevice))); } bool event_impl::is_host() { @@ -368,13 +369,7 @@ void HostProfilingInfo::end() { EndTime = getTimestamp(); } pi_native_handle event_impl::getNative() { ensureContextInitialized(); - // if (!MContext) { - // static context SyclContext; - // MContext = getSyclObjImpl(SyclContext); - // MIsContextInitialized = true; - // MHostEvent = MContext->is_host(); - // MOpenCLInterop = !MHostEvent; - // } + auto Plugin = getPlugin(); if (!MIsInitialized) { MIsInitialized = true; diff --git a/sycl/source/detail/scheduler/commands.cpp b/sycl/source/detail/scheduler/commands.cpp index ee17eca4d5c2c..82f8a40fcbd8e 100644 --- a/sycl/source/detail/scheduler/commands.cpp +++ b/sycl/source/detail/scheduler/commands.cpp @@ -1298,7 +1298,6 @@ MemCpyCommand::MemCpyCommand(Requirement SrcReq, MDstAllocaCmd(DstAllocaCmd) { if (!MSrcQueue->is_host()) { MEvent->setContextImpl(MSrcQueue->getContextImplPtr()); - MEvent->setStateIncomplete(); } emitInstrumentationDataProxy(); } @@ -1481,7 +1480,6 @@ MemCpyCommandHost::MemCpyCommandHost(Requirement SrcReq, MSrcAllocaCmd(SrcAllocaCmd), MDstReq(std::move(DstReq)), MDstPtr(DstPtr) { if (!MSrcQueue->is_host()) { MEvent->setContextImpl(MSrcQueue->getContextImplPtr()); - MEvent->setStateIncomplete(); } emitInstrumentationDataProxy(); diff --git a/sycl/unittests/scheduler/QueueFlushing.cpp b/sycl/unittests/scheduler/QueueFlushing.cpp index f216574ac02d5..c9bc28f5d2694 100644 --- a/sycl/unittests/scheduler/QueueFlushing.cpp +++ b/sycl/unittests/scheduler/QueueFlushing.cpp @@ -208,7 +208,6 @@ TEST_F(SchedulerTest, QueueFlushing) { access::mode::read_write}; detail::EventImplPtr DepEvent{new detail::event_impl(QueueImplB)}; DepEvent->setContextImpl(QueueImplB->getContextImplPtr()); - DepEvent->setStateIncomplete(); DepEvent->getHandleRef() = reinterpret_cast(new int{}); (void)Cmd.addDep(DepEvent, ToCleanUp); MockScheduler::enqueueCommand(&Cmd, Res, detail::NON_BLOCKING); From b8fdb479bcce9b164a16e7631cd1e78ca032927e Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Thu, 23 Jun 2022 08:18:34 -0700 Subject: [PATCH 10/13] overlooked entry --- sycl/unittests/scheduler/QueueFlushing.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sycl/unittests/scheduler/QueueFlushing.cpp b/sycl/unittests/scheduler/QueueFlushing.cpp index c9bc28f5d2694..194d7c14fa59a 100644 --- a/sycl/unittests/scheduler/QueueFlushing.cpp +++ b/sycl/unittests/scheduler/QueueFlushing.cpp @@ -225,7 +225,6 @@ TEST_F(SchedulerTest, QueueFlushing) { detail::QueueImplPtr TempQueueImpl = detail::getSyclObjImpl(TempQueue); DepEvent.reset(new detail::event_impl(TempQueueImpl)); DepEvent->setContextImpl(TempQueueImpl->getContextImplPtr()); - DepEvent->setStateIncomplete(); DepEvent->getHandleRef() = reinterpret_cast(new int{}); } (void)Cmd.addDep(DepEvent, ToCleanUp); From 50c7ea882aa5e44eb4d12e24502cb27ac7c9ed27 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Fri, 24 Jun 2022 12:10:46 -0700 Subject: [PATCH 11/13] moar reviewer feedback --- sycl/source/detail/helpers.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sycl/source/detail/helpers.cpp b/sycl/source/detail/helpers.cpp index cddaa4d25be44..921d33b7d68d5 100644 --- a/sycl/source/detail/helpers.cpp +++ b/sycl/source/detail/helpers.cpp @@ -23,10 +23,7 @@ std::vector getOrWaitEvents(std::vector DepEvents, std::vector Events; for (auto SyclEvent : DepEvents) { auto SyclEventImplPtr = detail::getSyclObjImpl(SyclEvent); - if (SyclEventImplPtr->getHandleRef() == nullptr) - continue; - if (SyclEventImplPtr->is_host() || - SyclEventImplPtr->getContextImpl() != Context) { + if (SyclEventImplPtr->getHandleRef() == nullptr) { SyclEventImplPtr->waitInternal(); } else { Events.push_back(SyclEventImplPtr->getHandleRef()); From a236afd6017938388d2767eb4da03f7443cb7c12 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Mon, 27 Jun 2022 11:04:06 -0700 Subject: [PATCH 12/13] feedback from offline discussion Signed-off-by: Chris Perkins --- sycl/source/detail/event_impl.hpp | 4 ++++ sycl/source/detail/helpers.cpp | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index c4876c29a0491..b6001e74c23e0 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -258,6 +258,10 @@ class event_impl { std::mutex MMutex; std::condition_variable cv; + + friend std::vector + getOrWaitEvents(std::vector DepEvents, + std::shared_ptr Context); }; } // namespace detail diff --git a/sycl/source/detail/helpers.cpp b/sycl/source/detail/helpers.cpp index 921d33b7d68d5..c5d3be5fe29cd 100644 --- a/sycl/source/detail/helpers.cpp +++ b/sycl/source/detail/helpers.cpp @@ -23,7 +23,14 @@ std::vector getOrWaitEvents(std::vector DepEvents, std::vector Events; for (auto SyclEvent : DepEvents) { auto SyclEventImplPtr = detail::getSyclObjImpl(SyclEvent); - if (SyclEventImplPtr->getHandleRef() == nullptr) { + // throwaway events created with default constructor will not have a context + // (which is set lazily) calling is_host(), getContextImpl() would set that + // context, which we wish to avoid as it is expensive. + if (SyclEventImplPtr->MIsContextInitialized == false) { + continue; + } + if (SyclEventImplPtr->is_host() || + SyclEventImplPtr->getContextImpl() != Context) { SyclEventImplPtr->waitInternal(); } else { Events.push_back(SyclEventImplPtr->getHandleRef()); From 1525a323f61622c930e2f6ba7542b24e174d57e0 Mon Sep 17 00:00:00 2001 From: Chris Perkins Date: Mon, 27 Jun 2022 12:06:05 -0700 Subject: [PATCH 13/13] overlooked mutable removal. much improved. --- sycl/source/detail/event_impl.hpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/event_impl.hpp b/sycl/source/detail/event_impl.hpp index b6001e74c23e0..364266cd8f210 100644 --- a/sycl/source/detail/event_impl.hpp +++ b/sycl/source/detail/event_impl.hpp @@ -226,12 +226,12 @@ class event_impl { // Events constructed without a context will lazily use the default context // when needed. void ensureContextInitialized(); - mutable bool MIsInitialized = true; - mutable bool MIsContextInitialized = false; - mutable RT::PiEvent MEvent = nullptr; - mutable ContextImplPtr MContext; - mutable bool MOpenCLInterop = false; - mutable bool MHostEvent = true; + bool MIsInitialized = true; + bool MIsContextInitialized = false; + RT::PiEvent MEvent = nullptr; + ContextImplPtr MContext; + bool MOpenCLInterop = false; + bool MHostEvent = true; std::unique_ptr MHostProfilingInfo; void *MCommand = nullptr; std::weak_ptr MQueue;