[llvm] [Offload] Improve `olDestroyQueue` logic (PR #153041)
Ross Brunton via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 26 03:58:53 PDT 2025
https://github.com/RossBrunton updated https://github.com/llvm/llvm-project/pull/153041
>From 53469bfaa6a5979960e7487045ea783bd7a887dc Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Mon, 11 Aug 2025 16:51:30 +0100
Subject: [PATCH 1/8] [Offload] Improve `olDestroyQueue` logic
Previously, `olDestroyQueue` would not actually destroy the queue,
instead leaving it for the device to clean up when it was destroyed.
Now, the queue is either released immediately if it is complete or put
into a list of "pending" queues if it is not. Whenever we create a new
queue, we check this list to see if any are now completed. If there are
any we release their resources and use them instead of pulling from
the pool.
This prevents long running programs that create and drop many queues
without syncing them from leaking memory all over the place.
---
offload/liboffload/src/OffloadImpl.cpp | 69 ++++++++++++++++++-
.../common/src/PluginInterface.cpp | 15 ++--
2 files changed, 76 insertions(+), 8 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index 9d342e06127a2..d5186172f42a4 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -43,6 +43,17 @@ using namespace error;
// we add some additional data here for now to avoid churn in the plugin
// interface.
struct ol_device_impl_t {
+ // std::mutex doesn't have a move constructor, so we need to create a new one
+ // in the destination. Since devices don't get moved once the platform has
+ // been created, this is safe.
+ ol_device_impl_t(ol_device_impl_t &&Other)
+ : DeviceNum(Other.DeviceNum), Device(Other.Device),
+ Platform(Other.Platform), Info(std::move(Other.Info)),
+ OutstandingQueues(), OutstandingQueuesMutex() {
+ assert(Other.OutstandingQueuesMutex.try_lock());
+ assert(!Other.OutstandingQueues.size());
+ }
+
ol_device_impl_t(int DeviceNum, GenericDeviceTy *Device,
ol_platform_handle_t Platform, InfoTreeNode &&DevInfo)
: DeviceNum(DeviceNum), Device(Device), Platform(Platform),
@@ -51,6 +62,9 @@ struct ol_device_impl_t {
GenericDeviceTy *Device;
ol_platform_handle_t Platform;
InfoTreeNode Info;
+
+ llvm::SmallVector<__tgt_async_info *> OutstandingQueues;
+ std::mutex OutstandingQueuesMutex;
};
struct ol_platform_impl_t {
@@ -566,14 +580,65 @@ Error olMemFree_impl(void *Address) {
Error olCreateQueue_impl(ol_device_handle_t Device, ol_queue_handle_t *Queue) {
auto CreatedQueue = std::make_unique<ol_queue_impl_t>(nullptr, Device);
- if (auto Err = Device->Device->initAsyncInfo(&(CreatedQueue->AsyncInfo)))
+
+ // The device may have some outstanding queues created by olDestroyQueue,
+ // check if any of those are finished and can be reused.
+ // Not locking the `size()` access is fine here - In the worst case we either
+ // miss a queue that exists or loop through an empty array after taking the
+ // lock. Both are sub-optimal but not that bad.
+ __tgt_async_info *OutstandingQueue = nullptr;
+ if (Device->OutstandingQueues.size()) {
+ std::lock_guard<std::mutex> Lock(Device->OutstandingQueuesMutex);
+
+ // As queues are pulled and popped from this list, longer running queues
+ // naturally bubble to the start of the array. Hence looping backwards.
+ for (auto Q = Device->OutstandingQueues.rbegin();
+ Q != Device->OutstandingQueues.rend(); Q++) {
+ if (!Device->Device->hasPendingWork(*Q)) {
+ OutstandingQueue = *Q;
+ *Q = Device->OutstandingQueues.back();
+ Device->OutstandingQueues.pop_back();
+ }
+ }
+ }
+
+ if (OutstandingQueue) {
+ // The queue is empty, but we still need to sync it to release any temporary
+ // memory allocations or do other cleanup
+ if (auto Err =
+ Device->Device->synchronize(OutstandingQueue, /*Release=*/false))
+ return Err;
+ CreatedQueue->AsyncInfo = OutstandingQueue;
+ } else if (auto Err =
+ Device->Device->initAsyncInfo(&(CreatedQueue->AsyncInfo))) {
return Err;
+ }
*Queue = CreatedQueue.release();
return Error::success();
}
-Error olDestroyQueue_impl(ol_queue_handle_t Queue) { return olDestroy(Queue); }
+Error olDestroyQueue_impl(ol_queue_handle_t Queue) {
+ // This is safe; as soon as olDestroyQueue is called it is not possible to add
+ // any more work to the queue, so if it's finished now it will remain finished
+ // forever.
+ auto Res = Queue->Device->Device->hasPendingWork(Queue->AsyncInfo);
+ if (!Res)
+ return Res.takeError();
+
+ if (!*Res) {
+ // The queue is complete, so sync it and throw it back into the pool
+ if (auto Err = Queue->Device->Device->synchronize(Queue->AsyncInfo,
+ /*Release=*/true))
+ return Err;
+ } else {
+ // The queue still has outstanding work. Store it so we can check it later
+ std::lock_guard<std::mutex> Lock(Queue->Device->OutstandingQueuesMutex);
+ Queue->Device->OutstandingQueues.push_back(Queue->AsyncInfo);
+ }
+
+ return olDestroy(Queue);
+}
Error olSyncQueue_impl(ol_queue_handle_t Queue) {
// Host plugin doesn't have a queue set so it's not safe to call synchronize
diff --git a/offload/plugins-nextgen/common/src/PluginInterface.cpp b/offload/plugins-nextgen/common/src/PluginInterface.cpp
index d4b5f914c6672..0acd532fb3463 100644
--- a/offload/plugins-nextgen/common/src/PluginInterface.cpp
+++ b/offload/plugins-nextgen/common/src/PluginInterface.cpp
@@ -1337,16 +1337,19 @@ Error PinnedAllocationMapTy::unlockUnmappedHostBuffer(void *HstPtr) {
Error GenericDeviceTy::synchronize(__tgt_async_info *AsyncInfo,
bool ReleaseQueue) {
+ if (!AsyncInfo)
+ return Plugin::error(ErrorCode::INVALID_ARGUMENT,
+ "invalid async info queue");
+
SmallVector<void *> AllocsToDelete{};
{
std::lock_guard<std::mutex> AllocationGuard{AsyncInfo->Mutex};
- if (!AsyncInfo || !AsyncInfo->Queue)
- return Plugin::error(ErrorCode::INVALID_ARGUMENT,
- "invalid async info queue");
-
- if (auto Err = synchronizeImpl(*AsyncInfo, ReleaseQueue))
- return Err;
+ // This can be false when no work has been added to the AsyncInfo. In which
+ // case, the device has nothing to synchronize.
+ if (AsyncInfo->Queue)
+ if (auto Err = synchronizeImpl(*AsyncInfo, ReleaseQueue))
+ return Err;
std::swap(AllocsToDelete, AsyncInfo->AssociatedAllocations);
}
>From 52ca4b80bd22c4e877022302ed837c28aac4ec48 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Tue, 12 Aug 2025 10:22:19 +0100
Subject: [PATCH 2/8] Store devices as pointers rather than in the array
directly
---
offload/liboffload/src/OffloadImpl.cpp | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index d5186172f42a4..76516c1957771 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -43,17 +43,6 @@ using namespace error;
// we add some additional data here for now to avoid churn in the plugin
// interface.
struct ol_device_impl_t {
- // std::mutex doesn't have a move constructor, so we need to create a new one
- // in the destination. Since devices don't get moved once the platform has
- // been created, this is safe.
- ol_device_impl_t(ol_device_impl_t &&Other)
- : DeviceNum(Other.DeviceNum), Device(Other.Device),
- Platform(Other.Platform), Info(std::move(Other.Info)),
- OutstandingQueues(), OutstandingQueuesMutex() {
- assert(Other.OutstandingQueuesMutex.try_lock());
- assert(!Other.OutstandingQueues.size());
- }
-
ol_device_impl_t(int DeviceNum, GenericDeviceTy *Device,
ol_platform_handle_t Platform, InfoTreeNode &&DevInfo)
: DeviceNum(DeviceNum), Device(Device), Platform(Platform),
@@ -72,7 +61,7 @@ struct ol_platform_impl_t {
ol_platform_backend_t BackendType)
: Plugin(std::move(Plugin)), BackendType(BackendType) {}
std::unique_ptr<GenericPluginTy> Plugin;
- std::vector<ol_device_impl_t> Devices;
+ llvm::SmallVector<std::unique_ptr<ol_device_impl_t>> Devices;
ol_platform_backend_t BackendType;
};
@@ -145,7 +134,7 @@ struct OffloadContext {
ol_device_handle_t HostDevice() {
// The host platform is always inserted last
- return &Platforms.back().Devices[0];
+ return Platforms.back().Devices[0].get();
}
static OffloadContext &get() {
@@ -204,8 +193,8 @@ Error initPlugins(OffloadContext &Context) {
auto Info = Device->obtainInfoImpl();
if (auto Err = Info.takeError())
return Err;
- Platform.Devices.emplace_back(DevNum, Device, &Platform,
- std::move(*Info));
+ Platform.Devices.emplace_back(std::make_unique<ol_device_impl_t>(
+ DevNum, Device, &Platform, std::move(*Info)));
}
}
}
@@ -213,7 +202,8 @@ Error initPlugins(OffloadContext &Context) {
// Add the special host device
auto &HostPlatform = Context.Platforms.emplace_back(
ol_platform_impl_t{nullptr, OL_PLATFORM_BACKEND_HOST});
- HostPlatform.Devices.emplace_back(-1, nullptr, nullptr, InfoTreeNode{});
+ HostPlatform.Devices.emplace_back(
+ std::make_unique<ol_device_impl_t>(-1, nullptr, nullptr, InfoTreeNode{}));
Context.HostDevice()->Platform = &HostPlatform;
Context.TracingEnabled = std::getenv("OFFLOAD_TRACE");
@@ -519,7 +509,7 @@ Error olGetDeviceInfoSize_impl(ol_device_handle_t Device,
Error olIterateDevices_impl(ol_device_iterate_cb_t Callback, void *UserData) {
for (auto &Platform : OffloadContext::get().Platforms) {
for (auto &Device : Platform.Devices) {
- if (!Callback(&Device, UserData)) {
+ if (!Callback(Device.get(), UserData)) {
break;
}
}
>From 9e5ee3e7040542aa658d006e8f000d31e25ffd49 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Tue, 12 Aug 2025 10:57:37 +0100
Subject: [PATCH 3/8] Refactoring in response to code review
---
offload/liboffload/src/OffloadImpl.cpp | 97 +++++++++++++++++++-------
1 file changed, 70 insertions(+), 27 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index 76516c1957771..9b5e5632445ca 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -47,6 +47,12 @@ struct ol_device_impl_t {
ol_platform_handle_t Platform, InfoTreeNode &&DevInfo)
: DeviceNum(DeviceNum), Device(Device), Platform(Platform),
Info(std::forward<InfoTreeNode>(DevInfo)) {}
+
+ ~ol_device_impl_t() {
+ assert(!OutstandingQueues.size() &&
+ "Device object dropped with outstanding queues");
+ }
+
int DeviceNum;
GenericDeviceTy *Device;
ol_platform_handle_t Platform;
@@ -54,6 +60,46 @@ struct ol_device_impl_t {
llvm::SmallVector<__tgt_async_info *> OutstandingQueues;
std::mutex OutstandingQueuesMutex;
+
+ /// If the device has any outstanding queues that are now complete, remove it
+ /// from the list and return it.
+ ///
+ /// Queues may be added to the outstanding queue list by olDestroyQueue if
+ /// they are destroyed but not completed.
+ std::optional<__tgt_async_info *> GetOutstandingQueue() {
+ // Not locking the `size()` access is fine here - In the worst case we
+ // either miss a queue that exists or loop through an empty array after
+ // taking the lock. Both are sub-optimal but not that bad.
+ if (OutstandingQueues.size()) {
+ std::lock_guard<std::mutex> Lock(OutstandingQueuesMutex);
+
+ // As queues are pulled and popped from this list, longer running queues
+ // naturally bubble to the start of the array. Hence looping backwards.
+ for (auto Q = OutstandingQueues.rbegin(); Q != OutstandingQueues.rend();
+ Q++) {
+ if (!Device->hasPendingWork(*Q)) {
+ auto OutstandingQueue = *Q;
+ *Q = OutstandingQueues.back();
+ OutstandingQueues.pop_back();
+ return OutstandingQueue;
+ }
+ }
+ }
+ return std::nullopt;
+ }
+
+ /// Complete all pending work for this device and perform any needed cleanup.
+ ///
+ /// After calling this function, no liboffload functions should be called with
+ /// this device handle.
+ llvm::Error Destroy() {
+ llvm::Error Result = Plugin::success();
+ for (auto Q : OutstandingQueues)
+ if (auto Err = Device->synchronize(Q, /*Release=*/true))
+ Result = llvm::joinErrors(std::move(Result), std::move(Err));
+ OutstandingQueues.clear();
+ return Result;
+ }
};
struct ol_platform_impl_t {
@@ -63,6 +109,23 @@ struct ol_platform_impl_t {
std::unique_ptr<GenericPluginTy> Plugin;
llvm::SmallVector<std::unique_ptr<ol_device_impl_t>> Devices;
ol_platform_backend_t BackendType;
+
+ /// Complete all pending work for this platform and perform any needed
+ /// cleanup.
+ ///
+ /// After calling this function, no liboffload functions should be called with
+ /// this platform handle.
+ llvm::Error Destroy() {
+ llvm::Error Result = Plugin::success();
+ for (auto &D : Devices)
+ if (auto Err = D->Destroy())
+ Result = llvm::joinErrors(std::move(Result), std::move(Err));
+
+ if (auto Res = Plugin->deinit())
+ Result = llvm::joinErrors(std::move(Result), std::move(Res));
+
+ return Result;
+ }
};
struct ol_queue_impl_t {
@@ -244,7 +307,7 @@ Error olShutDown_impl() {
if (!P.Plugin || !P.Plugin->is_initialized())
continue;
- if (auto Res = P.Plugin->deinit())
+ if (auto Res = P.Destroy())
Result = llvm::joinErrors(std::move(Result), std::move(Res));
}
@@ -571,34 +634,14 @@ Error olMemFree_impl(void *Address) {
Error olCreateQueue_impl(ol_device_handle_t Device, ol_queue_handle_t *Queue) {
auto CreatedQueue = std::make_unique<ol_queue_impl_t>(nullptr, Device);
- // The device may have some outstanding queues created by olDestroyQueue,
- // check if any of those are finished and can be reused.
- // Not locking the `size()` access is fine here - In the worst case we either
- // miss a queue that exists or loop through an empty array after taking the
- // lock. Both are sub-optimal but not that bad.
- __tgt_async_info *OutstandingQueue = nullptr;
- if (Device->OutstandingQueues.size()) {
- std::lock_guard<std::mutex> Lock(Device->OutstandingQueuesMutex);
-
- // As queues are pulled and popped from this list, longer running queues
- // naturally bubble to the start of the array. Hence looping backwards.
- for (auto Q = Device->OutstandingQueues.rbegin();
- Q != Device->OutstandingQueues.rend(); Q++) {
- if (!Device->Device->hasPendingWork(*Q)) {
- OutstandingQueue = *Q;
- *Q = Device->OutstandingQueues.back();
- Device->OutstandingQueues.pop_back();
- }
- }
- }
-
+ auto OutstandingQueue = Device->GetOutstandingQueue();
if (OutstandingQueue) {
// The queue is empty, but we still need to sync it to release any temporary
- // memory allocations or do other cleanup
+ // memory allocations or do other cleanup.
if (auto Err =
- Device->Device->synchronize(OutstandingQueue, /*Release=*/false))
+ Device->Device->synchronize(*OutstandingQueue, /*Release=*/false))
return Err;
- CreatedQueue->AsyncInfo = OutstandingQueue;
+ CreatedQueue->AsyncInfo = *OutstandingQueue;
} else if (auto Err =
Device->Device->initAsyncInfo(&(CreatedQueue->AsyncInfo))) {
return Err;
@@ -617,12 +660,12 @@ Error olDestroyQueue_impl(ol_queue_handle_t Queue) {
return Res.takeError();
if (!*Res) {
- // The queue is complete, so sync it and throw it back into the pool
+ // The queue is complete, so sync it and throw it back into the pool.
if (auto Err = Queue->Device->Device->synchronize(Queue->AsyncInfo,
/*Release=*/true))
return Err;
} else {
- // The queue still has outstanding work. Store it so we can check it later
+ // The queue still has outstanding work. Store it so we can check it later.
std::lock_guard<std::mutex> Lock(Queue->Device->OutstandingQueuesMutex);
Queue->Device->OutstandingQueues.push_back(Queue->AsyncInfo);
}
>From 95314a6827df0e5ed8144545abef5cf3a0b17e20 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Fri, 15 Aug 2025 09:29:21 +0100
Subject: [PATCH 4/8] Respond to feedback
---
offload/liboffload/src/OffloadImpl.cpp | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index 9b5e5632445ca..46560eccb687b 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -66,7 +66,7 @@ struct ol_device_impl_t {
///
/// Queues may be added to the outstanding queue list by olDestroyQueue if
/// they are destroyed but not completed.
- std::optional<__tgt_async_info *> GetOutstandingQueue() {
+ __tgt_async_info *getOutstandingQueue() {
// Not locking the `size()` access is fine here - In the worst case we
// either miss a queue that exists or loop through an empty array after
// taking the lock. Both are sub-optimal but not that bad.
@@ -85,14 +85,14 @@ struct ol_device_impl_t {
}
}
}
- return std::nullopt;
+ return nullptr;
}
/// Complete all pending work for this device and perform any needed cleanup.
///
/// After calling this function, no liboffload functions should be called with
/// this device handle.
- llvm::Error Destroy() {
+ llvm::Error destroy() {
llvm::Error Result = Plugin::success();
for (auto Q : OutstandingQueues)
if (auto Err = Device->synchronize(Q, /*Release=*/true))
@@ -115,10 +115,10 @@ struct ol_platform_impl_t {
///
/// After calling this function, no liboffload functions should be called with
/// this platform handle.
- llvm::Error Destroy() {
+ llvm::Error destroy() {
llvm::Error Result = Plugin::success();
for (auto &D : Devices)
- if (auto Err = D->Destroy())
+ if (auto Err = D->destroy())
Result = llvm::joinErrors(std::move(Result), std::move(Err));
if (auto Res = Plugin->deinit())
@@ -307,7 +307,7 @@ Error olShutDown_impl() {
if (!P.Plugin || !P.Plugin->is_initialized())
continue;
- if (auto Res = P.Destroy())
+ if (auto Res = P.destroy())
Result = llvm::joinErrors(std::move(Result), std::move(Res));
}
@@ -634,14 +634,14 @@ Error olMemFree_impl(void *Address) {
Error olCreateQueue_impl(ol_device_handle_t Device, ol_queue_handle_t *Queue) {
auto CreatedQueue = std::make_unique<ol_queue_impl_t>(nullptr, Device);
- auto OutstandingQueue = Device->GetOutstandingQueue();
+ auto OutstandingQueue = Device->getOutstandingQueue();
if (OutstandingQueue) {
// The queue is empty, but we still need to sync it to release any temporary
// memory allocations or do other cleanup.
if (auto Err =
- Device->Device->synchronize(*OutstandingQueue, /*Release=*/false))
+ Device->Device->synchronize(OutstandingQueue, /*Release=*/false))
return Err;
- CreatedQueue->AsyncInfo = *OutstandingQueue;
+ CreatedQueue->AsyncInfo = OutstandingQueue;
} else if (auto Err =
Device->Device->initAsyncInfo(&(CreatedQueue->AsyncInfo))) {
return Err;
>From 4b49fbdc75f5df93d0ec340c8941fc66ae94b592 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Fri, 15 Aug 2025 11:10:11 +0100
Subject: [PATCH 5/8] Add test for handling events after queue destruction and
fix the issues that came up from this.
---
offload/liboffload/src/OffloadImpl.cpp | 25 +++++++++++++------
.../OffloadAPI/queue/olDestroyQueue.cpp | 9 +++++++
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index 46560eccb687b..d35b2e5f959e6 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -130,17 +130,28 @@ struct ol_platform_impl_t {
struct ol_queue_impl_t {
ol_queue_impl_t(__tgt_async_info *AsyncInfo, ol_device_handle_t Device)
- : AsyncInfo(AsyncInfo), Device(Device) {}
+ : AsyncInfo(AsyncInfo), Device(Device), Id(IdCounter++) {}
__tgt_async_info *AsyncInfo;
ol_device_handle_t Device;
+ // A unique identifier for the queue
+ size_t Id;
+ static std::atomic<size_t> IdCounter;
};
+std::atomic<size_t> ol_queue_impl_t::IdCounter;
struct ol_event_impl_t {
- ol_event_impl_t(void *EventInfo, ol_queue_handle_t Queue)
- : EventInfo(EventInfo), Queue(Queue) {}
+ ol_event_impl_t(void *EventInfo, ol_device_handle_t Device,
+ ol_queue_handle_t Queue)
+ : EventInfo(EventInfo), Device(Device), QueueId(Queue->Id), Queue(Queue) {
+ }
// EventInfo may be null, in which case the event should be considered always
// complete
void *EventInfo;
+ ol_device_handle_t Device;
+ size_t QueueId;
+ // Events may outlive the queue - don't assume this is always valid
+ // It is provided only to implement OL_EVENT_INFO_QUEUE. Use QueueId to check
+ // for queue equality instead
ol_queue_handle_t Queue;
};
@@ -699,7 +710,7 @@ Error olWaitEvents_impl(ol_queue_handle_t Queue, ol_event_handle_t *Events,
"olWaitEvents asked to wait on a NULL event");
// Do nothing if the event is for this queue or the event is always complete
- if (Event->Queue == Queue || !Event->EventInfo)
+ if (Event->QueueId == Queue->Id || !Event->EventInfo)
continue;
if (auto Err = Device->waitEvent(Event->EventInfo, Queue->AsyncInfo))
@@ -747,7 +758,7 @@ Error olSyncEvent_impl(ol_event_handle_t Event) {
if (!Event->EventInfo)
return Plugin::success();
- if (auto Res = Event->Queue->Device->Device->syncEvent(Event->EventInfo))
+ if (auto Res = Event->Device->Device->syncEvent(Event->EventInfo))
return Res;
return Error::success();
@@ -755,7 +766,7 @@ Error olSyncEvent_impl(ol_event_handle_t Event) {
Error olDestroyEvent_impl(ol_event_handle_t Event) {
if (Event->EventInfo)
- if (auto Res = Event->Queue->Device->Device->destroyEvent(Event->EventInfo))
+ if (auto Res = Event->Device->Device->destroyEvent(Event->EventInfo))
return Res;
return olDestroy(Event);
@@ -806,7 +817,7 @@ Error olCreateEvent_impl(ol_queue_handle_t Queue, ol_event_handle_t *EventOut) {
if (auto Err = Pending.takeError())
return Err;
- *EventOut = new ol_event_impl_t(nullptr, Queue);
+ *EventOut = new ol_event_impl_t(nullptr, Queue->Device, Queue);
if (!*Pending)
// Queue is empty, don't record an event and consider the event always
// complete
diff --git a/offload/unittests/OffloadAPI/queue/olDestroyQueue.cpp b/offload/unittests/OffloadAPI/queue/olDestroyQueue.cpp
index 0dc8527df5323..aa9e372ede2c8 100644
--- a/offload/unittests/OffloadAPI/queue/olDestroyQueue.cpp
+++ b/offload/unittests/OffloadAPI/queue/olDestroyQueue.cpp
@@ -18,6 +18,15 @@ TEST_P(olDestroyQueueTest, Success) {
Queue = nullptr;
}
+TEST_P(olDestroyQueueTest, SuccessDelayedResolution) {
+ ManuallyTriggeredTask Manual;
+ ASSERT_SUCCESS(Manual.enqueue(Queue));
+ ASSERT_SUCCESS(olDestroyQueue(Queue));
+ Queue = nullptr;
+
+ ASSERT_SUCCESS(Manual.trigger());
+}
+
TEST_P(olDestroyQueueTest, InvalidNullHandle) {
ASSERT_ERROR(OL_ERRC_INVALID_NULL_HANDLE, olDestroyQueue(nullptr));
}
>From dca7a47430691ecb67cf23e5e840adf45a61ca76 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Mon, 18 Aug 2025 10:21:24 +0100
Subject: [PATCH 6/8] Comment format fixes
---
offload/liboffload/src/OffloadImpl.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index d35b2e5f959e6..21298932060e6 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -149,9 +149,9 @@ struct ol_event_impl_t {
void *EventInfo;
ol_device_handle_t Device;
size_t QueueId;
- // Events may outlive the queue - don't assume this is always valid
+ // Events may outlive the queue - don't assume this is always valid.
// It is provided only to implement OL_EVENT_INFO_QUEUE. Use QueueId to check
- // for queue equality instead
+ // for queue equality instead.
ol_queue_handle_t Queue;
};
>From 057fa6480d556ac8258839244705d93bda0638c1 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Tue, 19 Aug 2025 09:46:52 +0100
Subject: [PATCH 7/8] Respond to feedback
---
offload/liboffload/src/OffloadImpl.cpp | 13 +++++++------
offload/unittests/OffloadAPI/common/Fixtures.hpp | 1 +
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index 21298932060e6..a50f59a99daea 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -137,7 +137,7 @@ struct ol_queue_impl_t {
size_t Id;
static std::atomic<size_t> IdCounter;
};
-std::atomic<size_t> ol_queue_impl_t::IdCounter;
+std::atomic<size_t> ol_queue_impl_t::IdCounter(0);
struct ol_event_impl_t {
ol_event_impl_t(void *EventInfo, ol_device_handle_t Device,
@@ -663,22 +663,23 @@ Error olCreateQueue_impl(ol_device_handle_t Device, ol_queue_handle_t *Queue) {
}
Error olDestroyQueue_impl(ol_queue_handle_t Queue) {
+ auto *Device = Queue->Device;
// This is safe; as soon as olDestroyQueue is called it is not possible to add
// any more work to the queue, so if it's finished now it will remain finished
// forever.
- auto Res = Queue->Device->Device->hasPendingWork(Queue->AsyncInfo);
+ auto Res = Device->Device->hasPendingWork(Queue->AsyncInfo);
if (!Res)
return Res.takeError();
if (!*Res) {
// The queue is complete, so sync it and throw it back into the pool.
- if (auto Err = Queue->Device->Device->synchronize(Queue->AsyncInfo,
- /*Release=*/true))
+ if (auto Err = Device->Device->synchronize(Queue->AsyncInfo,
+ /*Release=*/true))
return Err;
} else {
// The queue still has outstanding work. Store it so we can check it later.
- std::lock_guard<std::mutex> Lock(Queue->Device->OutstandingQueuesMutex);
- Queue->Device->OutstandingQueues.push_back(Queue->AsyncInfo);
+ std::lock_guard<std::mutex> Lock(Device->OutstandingQueuesMutex);
+ Device->OutstandingQueues.push_back(Queue->AsyncInfo);
}
return olDestroy(Queue);
diff --git a/offload/unittests/OffloadAPI/common/Fixtures.hpp b/offload/unittests/OffloadAPI/common/Fixtures.hpp
index 0538e60f276e3..b0153c2f65110 100644
--- a/offload/unittests/OffloadAPI/common/Fixtures.hpp
+++ b/offload/unittests/OffloadAPI/common/Fixtures.hpp
@@ -8,6 +8,7 @@
#include <OffloadAPI.h>
#include <OffloadPrint.hpp>
+#include <condition_variable>
#include <gtest/gtest.h>
#include <thread>
>From 1b4b6be119d4e3e46bba2499f2b30e68b0416b46 Mon Sep 17 00:00:00 2001
From: Ross Brunton <ross at codeplay.com>
Date: Tue, 26 Aug 2025 11:58:35 +0100
Subject: [PATCH 8/8] Remove import
---
offload/unittests/OffloadAPI/common/Fixtures.hpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/offload/unittests/OffloadAPI/common/Fixtures.hpp b/offload/unittests/OffloadAPI/common/Fixtures.hpp
index b0153c2f65110..0538e60f276e3 100644
--- a/offload/unittests/OffloadAPI/common/Fixtures.hpp
+++ b/offload/unittests/OffloadAPI/common/Fixtures.hpp
@@ -8,7 +8,6 @@
#include <OffloadAPI.h>
#include <OffloadPrint.hpp>
-#include <condition_variable>
#include <gtest/gtest.h>
#include <thread>
More information about the llvm-commits
mailing list