[Openmp-commits] [PATCH] D132005: [OpenMP] Add non-blocking support for target nowait regions

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

New set of comments, some minor but not all. Some comments are "out-of-order" as I started commenting top-down while not understanding it all. Read accordingly.



================
Comment at: openmp/libomptarget/include/device.h:441
+  /// succeeds/fails. Must be called multiple times until AsyncInfo is
+  /// completed and AsyncInfo.isDone() returns true.
+  int32_t queryAsync(AsyncInfoTy &AsyncInfo);
----------------
Reading this I don't what this returns. SUCCESS if it completed and FAIL otherwise? Or FAIL only if something failed? Must be called multiple times is also unclear to me, I doubt that we should put that sentence here.

Update: I think I now understand the latter part but if I'm right we should change the interface.
So, queryAsync is supposed to be called before isDone to make sure isDone returns the right value, correct? If so, we should not expose queryAsync to the user as there doesn't seem to be a reason to call it otherwise. Arguably, calling it doesn't provide information, just a state change, thus a secondary query is necessary.


================
Comment at: openmp/libomptarget/include/omptarget.h:217
   /// \returns OFFLOAD_FAIL or OFFLOAD_SUCCESS appropriately.
-  int synchronize();
+  int synchronize(SyncType SyncType = SyncType::BLOCKING);
 
----------------
Nit: Rename argument to avoid shadowing.

Make the version that takes the sync type, and probably the sync type, private. IsDone can call the private version, users only the blocking one.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1272
+      // Not ready streams must be considered as successful operations.
+      Err = CUDA_SUCCESS;
+    } else {
----------------
return OFFLOAD_SUCCESS;

will reduce indention and logic later on.


================
Comment at: openmp/libomptarget/plugins/cuda/src/rtl.cpp:1278
+      StreamPool[DeviceId]->release(
+          reinterpret_cast<CUstream>(AsyncInfo->Queue));
+      AsyncInfo->Queue = nullptr;
----------------



================
Comment at: openmp/libomptarget/src/interface.cpp:86
+  int Rc = OFFLOAD_SUCCESS;
+  if (Dispatch)
+    Rc = TargetDataFunction(Loc, Device, ArgNum, ArgsBase, Args, ArgSizes,
----------------
This is different but similar to the condition used in the other new "helper" below. I have the same concerns as there. When would we ever not call the target data function?


================
Comment at: openmp/libomptarget/src/interface.cpp:95
+  handleTargetOutcome(Rc == OFFLOAD_SUCCESS, Loc);
+}
+
----------------
Is `FromMapper` ever set to true? Did I miss that?


================
Comment at: openmp/libomptarget/src/interface.cpp:252
+             AsyncInfoTy::SyncType SyncType = AsyncInfoTy::SyncType::BLOCKING,
+             bool Dispatch = true) {
   TIMESCOPE_WITH_IDENT(Loc);
----------------
There is more duplication in the callees to be moved here, no?
The two last arguments could be omitted and grabbed from AsyncInfo, also in the above rewrite.
Dispatch is not used?


================
Comment at: openmp/libomptarget/src/interface.cpp:279
+  int Rc = OFFLOAD_SUCCESS;
+  if (AsyncInfo.isDone())
+    Rc = target(Loc, Device, HostPtr, Args->NumArgs, Args->ArgBasePtrs,
----------------
This I don't understand. Why do we have to wait to enqueue the kernel? And even if, how does this not accidentally skip the target region and we will never execute it at all? Long story short, I doubt the conditional here makes sense.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:41-50
+
+  // Run any pending post-processing function registered on this async object.
+  if (Result == OFFLOAD_SUCCESS && isDone()) {
+    for (auto &PostProcFunc : PostProcessingFunctions) {
+      Result = PostProcFunc();
+      if (Result != OFFLOAD_SUCCESS)
+        break;
----------------
This would move to isDone below.


================
Comment at: openmp/libomptarget/src/private.h:227
+
+    delete AsyncInfo;
+    __kmpc_omp_set_target_async_handle(ExecThreadID, NULL);
----------------
Should this be guarded by IsNew?


================
Comment at: openmp/libomptarget/src/private.h:240
+  /// synchronization is needed.
+  bool shouldDispatch() { return !IsNew; }
+};
----------------
I don't understand what we want/need this dispatch idea for. It seems to skip operations but I don't understand how we would not forget about them and go back. 


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:5186
+ at param gtid Global Thread ID of current thread
+ at return Returns true if the current thread of the given thread has a task team
+allocated to it.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005