Synchronize after each GPU action in the nextgen plugin

[efwright]

Summary

Creating a debug option to synchronize GPU kernel launches and data transfers immediately. Done through an environment variable. Currently done in the common plugin interface, so hopefully it would be applicable to all architectures. We instead could do it inside the individual architecture implementations instead, such as having a "cudaStreamSynchronize" call immediately after the "cudaLaunchKernel", for example. Though I think the way I have it now is basically equivalent and doesn't need to be done for each architecture.]

The env variable for now is "LIBOMPTARGET_FORCE_SYNCHRONIZE"

[kevinsala]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

254

You can store this envar in the GenericDeviceTy as other envars using BoolEnvar or similar. This way the value will be cached.

260

I think we should return the error instead of processing it here. The plugin API should be the one reporting the error and perform the necessary actions.

932

Could this function could be executed inside the AsyncInfoWrapper.finalize(Err)?

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h

535 ↗

(On Diff #534851)

If you will keep this function, I would rename it to a clearer name. In this way, it seems like you are just checking if the force synchronization is enabled.

openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp

853 ↗

(On Diff #534851)

Should be removed

[kevinsala]

Please also document the new envar in openmp/docs/design/Runtimes.rst, in the Libomptarget Environment Variables section.

[jdoerfert]

Documentation of the new env var is missing. See openmp/docs/...

[ye-luo]

Please also include the environment variable in the patch summary.

[efwright]

Moved the functionality to the AsyncInfoWrapperTy.finalize() function. Stores the error and it will be returned from the function to reported properly. Looked into the BoolEnvar as recommended. was thinking of storing it in the AsyncInfoWrapperTy class, but since that class gets created and destroyed frequently it's probably a better idea to store it somewhere more permanent.

[jdoerfert]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

216

Are we sure that this has the desired effect? Also, do we need to check for the presence of Queue?

[kevinsala]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

214

We could define this variable as static to initialize just once.

216

We could generalize it and merge with the if below (line 221):

if ((ForceSynchronization || AsyncInfoPtr == &LocalAsyncInfo) && AsyncInfoPtr->Queue && !Err)
  Err = Device.synchronize(AsyncInfoPtr);

[efwright]

Made the env var static and added a check for the queue.

@jdoerfert I have a code where I'm just launching a couple of different kernels back to back, and I at least see the code is synchronizing here. Do you have something specific I could test? Possibly something we know will crash to see if this change helps with the error feedback and what not?

[kevinsala]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

217

I believe the parenthesis in my snippet above are necessary. As the code is now, it would synchronize inconditionally when the new envar is set. However, when there is no queue (stream) or there was a previous error (Err), we shouldn't synchronize. I think the condition should be:

if ((ForceSynchronization || AsyncInfoPtr == &LocalAsyncInfo) && AsyncInfoPtr->Queue && !Err)

Also, the sycnhronize call should be changed to the code below. Otherwise, we will always synchronize with the local async info.

Err = Device.synchronize(AsyncInfoPtr);

[ye-luo]

openmp/docs/design/Runtimes.rst

1248

Please mention the default value.

[ye-luo]

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

953

Better also remove all the added empty lines.

openmp/docs/design/Runtimes.rst

 * ``LIBOMPTARGET_NUM_INITIAL_STREAMS``
 * ``LIBOMPTARGET_NUM_INITIAL_EVENTS``
 * ``LIBOMPTARGET_LOCK_MAPPED_HOST_BUFFERS``
 * ``LIBOMPTARGET_AMDGPU_NUM_HSA_QUEUES``
 * ``LIBOMPTARGET_AMDGPU_HSA_QUEUE_SIZE``
 * ``LIBOMPTARGET_AMDGPU_TEAMS_PER_CU``
 * ``LIBOMPTARGET_AMDGPU_MAX_ASYNC_COPY_BYTES``
 * ``LIBOMPTARGET_AMDGPU_NUM_INITIAL_HSA_SIGNALS``
+* ``LIBOMPTARGET_FORCE_SYNCHRONIZE``
 
 The environment variables ``LIBOMPTARGET_SHARED_MEMORY_SIZE``,
 ``LIBOMPTARGET_STACK_SIZE`` and ``LIBOMPTARGET_HEAP_SIZE`` are described in
 :ref:`libopenmptarget_environment_vars`.
 
 LIBOMPTARGET_NUM_INITIAL_STREAMS
 """"""""""""""""""""""""""""""""
 

 """""""""""""""""""""""""""""""""""""""""""
 
 This environment variable controls the initial number of HSA signals per device
 in the AMDGPU plugin. There is one resource manager of signals per device
 managing several pre-created signals. These signals are mainly used by AMDGPU
 streams. More HSA signals will be created dynamically throughout the execution
 if needed. The default value is ``64``.
 
+LIBOMPTARGET_FORCE_SYNCHRONIZE
+""""""""""""""""""""""""""""""
+
+This environment variable causes the NextGen plugin to synchronize immediately
+after a kernel is launched or after a data transfer, instead of the default
+behavior. Doing so aims to make identifying the source of code crashes
+easier.
+
 .. _remote_offloading_plugin:
 
 Remote Offloading Plugin:
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The remote offloading plugin permits the execution of OpenMP target regions
 on devices in remote hosts in addition to the devices connected to the local
 host. All target devices on the remote host will be exposed to the

openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp

 
 AsyncInfoWrapperTy::AsyncInfoWrapperTy(GenericDeviceTy &Device,
                                        __tgt_async_info *AsyncInfoPtr)
     : Device(Device),
       AsyncInfoPtr(AsyncInfoPtr ? AsyncInfoPtr : &LocalAsyncInfo) {}
 
 void AsyncInfoWrapperTy::finalize(Error &Err) {
   assert(AsyncInfoPtr && "AsyncInfoWrapperTy already finalized");
+  static BoolEnvar ForceSynchronize("LIBOMPTARGET_FORCE_SYNCHRONIZE");
 
   // If we used a local async info object we want synchronous behavior. In that
   // case, and assuming the current status code is correct, we will synchronize
   // explicitly when the object is deleted. Update the error with the result of
   // the synchronize operation.
-  if (AsyncInfoPtr == &LocalAsyncInfo && LocalAsyncInfo.Queue && !Err)
-    Err = Device.synchronize(&LocalAsyncInfo);
+  // Optionally, if the LIBOMPTARGET_FORCE_SYNCHRONIZE env variable is set,
+  // synchronize regardless of the local async info.
+  if ((ForceSynchronize || AsyncInfoPtr == &LocalAsyncInfo) && AsyncInfoPtr->Queue && !Err)
+    Err = Device.synchronize(AsyncInfoPtr);
 
   // Invalidate the wrapper object.
   AsyncInfoPtr = nullptr;
 }
 
 Error GenericKernelTy::init(GenericDeviceTy &GenericDevice,
                             DeviceImageTy &Image) {
   PreferredNumThreads = getDefaultNumThreads(GenericDevice);

 Error GenericKernelTy::printLaunchInfoDetails(GenericDeviceTy &GenericDevice,
                                               KernelArgsTy &KernelArgs,
                                               uint32_t NumThreads,
                                               uint64_t NumBlocks) const {
   return Plugin::success();
 }
 
 Error GenericKernelTy::launch(GenericDeviceTy &GenericDevice, void **ArgPtrs,
                               ptrdiff_t *ArgOffsets, KernelArgsTy &KernelArgs,
                               AsyncInfoWrapperTy &AsyncInfoWrapper) const {
   llvm::SmallVector<void *, 16> Args;
   llvm::SmallVector<void *, 16> Ptrs;
 
   void *KernelArgsPtr = prepareArgs(GenericDevice, ArgPtrs, ArgOffsets,
                                     KernelArgs.NumArgs, Args, Ptrs);
 
   uint32_t NumThreads = getNumThreads(GenericDevice, KernelArgs.ThreadLimit);
   uint64_t NumBlocks = getNumBlocks(GenericDevice, KernelArgs.NumTeams,
                                     KernelArgs.Tripcount, NumThreads);
 
   if (auto Err =
           printLaunchInfo(GenericDevice, KernelArgs, NumThreads, NumBlocks))
     return Err;
Error GenericDeviceTy::dataDelete(void *TgtPtr, TargetAllocTy Kind) {
   return Plugin::success();
 }
 
 Error GenericDeviceTy::dataSubmit(void *TgtPtr, const void *HstPtr,
                                   int64_t Size, __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
   auto Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
+
   AsyncInfoWrapper.finalize(Err);
   return Err;
 }
 
 Error GenericDeviceTy::dataRetrieve(void *HstPtr, const void *TgtPtr,
                                     int64_t Size, __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
   auto Err = dataRetrieveImpl(HstPtr, TgtPtr, Size, AsyncInfoWrapper);
+
   AsyncInfoWrapper.finalize(Err);
   return Err;
 }
 
 Error GenericDeviceTy::dataExchange(const void *SrcPtr, GenericDeviceTy &DstDev,
                                     void *DstPtr, int64_t Size,
                                     __tgt_async_info *AsyncInfo) {
   AsyncInfoWrapperTy AsyncInfoWrapper(*this, AsyncInfo);
 
   auto Err = dataExchangeImpl(SrcPtr, DstDev, DstPtr, Size, AsyncInfoWrapper);
+
   AsyncInfoWrapper.finalize(Err);
   return Err;
 }
 
 Error GenericDeviceTy::launchKernel(void *EntryPtr, void **ArgPtrs,
                                     ptrdiff_t *ArgOffsets,
                                     KernelArgsTy &KernelArgs,
                                     __tgt_async_info *AsyncInfo) {