From 0ddf61e3ccaba45ee0af1d1bac12a83328e4015b Mon Sep 17 00:00:00 2001 From: Pablo Reble Date: Thu, 4 Dec 2025 07:26:41 -0800 Subject: [PATCH 1/5] Keep handle to last recorded queue after cleanup --- sycl/source/detail/graph/graph_impl.cpp | 13 ++++++------- sycl/source/detail/graph/graph_impl.hpp | 6 +++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/sycl/source/detail/graph/graph_impl.cpp b/sycl/source/detail/graph/graph_impl.cpp index 68de839376647..213ddc14b63a9 100644 --- a/sycl/source/detail/graph/graph_impl.cpp +++ b/sycl/source/detail/graph/graph_impl.cpp @@ -545,15 +545,14 @@ graph_impl::add(std::shared_ptr &DynCGImpl, return NodeImpl; } -std::shared_ptr graph_impl::getQueue() const { - std::shared_ptr Return{}; - if (!MRecordingQueues.empty()) - Return = MRecordingQueues.begin()->lock(); - return Return; +std::shared_ptr +graph_impl::getLastRecordedQueue() const { + return MLastRecordedQueue.lock(); } void graph_impl::addQueue(sycl::detail::queue_impl &RecordingQueue) { - MRecordingQueues.insert(RecordingQueue.weak_from_this()); + MLastRecordedQueue = RecordingQueue.weak_from_this(); + MRecordingQueues.insert(MLastRecordedQueue); } void graph_impl::removeQueue(sycl::detail::queue_impl &RecordingQueue) { @@ -932,7 +931,7 @@ exec_graph_impl::exec_graph_impl(sycl::context Context, // Copy nodes from GraphImpl and merge any subgraph nodes into this graph. duplicateNodes(); - if (auto PlaceholderQueuePtr = GraphImpl->getQueue()) { + if (auto PlaceholderQueuePtr = GraphImpl->getLastRecordedQueue()) { MQueueImpl = std::move(PlaceholderQueuePtr); } else { MQueueImpl = sycl::detail::queue_impl::create( diff --git a/sycl/source/detail/graph/graph_impl.hpp b/sycl/source/detail/graph/graph_impl.hpp index e6637032676cb..7f9e052fea20e 100644 --- a/sycl/source/detail/graph/graph_impl.hpp +++ b/sycl/source/detail/graph/graph_impl.hpp @@ -176,7 +176,9 @@ class graph_impl : public std::enable_shared_from_this { node_impl &add(std::shared_ptr &DynCGImpl, nodes_range Deps); - std::shared_ptr getQueue() const; + /// Get queue that was last recorded from. + /// @ return Queue that started last recording into associated graph. + std::shared_ptr getLastRecordedQueue() const; /// Add a queue to the set of queues which are currently recording to this /// graph. @@ -558,6 +560,8 @@ class graph_impl : public std::enable_shared_from_this { std::owner_less>>; /// Unique set of queues which are currently recording to this graph. RecQueuesStorage MRecordingQueues; + /// Queue that has been last recorded from. + std::weak_ptr MLastRecordedQueue; /// Map of events to their associated recorded nodes. std::unordered_map, node_impl *> MEventsMap; From 088dd501df3cf5dded3b624189cf506b6b3f6381 Mon Sep 17 00:00:00 2001 From: Pablo Reble Date: Thu, 4 Dec 2025 16:05:08 -0600 Subject: [PATCH 2/5] Adding test --- .../Extensions/CommandGraph/Common.hpp | 4 +++ .../Extensions/CommandGraph/Regressions.cpp | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/sycl/unittests/Extensions/CommandGraph/Common.hpp b/sycl/unittests/Extensions/CommandGraph/Common.hpp index aa7c59472105e..55dadfbcadbdf 100644 --- a/sycl/unittests/Extensions/CommandGraph/Common.hpp +++ b/sycl/unittests/Extensions/CommandGraph/Common.hpp @@ -46,6 +46,10 @@ class GraphImplTest { static int NumSyncPoints(const exec_graph_impl &Impl) { return Impl.MSyncPoints.size(); } + static std::shared_ptr + GetQueueImpl(const exec_graph_impl &Impl) { + return Impl.MQueueImpl; + } }; // Common Test fixture diff --git a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp index 0833357f14357..a29450526291b 100644 --- a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp +++ b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp @@ -94,3 +94,38 @@ TEST_F(CommandGraphTest, QueueRecordBarrierMultipleGraph) { Queue.ext_oneapi_submit_barrier(); GraphC.end_recording(Queue); } + +// Test that the last recorded queue is preserved after cleanup. +// This is a regression test for a bug where getLastRecordedQueue() would +// return nullptr after the recording queues were cleaned up, because the +// previous implementation (getQueue()) looked in the MRecordingQueues set +// which gets cleared on end_recording(). The fix introduces MLastRecordedQueue +// which persists even after cleanup, allowing the executable graph to retrieve +// the queue that was used for recording. +// Originally reported in commit: 0ddf61e3ccaba45ee0af1d1bac12a83328e4015b +TEST_F(CommandGraphTest, LastRecordedQueueAfterCleanup) { + // Record some work to the graph + Graph.begin_recording(Queue); + Queue.submit([&](sycl::handler &cgh) { cgh.single_task([]() {}); }); + Graph.end_recording(Queue); + + // Get the graph implementation to check internal state + auto GraphImpl = getSyclObjImpl(Graph); + + // getLastRecordedQueue() should return the queue that was used for recording + // even after end_recording() has cleared the recording queues + auto LastQueue = GraphImpl->getLastRecordedQueue(); + EXPECT_NE(LastQueue, nullptr); + EXPECT_EQ(LastQueue, getSyclObjImpl(Queue)); + + // Finalize the graph - this uses getLastRecordedQueue() internally + // to set up the executable graph's queue. Before the fix, this could fail + // if getLastRecordedQueue() returned nullptr. + auto GraphExec = Graph.finalize(); + auto ExecGraphImpl = *getSyclObjImpl(GraphExec); + + // The executable graph should have the queue from recording + auto ExecQueueImpl = GraphImplTest::GetQueueImpl(ExecGraphImpl); + EXPECT_NE(ExecQueueImpl, nullptr); + EXPECT_EQ(ExecQueueImpl, getSyclObjImpl(Queue)); +} From 8598f5b06996f402e581385e1e3e886acc417994 Mon Sep 17 00:00:00 2001 From: Pablo Reble Date: Thu, 4 Dec 2025 16:16:34 -0800 Subject: [PATCH 3/5] formatting --- sycl/unittests/Extensions/CommandGraph/Regressions.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp index a29450526291b..369051d18991d 100644 --- a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp +++ b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp @@ -106,7 +106,8 @@ TEST_F(CommandGraphTest, QueueRecordBarrierMultipleGraph) { TEST_F(CommandGraphTest, LastRecordedQueueAfterCleanup) { // Record some work to the graph Graph.begin_recording(Queue); - Queue.submit([&](sycl::handler &cgh) { cgh.single_task([]() {}); }); + Queue.submit( + [&](sycl::handler &cgh) { cgh.single_task([]() {}); }); Graph.end_recording(Queue); // Get the graph implementation to check internal state From 1df72bfd844728d0cb10b59aa7af6cc5c32eabcd Mon Sep 17 00:00:00 2001 From: Pablo Reble Date: Fri, 5 Dec 2025 08:53:17 -0800 Subject: [PATCH 4/5] fix test --- sycl/unittests/Extensions/CommandGraph/Regressions.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp index 369051d18991d..95cfc5f5e6df1 100644 --- a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp +++ b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp @@ -123,7 +123,8 @@ TEST_F(CommandGraphTest, LastRecordedQueueAfterCleanup) { // to set up the executable graph's queue. Before the fix, this could fail // if getLastRecordedQueue() returned nullptr. auto GraphExec = Graph.finalize(); - auto ExecGraphImpl = *getSyclObjImpl(GraphExec); + experimental::detail::exec_graph_impl &ExecGraphImpl = + *getSyclObjImpl(GraphExec); // The executable graph should have the queue from recording auto ExecQueueImpl = GraphImplTest::GetQueueImpl(ExecGraphImpl); From 454fe5dbe08ae6315932e58dd7d9e38df23b977c Mon Sep 17 00:00:00 2001 From: Pablo Reble Date: Fri, 5 Dec 2025 12:52:36 -0600 Subject: [PATCH 5/5] Cleanup --- sycl/unittests/Extensions/CommandGraph/Regressions.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp index 95cfc5f5e6df1..26f1ef70e60e8 100644 --- a/sycl/unittests/Extensions/CommandGraph/Regressions.cpp +++ b/sycl/unittests/Extensions/CommandGraph/Regressions.cpp @@ -102,7 +102,6 @@ TEST_F(CommandGraphTest, QueueRecordBarrierMultipleGraph) { // which gets cleared on end_recording(). The fix introduces MLastRecordedQueue // which persists even after cleanup, allowing the executable graph to retrieve // the queue that was used for recording. -// Originally reported in commit: 0ddf61e3ccaba45ee0af1d1bac12a83328e4015b TEST_F(CommandGraphTest, LastRecordedQueueAfterCleanup) { // Record some work to the graph Graph.begin_recording(Queue);