Skip to content

Commit a4c5ace

Browse files
aelovikov-intelaelovikov-intel
authored
[NFCI][SYCL] Refactor event_impl creation (#18907)
* Inherit from `std::enable_shared_for_this` as part of a bigger scope ongoing refactoring * Unlike other `*_impl` classes, `event_impl` ctors before this change weren't immediately obvious for an unfamiliar reader. Because of that, instead of a single variaded `event_impl::create(Ts&&...)` I opted to have multiple named versions of it, each delegating to its own ctor. Also, some ctor code was reshuffled between different overloads.
1 parent b0dc1ce commit a4c5ace

16 files changed

+109
-62
lines changed

‎sycl/source/backend.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ __SYCL_EXPORT event make_event(ur_native_handle_t NativeHandle,
182182
Adapter->call<UrApiKind::urEventCreateWithNativeHandle>(
183183
NativeHandle, ContextImpl->getHandleRef(), &Properties, &UrEvent);
184184
event Event = detail::createSyclObjFromImpl<event>(
185-
std::make_shared<event_impl>(UrEvent, Context));
185+
event_impl::create_from_handle(UrEvent, Context));
186186

187187
if (Backend == backend::opencl)
188188
__SYCL_OCL_CALL(clRetainEvent, ur::cast<cl_event>(NativeHandle));

‎sycl/source/detail/event_impl.cpp

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,8 @@ void event_impl::setContextImpl(const ContextImplPtr &Context) {
157157
MContext = Context;
158158
}
159159

160-
event_impl::event_impl(ur_event_handle_t Event, const context &SyclContext)
160+
event_impl::event_impl(ur_event_handle_t Event, const context &SyclContext,
161+
private_tag)
161162
: MEvent(Event), MContext(detail::getSyclObjImpl(SyclContext)),
162163
MIsFlushed(true), MState(HES_Complete) {
163164

@@ -174,21 +175,31 @@ event_impl::event_impl(ur_event_handle_t Event, const context &SyclContext)
174175
}
175176
}
176177

