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

aelovikov-intel · 2025-06-09T19:44:57Z

Continuation of the refactoring in
#18715
#18748

aelovikov-intel · 2025-06-09T22:13:51Z

sycl/source/detail/event_impl.cpp

  MState.store(HES_Complete);
 }

+// TODO: comment about https://github.com/intel/llvm/pull/14370
+event_impl::event_impl(std::nullptr_t) : MQueue{}, MIsProfilingEnabled{true} {


@sergey-semenov , @KseniyaTikhomirova any idea/preferences on how I should change the ctors here?

I think it's very surprising that (before my changes)
event_impl e() and event_impl e(nullptr) behave/are implemented so differently.

seems to be not applicable anymore, doesn't it? I don't see this constructor in the current mainline.

Yes, I've split that part of the change into dedicated #18907 that has landed already.

aelovikov-intel · 2025-06-09T22:15:24Z

sycl/source/detail/scheduler/commands.hpp

@@ -121,7 +120,7 @@ class Command {
  };

  Command(
-      CommandType Type, QueueImplPtr Queue,
+      CommandType Type, queue_impl *Queue,


@sergey-semenov or @intel/sycl-graphs-reviewers , do you know if Queue must be non-null for any of the subclasses?

I'm not really an expert on the scheduler, but I'd expect the ExecCGCommand subclass to expect the queue to be non-null as MQueue will be used to enqueue work.

Looking at ExecCGCommand::enqueueImpQueue in scheduler.cpp there are a few places with assert(MQueue

I believe it must be not-null for all of them. even host task.

Even if we're submitting to the graph and not actual queue?

graph has queue as input parameter for begin_recording API https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_graph.asciidoc#84-graph,
so I expect graph commands to have corresponding queue too.
Although it is better to have a confirmation from graph extension authors or check yourself.

Yeah, since sycl buffers are separated into initialization commands and copy commands in the scheduler, we make sure that those have completed when it comes to finalize graph. We've got a bit of info on that in the last 3 paragraphs of https://github.com/intel/llvm/blob/sycl/sycl/doc/design/CommandGraph.md#scheduler-integration

KseniyaTikhomirova

I think at this point we need to clearly document somewhere why & how it is guaranteed to have valid (alive) raw pointer to Queue among so many objects, what object controls its lifetime and what destruction order we have.

KseniyaTikhomirova · 2025-06-16T09:41:48Z

sycl/source/detail/event_impl.cpp

@@ -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()) {


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".

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.

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

KseniyaTikhomirova · 2025-06-16T09:49:54Z

sycl/source/detail/scheduler/scheduler.hpp

@@ -559,9 +558,8 @@ class Scheduler {
    /// \return a command that represents command group execution and a bool
    /// indicating whether this command should be enqueued to the graph
    /// processor right away or not.
-    Command *addCG(std::unique_ptr<detail::CG> CommandGroup,


I'd prefer method with similar names (addCG) to access same variable type of Queue. It is confusing when one method accepts reference while another - raw pointer.

I disagree. The distinction (that one interface has nullptr as valid input and the othder doesn't) is important, and I want to know it in advance and verified by the compiler through type system.

KseniyaTikhomirova · 2025-06-16T09:56:48Z

sycl/source/detail/queue_impl.hpp

@@ -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) {


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.

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?).

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.

KseniyaTikhomirova · 2025-06-16T10:01:41Z

sycl/source/detail/scheduler/commands.hpp

@@ -121,7 +120,7 @@ class Command {
  };

  Command(
-      CommandType Type, QueueImplPtr Queue,
+      CommandType Type, queue_impl *Queue,


I believe it must be not-null for all of them. even host task.

aelovikov-intel temporarily deployed to WindowsCILock June 9, 2025 19:45 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 9, 2025 20:22 — with GitHub Actions Inactive

aelovikov-intel force-pushed the queue_impl branch from 0659eb9 to a281790 Compare June 9, 2025 21:07

aelovikov-intel temporarily deployed to WindowsCILock June 9, 2025 21:07 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 9, 2025 21:35 — with GitHub Actions Inactive

aelovikov-intel force-pushed the queue_impl branch from a281790 to 09a1495 Compare June 9, 2025 22:11

aelovikov-intel had a problem deploying to WindowsCILock June 9, 2025 22:11 — with GitHub Actions Failure

aelovikov-intel commented Jun 9, 2025

View reviewed changes

aelovikov-intel temporarily deployed to WindowsCILock June 9, 2025 22:38 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 10, 2025 01:43 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 10, 2025 02:05 — with GitHub Actions Inactive

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

ab2a122

aelovikov-intel force-pushed the queue_impl branch from 09a1495 to ab2a122 Compare June 13, 2025 22:36

aelovikov-intel temporarily deployed to WindowsCILock June 13, 2025 22:37 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock June 13, 2025 23:04 — with GitHub Actions Inactive

KseniyaTikhomirova reviewed Jun 16, 2025

View reviewed changes

aelovikov-intel closed this Jun 16, 2025

aelovikov-intel deleted the queue_impl branch June 16, 2025 21:13

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

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

Uh oh!

Conversation

aelovikov-intel commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

KseniyaTikhomirova Jun 16, 2025 •

edited

Loading