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

[Kevin Sala via Phabricator via Openmp-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:251
+void GenericDeviceTy::checkForForceSynchronize(__tgt_async_info *AsyncInfo) {
+  if (std::getenv("LIBOMPTARGET_FORCE_SYNCHRONIZE")) {
+    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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153837/new/

https://reviews.llvm.org/D153837