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

[Guilherme Valarini via Phabricator via Openmp-commits]

gValarini marked an inline comment as not done.
gValarini added a comment.

I believe I answered and fixed most of the comments on this revision. Waiting for the next round. 😉



================
Comment at: openmp/libomptarget/include/omptarget.h:189
+public:
+  enum class SyncTypeTy { BLOCKING, NON_BLOCKING };
+
----------------
tianshilei1992 wrote:
> `SyncTypeTy` looks weird. It's like having an LLVM class called `TypeTy`. I think `SyncTy` or `SyncType` are both fine.
It makes sense, that was a little redundant. It is now renamed to `SyncTy`. Thanks!


================
Comment at: openmp/libomptarget/include/omptarget.h:414
+// and a value different than zero otherwise.
+void __tgt_target_nowait_query(void **AsyncHandle);
+
----------------
jdoerfert wrote:
> It's void return but comment talks about the return value.
Yep, that was a leftover from some previous revisions. Thanks!


================
Comment at: openmp/libomptarget/src/interface.cpp:64
 
+static inline void targetDataMapper(
+    ident_t *Loc, DeviceTy &Device, int64_t DeviceId, int32_t ArgNum,
----------------
jdoerfert wrote:
> 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?
Indeed, that is a nice idea. Since `TaskAsyncInfoWrapperTy` is a wrapper around `AsyncInfoTy`, I only needed to acquire a reference to it so we would end up always using `AsyncInfoTy`.


================
Comment at: openmp/libomptarget/src/interface.cpp:84
+  int Rc = OFFLOAD_SUCCESS;
+  Rc = TargetDataFunction(Loc, Device, ArgNum, ArgsBase, Args, ArgSizes,
+                          ArgTypes, ArgNames, ArgMappers, AsyncInfo,
----------------
tianshilei1992 wrote:
> nit: `targetDataFunction`
Uhm, `TargetDataFunction` is a function pointer. Shouldn't we also capitalize the first word in this case? 


================
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,
----------------
jdoerfert wrote:
> Same comment as above wrt. templated version. The duplication we introduce is something I would like to avoid.
Done as well.


================
Comment at: openmp/libomptarget/src/interface.cpp:382
+           "this a target nowait region?\n");
+    handleTargetOutcome(false, nullptr);
+  }
----------------
jdoerfert wrote:
> Do we know the above call is "noreturn"? If not, we should explicitly exit here. On second thought, we should exit either way.
Uhm, yep, you are right. We should always exit here. I am converting to using `FATAL_MESSAGE0`, so we directly abort the program.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:73
+  SmallVector<PostProcFuncTy> Functions(
+      std::move(PostProcessingFunctions));
+
----------------
jdoerfert wrote:
> 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.
Uhm, really? When moving a `SmallVector` like this, wouldn't `PostProcessingFunctions` be emptied and all the values moved to `Functions`?


================
Comment at: openmp/libomptarget/src/private.h:216
+    if (__kmpc_omp_has_task_team(ExecThreadID))
+      return;
+
----------------
jdoerfert wrote:
> Is there a `!` missing?
I forget to submit some local changes! Done.


================
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) {
----------------
jdoerfert wrote:
> 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?
Nice.

And yep, that is already done at the task finalization. Take a look at the changes on `kmp_tasking.cpp:1099`. When `async_handle` is not null, the task is re-enqueued, otherwise, the task is finished normally.


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:5189
+  }
+}
+
----------------
jdoerfert wrote:
> 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.
Uhm, that makes sense. I'll try to add this functionally and fall back to the old execution flow if it fails.


================
Comment at: openmp/runtime/src/kmp_tasking.cpp:5199
+*/
+KMP_EXPORT bool __kmpc_omp_has_task_team(kmp_int32 gtid) {
+  kmp_info_t *thread = __kmp_thread_from_gtid(gtid);
----------------
tianshilei1992 wrote:
> Do we need to check if `gtid` is valid here?
Uhm, yep we indeed need to check it. I'll add it here and return false if `gtid` is invalid. This way we can fall back to the old execution flow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132005