From afb29b2da252fe6e12bfde5c52e653db8583245c Mon Sep 17 00:00:00 2001 From: "Tikhomirova, Kseniya" Date: Wed, 12 Nov 2025 04:43:36 -0800 Subject: [PATCH 1/4] [SYCL] Fix coverity hits in queue & graph Signed-off-by: Tikhomirova, Kseniya --- sycl/source/detail/graph/graph_impl.cpp | 23 +++++++++++++---------- sycl/source/detail/graph/graph_impl.hpp | 14 +++++++------- sycl/source/detail/queue_impl.cpp | 2 +- sycl/source/detail/queue_impl.hpp | 6 ++++-- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/sycl/source/detail/graph/graph_impl.cpp b/sycl/source/detail/graph/graph_impl.cpp index 9f878f0e8ea66..af10c391963b2 100644 --- a/sycl/source/detail/graph/graph_impl.cpp +++ b/sycl/source/detail/graph/graph_impl.cpp @@ -327,7 +327,7 @@ graph_impl::graph_impl(const sycl::context &SyclContext, graph_impl::~graph_impl() { try { - clearQueues(); + clearQueues(false /*Needs lock*/); for (auto &MemObj : MMemObjs) { MemObj->markNoLongerBeingUsedInGraph(); } @@ -564,17 +564,21 @@ void graph_impl::removeQueue(sycl::detail::queue_impl &RecordingQueue) { MRecordingQueues.erase(RecordingQueue.weak_from_this()); } -bool graph_impl::clearQueues() { - bool AnyQueuesCleared = false; - for (auto &Queue : MRecordingQueues) { +void graph_impl::clearQueues(bool NeedsLock) { + graph_impl::RecQueuesStorage ToSwap; + if (NeedsLock) { + graph_impl::WriteLock Lock(MMutex); + std::swap(MRecordingQueues, ToSwap); + } + graph_impl::RecQueuesStorage &QueuesToClear = + NeedsLock ? ToSwap : MRecordingQueues; + + for (auto &Queue : QueuesToClear) { if (auto ValidQueue = Queue.lock(); ValidQueue) { ValidQueue->setCommandGraph(nullptr); - AnyQueuesCleared = true; } } - MRecordingQueues.clear(); - - return AnyQueuesCleared; + QueuesToClear.clear(); } bool graph_impl::checkForCycles() { @@ -1964,8 +1968,7 @@ void modifiable_command_graph::begin_recording( } void modifiable_command_graph::end_recording() { - graph_impl::WriteLock Lock(impl->MMutex); - impl->clearQueues(); + impl->clearQueues(true /*Needs lock*/); } void modifiable_command_graph::end_recording(queue &RecordingQueue) { diff --git a/sycl/source/detail/graph/graph_impl.hpp b/sycl/source/detail/graph/graph_impl.hpp index 10cbbfab0282c..52a910944b908 100644 --- a/sycl/source/detail/graph/graph_impl.hpp +++ b/sycl/source/detail/graph/graph_impl.hpp @@ -188,11 +188,9 @@ class graph_impl : public std::enable_shared_from_this { /// @param RecordingQueue Queue to remove from set. void removeQueue(sycl::detail::queue_impl &RecordingQueue); - /// Remove all queues which are recording to this graph, also sets all queues - /// cleared back to the executing state. + /// Sets all queues cleared back to the executing state. /// - /// @return True if any queues were removed. - bool clearQueues(); + void clearQueues(bool NeedsLock); /// Associate a sycl event with a node in the graph. /// @param EventImpl Event to associate with a node in map. @@ -561,10 +559,12 @@ class graph_impl : public std::enable_shared_from_this { /// Device associated with this graph. All graph nodes will execute on this /// device. sycl::device MDevice; + + using RecQueuesStorage = + std::set, + std::owner_less>>; /// Unique set of queues which are currently recording to this graph. - std::set, - std::owner_less>> - MRecordingQueues; + RecQueuesStorage MRecordingQueues; /// Map of events to their associated recorded nodes. std::unordered_map, node_impl *> MEventsMap; diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 5b7bfb5e90fae..58aed358f3cb0 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -609,7 +609,7 @@ EventImplPtr queue_impl::submit_kernel_direct_impl( KData.getNDRDesc(), std::move(HostKernelPtr), nullptr, // Kernel nullptr, // KernelBundle - std::move(CGData), std::move(KData).getArgs(), + std::move(CGData), std::move(KData.getArgs()), *KData.getDeviceKernelInfoPtr(), std::move(StreamStorage), std::move(AuxiliaryResources), detail::CGType::Kernel, KData.getKernelCacheConfig(), KData.isCooperative(), diff --git a/sycl/source/detail/queue_impl.hpp b/sycl/source/detail/queue_impl.hpp index 7c793b619ecab..b7c74e07c2681 100644 --- a/sycl/source/detail/queue_impl.hpp +++ b/sycl/source/detail/queue_impl.hpp @@ -602,7 +602,8 @@ class queue_impl : public std::enable_shared_from_this { bool CallerNeedsEvent); void setCommandGraphUnlocked( - std::shared_ptr Graph) { + const std::shared_ptr + &Graph) { MGraph = Graph; MExtGraphDeps.reset(); @@ -614,7 +615,8 @@ class queue_impl : public std::enable_shared_from_this { } void setCommandGraph( - std::shared_ptr Graph) { + const std::shared_ptr + &Graph) { std::lock_guard Lock(MMutex); setCommandGraphUnlocked(Graph); } From d9c4e9fb35920afc858dee0bb53684a58a2029bd Mon Sep 17 00:00:00 2001 From: "Tikhomirova, Kseniya" Date: Wed, 12 Nov 2025 04:45:05 -0800 Subject: [PATCH 2/4] return comment back Signed-off-by: Tikhomirova, Kseniya --- sycl/source/detail/graph/graph_impl.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sycl/source/detail/graph/graph_impl.hpp b/sycl/source/detail/graph/graph_impl.hpp index 52a910944b908..d35b271493ed0 100644 --- a/sycl/source/detail/graph/graph_impl.hpp +++ b/sycl/source/detail/graph/graph_impl.hpp @@ -188,8 +188,8 @@ class graph_impl : public std::enable_shared_from_this { /// @param RecordingQueue Queue to remove from set. void removeQueue(sycl::detail::queue_impl &RecordingQueue); - /// Sets all queues cleared back to the executing state. - /// + /// Remove all queues which are recording to this graph, also sets all queues + /// cleared back to the executing state. void clearQueues(bool NeedsLock); /// Associate a sycl event with a node in the graph. From 84fc53ddba2bfd8a2f7e55e87da1f63f27202d8b Mon Sep 17 00:00:00 2001 From: "Tikhomirova, Kseniya" Date: Thu, 13 Nov 2025 02:45:44 -0800 Subject: [PATCH 3/4] revert KData changes Signed-off-by: Tikhomirova, Kseniya --- sycl/source/detail/queue_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/queue_impl.cpp b/sycl/source/detail/queue_impl.cpp index 58aed358f3cb0..5b7bfb5e90fae 100644 --- a/sycl/source/detail/queue_impl.cpp +++ b/sycl/source/detail/queue_impl.cpp @@ -609,7 +609,7 @@ EventImplPtr queue_impl::submit_kernel_direct_impl( KData.getNDRDesc(), std::move(HostKernelPtr), nullptr, // Kernel nullptr, // KernelBundle - std::move(CGData), std::move(KData.getArgs()), + std::move(CGData), std::move(KData).getArgs(), *KData.getDeviceKernelInfoPtr(), std::move(StreamStorage), std::move(AuxiliaryResources), detail::CGType::Kernel, KData.getKernelCacheConfig(), KData.isCooperative(), From 5c2cd7fb153bb6a77bb2e6aff1da69e6a3833f0c Mon Sep 17 00:00:00 2001 From: "Tikhomirova, Kseniya" Date: Thu, 13 Nov 2025 06:47:08 -0800 Subject: [PATCH 4/4] fix comments Signed-off-by: Tikhomirova, Kseniya --- sycl/source/detail/graph/graph_impl.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sycl/source/detail/graph/graph_impl.cpp b/sycl/source/detail/graph/graph_impl.cpp index af10c391963b2..19a60c47dab34 100644 --- a/sycl/source/detail/graph/graph_impl.cpp +++ b/sycl/source/detail/graph/graph_impl.cpp @@ -565,20 +565,20 @@ void graph_impl::removeQueue(sycl::detail::queue_impl &RecordingQueue) { } void graph_impl::clearQueues(bool NeedsLock) { - graph_impl::RecQueuesStorage ToSwap; - if (NeedsLock) { - graph_impl::WriteLock Lock(MMutex); - std::swap(MRecordingQueues, ToSwap); + graph_impl::RecQueuesStorage SwappedQueues; + { + graph_impl::WriteLock Guard(MMutex, std::defer_lock); + if (NeedsLock) { + Guard.lock(); + } + std::swap(MRecordingQueues, SwappedQueues); } - graph_impl::RecQueuesStorage &QueuesToClear = - NeedsLock ? ToSwap : MRecordingQueues; - for (auto &Queue : QueuesToClear) { + for (auto &Queue : SwappedQueues) { if (auto ValidQueue = Queue.lock(); ValidQueue) { ValidQueue->setCommandGraph(nullptr); } } - QueuesToClear.clear(); } bool graph_impl::checkForCycles() {