[Openmp-commits] [PATCH] D96379: [OpenMP] Unify omptarget API and usage wrt. `__tgt_async_info`

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:644
 
-  // We need to synchronize before deallocating data.
-  // If AsyncInfo is nullptr, the previous data transfer (if has) will be
-  // synchronous, so we don't need to synchronize again. If AsyncInfo->Queue is
-  // nullptr, there is no data transfer happened because once there is,
-  // AsyncInfo->Queue will not be nullptr, so again, we don't need to
-  // synchronize.
-  if (AsyncInfo) {
-    Ret = AsyncInfo->synchronize();
-    if (Ret != OFFLOAD_SUCCESS)
-      return OFFLOAD_FAIL;
-  }
+  // TODO: We should not synchronized here but pass the AsyncInfo object to the
+  //       allocate/deallocate device APIs.
----------------
synchronized --> synchronize


================
Comment at: openmp/libomptarget/src/omptarget.cpp:647-649
+  // We need to synchronize before deallocating data. If AsyncInfo is nullptr,
+  // the previous data transfer (if has) will be synchronous, so we don't need
+  // to synchronize again. If AsyncInfo->Queue is nullptr, there is no data
----------------
> If AsyncInfo is nullptr, the previous data transfer (if has) will be synchronous, so we don't need to synchronize again.

I think this sentence is no longer relevant. After this patch, we will always have an AsyncInfo object here.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:649-651
+  // to synchronize again. If AsyncInfo->Queue is nullptr, there is no data
+  // transfer happened because once there is, AsyncInfo->Queue will not be
+  // nullptr, so again, we don't need to synchronize.
----------------
Also, the second part of this comment is confusing. From now on we always call `AsyncInfo.synchronize()`, so we cannot claim that:

> so again, we don't need to synchronize



================
Comment at: openmp/libomptarget/src/omptarget.cpp:1323-1324
   } else {
     // TODO: We should not synchronize here but on the outer level once we pass
-    // in a reference AsyncInfo object.
+    //       in a reference AsyncInfo object.
+    //
----------------
So now this TODO (and the whole else-branch if I understand correctly) is ready to be removed, right? Synchronization takes place at the outer level (in `__tgt_target*`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96379