Skip to content

[NFCI][SYCL] Prefer raw ptr/ref for queue_impl #18874

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

Closed
wants to merge 1 commit into from
Closed

[NFCI][SYCL] Prefer raw ptr/ref for queue_impl #18874

Show file tree
Hide file tree
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
9 changes: 3 additions & 6 deletions sycl/source/detail/cg.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,14 +725,11 @@ class CGHostTask : public CG {
std::shared_ptr<detail::context_impl> MContext;
std::vector<ArgDesc> MArgs;

CGHostTask(std::shared_ptr<HostTask> HostTask,
std::shared_ptr<detail::queue_impl> Queue,
// TODO: ref?
CGHostTask(std::shared_ptr<HostTask> HostTask, detail::queue_impl *Queue,
std::shared_ptr<detail::context_impl> Context,
std::vector<ArgDesc> Args, CG::StorageInitHelper CGData,
CGType Type, detail::code_location loc = {})
: CG(Type, std::move(CGData), std::move(loc)),
MHostTask(std::move(HostTask)), MQueue(Queue), MContext(Context),
MArgs(std::move(Args)) {}
CGType Type, detail::code_location loc = {});
};

} // namespace detail
Expand Down
20 changes: 10 additions & 10 deletions sycl/source/detail/event_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ void *event_impl::instrumentationProlog(std::string &Name, int32_t StreamID,
// queue is available with the wait events. We check to see if the
// TraceEvent is available in the Queue object.
void *TraceEvent = nullptr;
if (QueueImplPtr Queue = MQueue.lock()) {
if (auto Queue = MQueue.lock()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to get rid of QueueImplPtr? When it comes to code reading & understanding it is much easier to understand what type variable "Queue" is if it is declared as specific type. In this case I do not see any benefits in usage of "auto".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying from my reply in #18983 (comment):

This alias was created because we had almost all APIs using it. The idea behind the ongoing refactoring is to have as little APIs operating on shared_ptr as possible (and avoid accidental copies as a byproduct). As such, having the alias in future won't be improving readability but reducing it, because the usage of that alias would be very rare.

As such, I'm simply removing the aliases from the headers that were [mostly] cleaned of excessive shared_ptr usage.

Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest to use shared_ptr explicitly here. I am ok with alias removal but I don't like its replacement with auto.

TraceEvent = Queue->getTraceEvent();
WaitEvent =
(TraceEvent ? static_cast<xpti_td *>(TraceEvent) : GSYCLGraphEvent);
Expand Down Expand Up @@ -317,7 +317,7 @@ void event_impl::wait_and_throw(
std::shared_ptr<sycl::detail::event_impl> Self) {
wait(Self);

if (QueueImplPtr SubmittedQueue = MSubmittedQueue.lock())
if (auto SubmittedQueue = MSubmittedQueue.lock())
SubmittedQueue->throw_asynchronous();
}

Expand Down Expand Up @@ -462,7 +462,7 @@ event_impl::get_backend_info<info::platform::version>() const {
"the info::platform::version info descriptor can "
"only be queried with an OpenCL backend");
}
if (QueueImplPtr Queue = MQueue.lock()) {
if (auto Queue = MQueue.lock()) {
return Queue->getDeviceImpl()
.get_platform()
.get_info<info::platform::version>();
Expand All @@ -485,7 +485,7 @@ event_impl::get_backend_info<info::device::version>() const {
"the info::device::version info descriptor can only "
"be queried with an OpenCL backend");
}
if (QueueImplPtr Queue = MQueue.lock()) {
if (auto Queue = MQueue.lock()) {
return Queue->getDeviceImpl().get_info<info::device::version>();
}
return ""; // If the queue has been released, no device will be associated so
Expand Down Expand Up @@ -552,21 +552,21 @@ std::vector<EventImplPtr> event_impl::getWaitList() {
return Result;
}

void event_impl::flushIfNeeded(const QueueImplPtr &UserQueue) {
void event_impl::flushIfNeeded(queue_impl *UserQueue) {
// Some events might not have a native handle underneath even at this point,
// e.g. those produced by memset with 0 size (no UR call is made).
auto Handle = this->getHandle();
if (MIsFlushed || !Handle)
return;

QueueImplPtr Queue = MQueue.lock();
auto Queue = MQueue.lock();
// If the queue has been released, all of the commands have already been
// implicitly flushed by urQueueRelease.
if (!Queue) {
MIsFlushed = true;
return;
}
if (Queue == UserQueue)
if (Queue.get() == UserQueue)
return;

// Check if the task for this event has already been submitted.
Expand Down Expand Up @@ -604,9 +604,9 @@ void event_impl::setSubmissionTime() {
if (!MIsProfilingEnabled && !MProfilingTagEvent)
return;

std::weak_ptr<queue_impl> Queue = isHost() ? MSubmittedQueue : MQueue;
if (QueueImplPtr QueuePtr = Queue.lock()) {
device_impl &Device = QueuePtr->getDeviceImpl();
if (std::shared_ptr<queue_impl> Queue =
isHost() ? MSubmittedQueue.lock() : MQueue.lock()) {
device_impl &Device = Queue->getDeviceImpl();
MSubmitTime = getTimestamp(&Device);
}
}
Expand Down
11 changes: 7 additions & 4 deletions sycl/source/detail/event_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class Adapter;
class context_impl;
using ContextImplPtr = std::shared_ptr<sycl::detail::context_impl>;
class queue_impl;
using QueueImplPtr = std::shared_ptr<sycl::detail::queue_impl>;
class event_impl;
using EventImplPtr = std::shared_ptr<sycl::detail::event_impl>;

Expand Down Expand Up @@ -242,7 +241,7 @@ class event_impl : public std::enable_shared_from_this<event_impl> {
/// Performs a flush on the queue associated with this event if the user queue
/// is different and the task associated with this event hasn't been submitted
/// to the device yet.
void flushIfNeeded(const QueueImplPtr &UserQueue);
void flushIfNeeded(queue_impl *UserQueue);

/// Cleans dependencies of this event_impl.
void cleanupDependencyEvents();
Expand All @@ -262,7 +261,9 @@ class event_impl : public std::enable_shared_from_this<event_impl> {
///
/// @return shared_ptr to MWorkerQueue, please be aware it can be empty
/// pointer
QueueImplPtr getWorkerQueue() { return MWorkerQueue.lock(); };
std::shared_ptr<sycl::detail::queue_impl> getWorkerQueue() {
return MWorkerQueue.lock();
};

/// Sets worker queue for command.
///
Expand All @@ -289,7 +290,9 @@ class event_impl : public std::enable_shared_from_this<event_impl> {
/// @return Submission time for command associated with this event
uint64_t getSubmissionTime();

QueueImplPtr getSubmittedQueue() const { return MSubmittedQueue.lock(); };
std::shared_ptr<sycl::detail::queue_impl> getSubmittedQueue() const {
return MSubmittedQueue.lock();
};

/// Checks if this event is complete.
///
Expand Down
8 changes: 4 additions & 4 deletions sycl/source/detail/graph_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ exec_graph_impl::enqueueNode(ur_exp_command_buffer_handle_t CommandBuffer,

sycl::detail::EventImplPtr Event =
sycl::detail::Scheduler::getInstance().addCG(
Node->getCGCopy(), MQueueImpl,
Node->getCGCopy(), *MQueueImpl,
/*EventNeeded=*/true, CommandBuffer, Deps);

if (MIsUpdatable) {
Expand Down Expand Up @@ -1120,7 +1120,7 @@ exec_graph_impl::enqueue(sycl::detail::queue_impl &Queue,
CommandBuffer, nullptr, std::move(CGData));

NewEvent = sycl::detail::Scheduler::getInstance().addCG(
std::move(CommandGroup), Queue.shared_from_this(),
std::move(CommandGroup), Queue,
/*EventNeeded=*/true);
}
NewEvent->setEventFromSubmittedExecCommandBuffer(true);
Expand All @@ -1140,7 +1140,7 @@ exec_graph_impl::enqueue(sycl::detail::queue_impl &Queue,
.MQueue = Queue.shared_from_this();

NewEvent = sycl::detail::Scheduler::getInstance().addCG(
NodeImpl->getCGCopy(), Queue.shared_from_this(),
NodeImpl->getCGCopy(), Queue,
/*EventNeeded=*/true);
}
PartitionsExecutionEvents[CurrentPartition] = NewEvent;
Expand Down Expand Up @@ -1422,7 +1422,7 @@ void exec_graph_impl::update(
// other scheduler commands
auto UpdateEvent =
sycl::detail::Scheduler::getInstance().addCommandGraphUpdate(
this, Nodes, MQueueImpl, std::move(UpdateRequirements),
this, Nodes, MQueueImpl.get(), std::move(UpdateRequirements),
MExecutionEvents);

MExecutionEvents.push_back(UpdateEvent);
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/graph_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ class node_impl : public std::enable_shared_from_this<node_impl> {

return std::make_unique<sycl::detail::CGHostTask>(
sycl::detail::CGHostTask(
std::move(HostTaskSPtr), CommandGroupPtr->MQueue,
std::move(HostTaskSPtr), CommandGroupPtr->MQueue.get(),
CommandGroupPtr->MContext, std::move(NewArgs), std::move(Data),
CommandGroupPtr->getType(), Loc));
}
Expand Down
1 change: 0 additions & 1 deletion sycl/source/detail/helpers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class event;
namespace detail {
class CGExecKernel;
class queue_impl;
using QueueImplPtr = std::shared_ptr<sycl::detail::queue_impl>;
class RTDeviceBinaryImage;

#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
Expand Down
1 change: 0 additions & 1 deletion sycl/source/detail/memory_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class queue_impl;
class event_impl;
class context_impl;

using QueueImplPtr = std::shared_ptr<detail::queue_impl>;
using EventImplPtr = std::shared_ptr<detail::event_impl>;

// The class contains methods that work with memory. All operations with
Expand Down
10 changes: 5 additions & 5 deletions sycl/source/detail/queue_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ queue_impl::get_backend_info<info::device::backend_version>() const {
}
#endif

static event prepareSYCLEventAssociatedWithQueue(
const std::shared_ptr<detail::queue_impl> &QueueImpl) {
auto EventImpl = detail::event_impl::create_device_event(*QueueImpl);
EventImpl->setContextImpl(detail::getSyclObjImpl(QueueImpl->get_context()));
static event
prepareSYCLEventAssociatedWithQueue(detail::queue_impl &QueueImpl) {
auto EventImpl = detail::event_impl::create_device_event(QueueImpl);
EventImpl->setContextImpl(QueueImpl.getContextImplPtr());
EventImpl->setStateIncomplete();
return detail::createSyclObjFromImpl<event>(EventImpl);
}
Expand Down Expand Up @@ -461,7 +461,7 @@ event queue_impl::submitMemOpHelper(const std::vector<event> &DepEvents,
event_impl::create_discarded_event());
}

event ResEvent = prepareSYCLEventAssociatedWithQueue(shared_from_this());
event ResEvent = prepareSYCLEventAssociatedWithQueue(*this);
const auto &EventImpl = detail::getSyclObjImpl(ResEvent);
{
NestedCallsTracker tracker;
Expand Down
7 changes: 5 additions & 2 deletions sycl/source/detail/queue_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,9 +650,12 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> {
// for in order ones.
void revisitUnenqueuedCommandsState(const EventImplPtr &CompletedHostTask);

static ContextImplPtr getContext(const QueueImplPtr &Queue) {
static ContextImplPtr getContext(queue_impl *Queue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

getContextImplPtr is const method. Why can't we declare Queue as pointer to const queue_impl then? To emphasize that the object doesn't change. Applicable to other places where it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const std::shared_ptr<queue_impl> & only said that the shared pointer didn't change, it provided no such guarantees about queue_impl. We can cleanup const-correctness in a separate refactoring once this one is finished, but we'll first have to agree how exactly we want to treat it (e.g. should we return const queue_impl or queue_impl from const handler_impl?).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not referring to the old impl, I saw that we have const for pointer only, but since we have another type now - probably we can do better.

return Queue ? Queue->getContextImplPtr() : nullptr;
}
static ContextImplPtr getContext(const std::shared_ptr<queue_impl> &Queue) {
return getContext(Queue.get());
}

// Must be called under MMutex protection
void doUnenqueuedCommandCleanup(
Expand Down Expand Up @@ -689,7 +692,7 @@ class queue_impl : public std::enable_shared_from_this<queue_impl> {
protected:
template <typename HandlerType = handler>
EventImplPtr insertHelperBarrier(const HandlerType &Handler) {
auto &Queue = Handler.impl->get_queue();
queue_impl &Queue = Handler.impl->get_queue();
auto ResEvent = detail::event_impl::create_device_event(Queue);
ur_event_handle_t UREvent = nullptr;
getAdapter()->call<UrApiKind::urEnqueueEventsWaitWithBarrier>(
Expand Down
Loading