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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D132005#3871439 <https://reviews.llvm.org/D132005#3871439>, @gValarini wrote:

> @jdoerfert I believe the new revision has a better code structure for the dispatch and synchronize stages. Now we have an exclusive function only for synchronization. No more `Dispatch` checks!

Great, I think we are almost there. The code looks much cleaner and the approach is much clearer than it was in the beginning (IMHO, I hope people agree).

I left some final clarification questions and some cleanup requests.

One conceptual question which is not blocking the patch though:
 Does this approach require hidden helper threads to execute the target task or could we enable it for non-hidden helper threads as well?



================
Comment at: openmp/libomptarget/include/omptarget.h:234
+  /// \note if the operations are completed, the registered post-processing
+  /// functions will be executed.
+  ///
----------------
Make it clear that this happens only once. Either here or via synchronize. Right now it could be read like every isDone call might invoke the post-processing functions.


================
Comment at: openmp/libomptarget/include/omptarget.h:414
+// and a value different than zero otherwise.
+void __tgt_target_nowait_query(void **AsyncHandle);
+
----------------
It's void return but comment talks about the return value.


================
Comment at: openmp/libomptarget/src/interface.cpp:64
 
+static inline void targetDataMapper(
+    ident_t *Loc, DeviceTy &Device, int64_t DeviceId, int32_t ArgNum,
----------------
If you make this a templated function accepting the (sub)type of the AsyncInfo object instead of the object itself, you can move all the remaining duplication at the call sites (namely: checkCtorDtor, get device, create AsyncInfo) into this function. WDYT?


================
Comment at: openmp/libomptarget/src/interface.cpp:241
 
-/// Implements a kernel entry that executes the target region on the specified
-/// device.
-///
-/// \param Loc Source location associated with this target region.
-/// \param DeviceId The device to execute this region, -1 indicated the default.
-/// \param NumTeams Number of teams to launch the region with, -1 indicates a
-///                 non-teams region and 0 indicates it was unspecified.
-/// \param ThreadLimit Limit to the number of threads to use in the kernel
-///                    launch, 0 indicates it was unspecified.
-/// \param HostPtr  The pointer to the host function registered with the kernel.
-/// \param Args     All arguments to this kernel launch (see struct definition).
-EXTERN int __tgt_target_kernel(ident_t *Loc, int64_t DeviceId, int32_t NumTeams,
-                               int32_t ThreadLimit, void *HostPtr,
-                               __tgt_kernel_arguments *Args) {
+static inline int targetKernel(ident_t *Loc, DeviceTy &Device, int64_t DeviceId,
+                               int32_t NumTeams, int32_t ThreadLimit,
----------------
Same comment as above wrt. templated version. The duplication we introduce is something I would like to avoid.


================
Comment at: openmp/libomptarget/src/interface.cpp:382
+           "this a target nowait region?\n");
+    handleTargetOutcome(false, nullptr);
+  }
----------------
Do we know the above call is "noreturn"? If not, we should explicitly exit here. On second thought, we should exit either way.


================
Comment at: openmp/libomptarget/src/interface.cpp:385
+
+  auto AsyncInfo = (AsyncInfoTy *)*AsyncHandle;
+
----------------



================
Comment at: openmp/libomptarget/src/omptarget.cpp:73
+  SmallVector<PostProcFuncTy> Functions(
+      std::move(PostProcessingFunctions));
+
----------------
This is not a good idea, I think. The state of PostProcessingFunctions is undefined afterwards, even if this works in practice. Simply iterate PostProcessingFunctions and then clear it.


================
Comment at: openmp/libomptarget/src/private.h:216
+    if (__kmpc_omp_has_task_team(ExecThreadID))
+      return;
+
----------------
Is there a `!` missing?


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:1805
+      // instead of re-executing the routine.
+      __tgt_target_nowait_query(&taskdata->td_target_data.async_handle);
+    } else if (task->routine != NULL) {
----------------
Much better than the "execute but don't actually execute" version before. Thanks!
Do we need to, or should we, act on the updated async_handle value (I mean if it actually finished, should we do something different than when it hasn't)? Or is that already done?


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:5189
+  }
+}
+
----------------
This can fail, right? If so, we should report it to the user and deal with it properly. Otherwise we should assert it can't fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005