177-
event_impl::event_impl(const QueueImplPtr &Queue)
178-
: MQueue{Queue}, MIsProfilingEnabled{!Queue || Queue->MIsProfilingEnabled} {
179-
if (Queue)
180-
this->setContextImpl(Queue->getContextImplPtr());
181-
else {
182-
MState.store(HES_NotComplete);
178+
event_impl::event_impl(queue_impl &Queue, private_tag)
179+
: MQueue{Queue.weak_from_this()},
180+
MIsProfilingEnabled{Queue.MIsProfilingEnabled} {
181+
this->setContextImpl(Queue.getContextImplPtr());
182+
MState.store(HES_Complete);
183+
}
184+
185+
event_impl::event_impl(HostEventState State, private_tag) : MState(State) {
186+
switch (State) {
187+
case HES_Discarded:
188+
case HES_Complete: {
189+
MIsFlushed = true;
190+
MIsHostEvent = true;
191+
break;
192+
}
193+
case HES_NotComplete: {
194+
MIsProfilingEnabled = true;
183195
MHostProfilingInfo.reset(new HostProfilingInfo());
184196
if (!MHostProfilingInfo)
185197
throw sycl::exception(
186198
sycl::make_error_code(sycl::errc::runtime),
187199
"Out of host memory " +
188200
codeToString(UR_RESULT_ERROR_OUT_OF_HOST_MEMORY));
189-
return;
190201
}
191-
MState.store(HES_Complete);
202+
}
192203
}
193204

194205
void event_impl::setQueue(queue_impl &Queue) {

‎sycl/source/detail/event_impl.hpp

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,11 @@ using QueueImplPtr = std::shared_ptr<sycl::detail::queue_impl>;
3535
class event_impl;
3636
using EventImplPtr = std::shared_ptr<sycl::detail::event_impl>;
3737

38-
class event_impl {
38+
class event_impl : public std::enable_shared_from_this<event_impl> {
39+
struct private_tag {
40+
explicit private_tag() = default;
41+
};
42+
3943
public:
4044
enum HostEventState : int {
4145
HES_NotComplete = 0,
@@ -46,11 +50,9 @@ class event_impl {
4650
/// Constructs a ready SYCL event.
4751
///
4852
/// If the constructed SYCL event is waited on it will complete immediately.
49-
/// Normally constructs a host event, use std::nullopt to instead instantiate
50-
/// a device event.
51-
event_impl(std::optional<HostEventState> State = HES_Complete)
52-
: MIsFlushed(true), MState(State.value_or(HES_Complete)),
53-
MIsDefaultConstructed(!State), MIsHostEvent(State) {
53+
event_impl(private_tag)
54+
: MIsFlushed(true), MState(HES_Complete), MIsDefaultConstructed(true),
55+
MIsHostEvent(false) {
5456
// Need to fail in event() constructor if there are problems with the
5557
// ONEAPI_DEVICE_SELECTOR. Deferring may lead to conficts with noexcept
5658
// event methods. This ::get() call uses static vars to read and parse the
@@ -65,8 +67,39 @@ class event_impl {
6567
///
6668
/// \param Event is a valid instance of UR event.
6769
/// \param SyclContext is an instance of SYCL context.
68-
event_impl(ur_event_handle_t Event, const context &SyclContext);
69-
event_impl(const QueueImplPtr &Queue);
70+
event_impl(ur_event_handle_t Event, const context &SyclContext, private_tag);
71+
72+
event_impl(queue_impl &Queue, private_tag);
73+
event_impl(HostEventState State, private_tag);
74+
75+
// Corresponds to `sycl::event{}`.
76+
static std::shared_ptr<event_impl> create_default_event() {
77+
return std::make_shared<event_impl>(private_tag{});
78+
}
79+
80+
static std::shared_ptr<event_impl>
81+
create_from_handle(ur_event_handle_t Event, const context &SyclContext) {
82+
return std::make_shared<event_impl>(Event, SyclContext, private_tag{});
83+
}
84+
85+
static std::shared_ptr<event_impl> create_device_event(queue_impl &queue) {
86+
return std::make_shared<event_impl>(queue, private_tag{});
87+
}
88+
89+
static std::shared_ptr<event_impl> create_discarded_event() {
90+
return std::make_shared<event_impl>(HostEventState::HES_Discarded,
91+
private_tag{});
92+
}
93+
94+
static std::shared_ptr<event_impl> create_completed_host_event() {
95+
return std::make_shared<event_impl>(HostEventState::HES_Complete,
96+
private_tag{});
97+
}
98+
99+
static std::shared_ptr<event_impl> create_incomplete_host_event() {
100+
return std::make_shared<event_impl>(HostEventState::HES_NotComplete,
101+
private_tag{});
102+
}
70103

71104
/// Sets a queue associated with the event
72105
///

‎sycl/source/detail/graph_impl.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,8 @@ std::shared_ptr<node_impl> graph_impl::addNodesToExits(
404404
// Add all the new nodes to the node storage
405405
for (auto &Node : NodeList) {
406406
MNodeStorage.push_back(Node);
407-
addEventForNode(std::make_shared<sycl::detail::event_impl>(), Node);
407+
addEventForNode(sycl::detail::event_impl::create_completed_host_event(),
408+
Node);
408409
}
409410

410411
return this->add(Outputs);
@@ -494,7 +495,8 @@ graph_impl::add(std::vector<std::shared_ptr<node_impl>> &Deps) {
494495

495496
addDepsToNode(NodeImpl, Deps);
496497
// Add an event associated with this explicit node for mixed usage
497-
addEventForNode(std::make_shared<sycl::detail::event_impl>(), NodeImpl);
498+
addEventForNode(sycl::detail::event_impl::create_completed_host_event(),
499+
NodeImpl);
498500
return NodeImpl;
499501
}
500502

@@ -552,7 +554,8 @@ graph_impl::add(std::function<void(handler &)> CGF,
552554
this->add(NodeType, std::move(Handler.impl->MGraphNodeCG), Deps);
553555

554556
// Add an event associated with this explicit node for mixed usage
555-
addEventForNode(std::make_shared<sycl::detail::event_impl>(), NodeImpl);
557+
addEventForNode(sycl::detail::event_impl::create_completed_host_event(),
558+
NodeImpl);
556559

557560
// Retrieve any dynamic parameters which have been registered in the CGF and
558561
// register the actual nodes with them.
@@ -651,7 +654,8 @@ graph_impl::add(std::shared_ptr<dynamic_command_group_impl> &DynCGImpl,
651654
add(NodeType, ActiveKernel, Deps);
652655

653656
// Add an event associated with this explicit node for mixed usage
654-
addEventForNode(std::make_shared<sycl::detail::event_impl>(), NodeImpl);
657+
addEventForNode(sycl::detail::event_impl::create_completed_host_event(),
658+
NodeImpl);
655659

656660
// Track the dynamic command-group used inside the node object
657661
DynCGImpl->MNodes.push_back(NodeImpl);
@@ -1032,8 +1036,7 @@ exec_graph_impl::enqueue(sycl::detail::queue_impl &Queue,
10321036
PartitionsExecutionEvents;
10331037

10341038
auto CreateNewEvent([&]() {
1035-
auto NewEvent =
1036-
std::make_shared<sycl::detail::event_impl>(Queue.shared_from_this());
1039+
auto NewEvent = sycl::detail::event_impl::create_device_event(Queue);
10371040
NewEvent->setContextImpl(Queue.getContextImplPtr());
10381041
NewEvent->setStateIncomplete();
10391042
return NewEvent;

‎sycl/source/detail/queue_impl.cpp

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -120,18 +120,12 @@ queue_impl::get_backend_info<info::device::backend_version>() const {
120120

121121
static event prepareSYCLEventAssociatedWithQueue(
122122
const std::shared_ptr<detail::queue_impl> &QueueImpl) {
123-
auto EventImpl = std::make_shared<detail::event_impl>(QueueImpl);
123+
auto EventImpl = detail::event_impl::create_device_event(*QueueImpl);
124124
EventImpl->setContextImpl(detail::getSyclObjImpl(QueueImpl->get_context()));
125125
EventImpl->setStateIncomplete();
126126
return detail::createSyclObjFromImpl<event>(EventImpl);
127127
}
128128

129-
static event createDiscardedEvent() {
130-
EventImplPtr EventImpl =
131-
std::make_shared<event_impl>(event_impl::HES_Discarded);
132-
return createSyclObjFromImpl<event>(std::move(EventImpl));
133-
}
134-
135129
const std::vector<event> &
136130
queue_impl::getExtendDependencyList(const std::vector<event> &DepEvents,
137131
std::vector<event> &MutableVec,
@@ -429,7 +423,7 @@ event queue_impl::submitWithHandler(const std::vector<event> &DepEvents,
429423
if (!CallerNeedsEvent && supportsDiscardingPiEvents()) {
430424
submit_without_event(CGF, SI,
431425
/*CodeLoc*/ {}, /*IsTopCodeLoc*/ true);
432-
return createDiscardedEvent();
426+
return createSyclObjFromImpl<event>(event_impl::create_discarded_event());
433427
}
434428
return submit_with_event(CGF, SI,
435429
/*CodeLoc*/ {}, /*IsTopCodeLoc*/ true);
@@ -463,7 +457,8 @@ event queue_impl::submitMemOpHelper(const std::vector<event> &DepEvents,
463457
getUrEvents(ExpandedDepEvents),
464458
/*PiEvent*/ nullptr);
465459

466-
return createDiscardedEvent();
460+
return createSyclObjFromImpl<event>(
461+
event_impl::create_discarded_event());
467462
}
468463

469464
event ResEvent = prepareSYCLEventAssociatedWithQueue(shared_from_this());

‎sycl/source/detail/queue_impl.hpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> {
665665
/// will wait for the completion of all work in the queue at the time of the
666666
/// insertion, but will not act as a barrier unless the queue is in-order.
667667
EventImplPtr insertMarkerEvent() {
668-
auto ResEvent = std::make_shared<detail::event_impl>(shared_from_this());
668+
auto ResEvent = detail::event_impl::create_device_event(*this);
669669
ur_event_handle_t UREvent = nullptr;
670670
getAdapter()->call<UrApiKind::urEnqueueEventsWait>(getHandleRef(), 0,
671671
nullptr, &UREvent);
@@ -690,8 +690,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> {
690690
template <typename HandlerType = handler>
691691
EventImplPtr insertHelperBarrier(const HandlerType &Handler) {
692692
auto &Queue = Handler.impl->get_queue();
693-
auto ResEvent =
694-
std::make_shared<detail::event_impl>(Queue.shared_from_this());
693+
auto ResEvent = detail::event_impl::create_device_event(Queue);
695694
ur_event_handle_t UREvent = nullptr;
696695
getAdapter()->call<UrApiKind::urEnqueueEventsWaitWithBarrier>(
697696
Queue.getHandleRef(), 0, nullptr, &UREvent);

‎sycl/source/detail/reduction.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ __SYCL_EXPORT size_t reduGetPreferredWGSize(std::shared_ptr<queue_impl> &Queue,
207207
__SYCL_EXPORT void
208208
addCounterInit(handler &CGH, std::shared_ptr<sycl::detail::queue_impl> &Queue,
209209
std::shared_ptr<int> &Counter) {
210-
auto EventImpl = std::make_shared<detail::event_impl>(Queue);
210+
auto EventImpl = detail::event_impl::create_device_event(*Queue);
211211
EventImpl->setContextImpl(detail::getSyclObjImpl(Queue->get_context()));
212212
EventImpl->setStateIncomplete();
213213
ur_event_handle_t UREvent = nullptr;

‎sycl/source/detail/scheduler/commands.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -570,7 +570,8 @@ Command::Command(
570570
ur_exp_command_buffer_handle_t CommandBuffer,
571571
const std::vector<ur_exp_command_buffer_sync_point_t> &SyncPoints)
572572
: MQueue(std::move(Queue)),
573-
MEvent(std::make_shared<detail::event_impl>(MQueue)),
573+
MEvent(MQueue ? detail::event_impl::create_device_event(*MQueue)
574+
: detail::event_impl::create_incomplete_host_event()),
574575
MPreparedDepsEvents(MEvent->getPreparedDepsEvents()),
575576
MPreparedHostDepsEvents(MEvent->getPreparedHostDepsEvents()), MType(Type),
576577
MCommandBuffer(CommandBuffer), MSyncPointDeps(SyncPoints) {
@@ -1298,9 +1299,11 @@ ur_result_t ReleaseCommand::enqueueImp() {
12981299
? MAllocaCmd->MLinkedAllocaCmd->getQueue()
12991300
: MAllocaCmd->getQueue();
13001301

1301-
EventImplPtr UnmapEventImpl(new event_impl(Queue));
1302-
UnmapEventImpl->setContextImpl(Queue ? Queue->getContextImplPtr()
1303-
: nullptr);
1302+
assert(Queue);
1303+
1304+
std::shared_ptr<event_impl> UnmapEventImpl =
1305+
event_impl::create_device_event(*Queue);
1306+
UnmapEventImpl->setContextImpl(Queue->getContextImplPtr());
13041307
UnmapEventImpl->setStateIncomplete();
13051308
ur_event_handle_t UREvent = nullptr;
13061309

‎sycl/source/event.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@
2222
namespace sycl {
2323
inline namespace _V1 {
2424

25-
event::event() : impl(std::make_shared<detail::event_impl>(std::nullopt)) {}
25+
event::event() : impl(detail::event_impl::create_default_event()) {}
2626

2727
event::event(cl_event ClEvent, const context &SyclContext)
28-
: impl(std::make_shared<detail::event_impl>(
28+
: impl(detail::event_impl::create_from_handle(
2929
detail::ur::cast<ur_event_handle_t>(ClEvent), SyclContext)) {
3030
// This is a special interop constructor for OpenCL, so the event must be
3131
// retained.

‎sycl/source/handler.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -547,7 +547,7 @@ event handler::finalize() {
547547

548548
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
549549
if (!DiscardEvent) {
550-
LastEventImpl = std::make_shared<detail::event_impl>();
550+
LastEventImpl = detail::event_impl::create_completed_host_event();
551551
}
552552
#endif
553553

@@ -824,7 +824,7 @@ event handler::finalize() {
824824
// so it can be retrieved by the graph later.
825825
if (impl->get_graph_or_null()) {
826826
impl->MGraphNodeCG = std::move(CommandGroup);
827-
auto EventImpl = std::make_shared<detail::event_impl>();
827+
auto EventImpl = detail::event_impl::create_completed_host_event();
828828
#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
829829
return EventImpl;
830830
#else
@@ -838,7 +838,7 @@ event handler::finalize() {
838838
// If the queue has an associated graph then we need to take the CG and pass
839839
// it to the graph to create a node, rather than submit it to the scheduler.
840840
if (auto GraphImpl = Queue->getCommandGraph(); GraphImpl) {
841-
auto EventImpl = std::make_shared<detail::event_impl>();
841+
auto EventImpl = detail::event_impl::create_completed_host_event();
842842
EventImpl->setSubmittedQueue(Queue->weak_from_this());
843843
std::shared_ptr<ext::oneapi::experimental::detail::node_impl> NodeImpl =
844844
nullptr;

‎sycl/source/queue.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ getBarrierEventForInorderQueueHelper(const detail::QueueImplPtr QueueImpl) {
342342

343343
// If there was no last event, we create an empty one.
344344
return detail::createSyclObjFromImpl<event>(
345-
std::make_shared<detail::event_impl>(std::nullopt));
345+
detail::event_impl::create_default_event());
346346
}
347347

348348
/// Prevents any commands submitted afterward to this queue from executing

‎sycl/unittests/scheduler/BlockedCommands.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,8 @@ TEST_F(SchedulerTest, EnqueueHostDependency) {
164164
B.MIsBlockable = true;
165165
B.MRetVal = UR_RESULT_SUCCESS;
166166

167-
sycl::detail::EventImplPtr DepEvent{
168-
new sycl::detail::event_impl(detail::getSyclObjImpl(Q))};
167+
std::shared_ptr<sycl::detail::event_impl> DepEvent =
168+
sycl::detail::event_impl::create_device_event(*detail::getSyclObjImpl(Q));
169169
DepEvent->setCommand(&B);
170170

171171
std::vector<detail::Command *> ToCleanUp;

‎sycl/unittests/scheduler/Commands.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,12 @@ TEST_F(SchedulerTest, WaitEmptyEventWithBarrier) {
6363
mock::getCallbacks().set_before_callback("urEventGetInfo",
6464
&redefineUrEventGetInfo);
6565

66-
auto EmptyEvent = std::make_shared<detail::event_impl>();
66+
auto EmptyEvent = detail::event_impl::create_completed_host_event();
6767

6868
ur_event_handle_t UREvent = mock::createDummyHandle<ur_event_handle_t>();
6969

7070
auto Event =
71-
std::make_shared<detail::event_impl>(UREvent, Queue.get_context());
71+
detail::event_impl::create_from_handle(UREvent, Queue.get_context());
7272

7373
using EventList = std::vector<detail::EventImplPtr>;
7474
std::vector<EventList> InputEventWaitLists = {

‎sycl/unittests/scheduler/CommandsWaitForEvents.cpp

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ TEST_F(SchedulerTest, StreamAUXCmdsWait) {
182182

183183
ur_event_handle_t UREvent = mock::createDummyHandle<ur_event_handle_t>();
184184

185-
auto EventImpl = std::make_shared<sycl::detail::event_impl>(QueueImpl);
185+
auto EventImpl = sycl::detail::event_impl::create_device_event(*QueueImpl);
186186
EventImpl->setHandle(UREvent);
187187

188188
QueueImplProxy->registerStreamServiceEvent(EventImpl);
@@ -209,10 +209,12 @@ TEST_F(SchedulerTest, CommandsWaitForEvents) {
209209

210210
TestContext.reset(new TestCtx(Q1, Q2));
211211

212-
std::shared_ptr<detail::event_impl> E1(
213-
new detail::event_impl(TestContext->EventCtx1, Q1.get_context()));
214-
std::shared_ptr<detail::event_impl> E2(
215-
new detail::event_impl(TestContext->EventCtx2, Q2.get_context()));
212+
std::shared_ptr<detail::event_impl> E1 =
213+
detail::event_impl::create_from_handle(TestContext->EventCtx1,
214+
Q1.get_context());
215+
std::shared_ptr<detail::event_impl> E2 =
216+
detail::event_impl::create_from_handle(TestContext->EventCtx2,
217+
Q2.get_context());
216218

217219
MockCommand Cmd(nullptr);
218220

‎sycl/unittests/scheduler/InOrderQueueSyncCheck.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class LimitedHandler {
4949
#else
5050
virtual event finalize() {
5151
sycl::detail::EventImplPtr NewEvent =
52-
std::make_shared<detail::event_impl>();
52+
detail::event_impl::create_completed_host_event();
5353
return sycl::detail::createSyclObjFromImpl<sycl::event>(NewEvent);
5454
}
5555
#endif

0 commit comments

Comments
 (0)