[Openmp-commits] [PATCH] D153837: Synchronize after each GPU action in the nextgen plugin

Kevin Sala via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Jun 27 01:45:35 PDT 2023

kevinsala added inline comments.

Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:251
+void GenericDeviceTy::checkForForceSynchronize(__tgt_async_info *AsyncInfo) {
+    if (AsyncInfo) {
You can store this envar in the GenericDeviceTy as other envars using `BoolEnvar` or similar.  This way the value will be cached.

Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:257
+               toString(std::move(SyncErr)).data());
+      }
+    }
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.

Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:941
   auto Err = dataSubmitImpl(TgtPtr, HstPtr, Size, AsyncInfoWrapper);
+  checkForForceSynchronize(AsyncInfo);
Could this function could be executed inside the `AsyncInfoWrapper.finalize(Err)`?

Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:535
+  // Error forceSynchronize(__tgt_async_info *AsyncInfo);
+  void checkForForceSynchronize(__tgt_async_info *AsyncInfo);
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.

Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:853
+  //Plugin::check(cuStreamSynchronize(Stream), "Error in stream synchronize for '%s': %s", getName());
+  //cuStreamSynchronize(Stream);
Should be removed

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list