PR response

Author: RossBrunton

offload/liboffload/src/OffloadImpl.cpp

@@ -208,7 +208,7 @@ Error initPlugins(OffloadContext &Context) {
 }
 
 Error olInit_impl() {
-  std::lock_guard<std::mutex> Lock{OffloadContextValMutex};
+  std::lock_guard<std::mutex> Lock(OffloadContextValMutex);
 
   if (isOffloadInitialized()) {
     OffloadContext::get().RefCount++;
@@ -226,7 +226,7 @@ Error olInit_impl() {
 }
 
 Error olShutDown_impl() {
-  std::lock_guard<std::mutex> Lock{OffloadContextValMutex};
+  std::lock_guard<std::mutex> Lock(OffloadContextValMutex);
 
   if (--OffloadContext::get().RefCount != 0)
     return Error::success();
@@ -487,6 +487,8 @@ Error olSyncQueue_impl(ol_queue_handle_t Queue) {
   // Host plugin doesn't have a queue set so it's not safe to call synchronize
   // on it, but we have nothing to synchronize in that situation anyway.
   if (Queue->AsyncInfo->Queue) {
+    // We don't need to release the queue and we would like the ability for
+    // other offload threads to submit work concurrently, so pass "false" here.
     if (auto Err = Queue->Device->Device->synchronize(Queue->AsyncInfo, false))
       return Err;
   }
@@ -735,7 +737,7 @@ Error olGetSymbol_impl(ol_program_handle_t Program, const char *Name,
                        ol_symbol_kind_t Kind, ol_symbol_handle_t *Symbol) {
   auto &Device = Program->Image->getDevice();
 
-  std::lock_guard<std::mutex> Lock{Program->SymbolListMutex};
+  std::lock_guard<std::mutex> Lock(Program->SymbolListMutex);
 
   switch (Kind) {
   case OL_SYMBOL_KIND_KERNEL: {

offload/plugins-nextgen/amdgpu/src/rtl.cpp

@@ -2292,7 +2292,7 @@ struct AMDGPUDeviceTy : public GenericDeviceTy, AMDGenericDeviceTy {
 
   /// Synchronize current thread with the pending operations on the async info.
   Error synchronizeImpl(__tgt_async_info &AsyncInfo,
-                        bool RemoveQueue) override {
+                        bool ReleaseQueue) override {
     AMDGPUStreamTy *Stream =
         reinterpret_cast<AMDGPUStreamTy *>(AsyncInfo.Queue);
     assert(Stream && "Invalid stream");
@@ -2303,7 +2303,7 @@ struct AMDGPUDeviceTy : public GenericDeviceTy, AMDGenericDeviceTy {
     // Once the stream is synchronized, return it to stream pool and reset
     // AsyncInfo. This is to make sure the synchronization only works for its
     // own tasks.
-    if (RemoveQueue) {
+    if (ReleaseQueue) {
       AsyncInfo.Queue = nullptr;
       return AMDGPUStreamManager.returnResource(Stream);
     }

offload/plugins-nextgen/common/include/PluginInterface.h

@@ -843,10 +843,12 @@ struct GenericDeviceTy : public DeviceAllocatorTy {
   Error setupRPCServer(GenericPluginTy &Plugin, DeviceImageTy &Image);
 
   /// Synchronize the current thread with the pending operations on the
-  /// __tgt_async_info structure.
-  Error synchronize(__tgt_async_info *AsyncInfo, bool RemoveQueue = true);
+  /// __tgt_async_info structure. If ReleaseQueue is false, then the
+  // underlying queue will not be released. In this case, additional
+  // work may be submitted to the queue whilst a synchronize is running.
+  Error synchronize(__tgt_async_info *AsyncInfo, bool ReleaseQueue = true);
   virtual Error synchronizeImpl(__tgt_async_info &AsyncInfo,
-                                bool RemoveQueue) = 0;
+                                bool ReleaseQueue) = 0;
 
   /// Invokes any global constructors on the device if present and is required
   /// by the target.

offload/plugins-nextgen/common/src/PluginInterface.cpp

@@ -1336,7 +1336,7 @@ Error PinnedAllocationMapTy::unlockUnmappedHostBuffer(void *HstPtr) {
 }
 
 Error GenericDeviceTy::synchronize(__tgt_async_info *AsyncInfo,
-                                   bool RemoveQueue) {
+                                   bool ReleaseQueue) {
   SmallVector<void *, 2> AllocsToDelete{};
   {
     std::lock_guard<std::mutex> AllocationGuard{AsyncInfo->Mutex};
@@ -1345,7 +1345,7 @@ Error GenericDeviceTy::synchronize(__tgt_async_info *AsyncInfo,
       return Plugin::error(ErrorCode::INVALID_ARGUMENT,
                            "invalid async info queue");
 
-    if (auto Err = synchronizeImpl(*AsyncInfo, RemoveQueue))
+    if (auto Err = synchronizeImpl(*AsyncInfo, ReleaseQueue))
       return Err;
 
     std::swap(AllocsToDelete, AsyncInfo->AssociatedAllocations);

offload/plugins-nextgen/cuda/src/rtl.cpp

@@ -638,15 +638,15 @@ struct CUDADeviceTy : public GenericDeviceTy {
 
   /// Synchronize current thread with the pending operations on the async info.
   Error synchronizeImpl(__tgt_async_info &AsyncInfo,
-                        bool RemoveQueue) override {
+                        bool ReleaseQueue) override {
     CUstream Stream = reinterpret_cast<CUstream>(AsyncInfo.Queue);
     CUresult Res;
     Res = cuStreamSynchronize(Stream);
 
-    // Once the stream is synchronized, return it to stream pool and reset
-    // AsyncInfo. This is to make sure the synchronization only works for its
-    // own tasks.
-    if (RemoveQueue) {
+    // Once the stream is synchronized and we want to release the queue, return
+    // it to stream pool and reset AsyncInfo. This is to make sure the
+    // synchronization only works for its own tasks.
+    if (ReleaseQueue) {
       AsyncInfo.Queue = nullptr;
       if (auto Err = CUDAStreamManager.returnResource(Stream))
         return Err;

offload/plugins-nextgen/host/src/rtl.cpp

@@ -298,7 +298,7 @@ struct GenELF64DeviceTy : public GenericDeviceTy {
   /// All functions are already synchronous. No need to do anything on this
   /// synchronization function.
   Error synchronizeImpl(__tgt_async_info &AsyncInfo,
-                        bool RemoveQueue) override {
+                        bool ReleaseQueue) override {
     return Plugin::success();
   }