[Openmp-commits] [PATCH] D107656: [OpenMP] Use events and taskyield in target nowait task to unblock host threads

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:349
 
+void __kmpc_target_task_yield();
+
----------------
This function is defined in libomp, so it needs to be declared with the `weak` attribute in `private.h` alongside the other API functions from libomp (see `private.h`, the code block around line 90). Otherwise, we make libomptarget dependent on libomp, whereas we want it to be able to be build independently from any specific host OpenMP runtime.


================
Comment at: openmp/libomptarget/src/CMakeLists.txt:38
 target_link_libraries(omptarget PRIVATE
+  omp
   ${CMAKE_DL_LIBS}
----------------
If you move the declaration of `__kmpc_target_task_yield` to `private.h` and mark it as weak, we can skip linking against `omp`.


================
Comment at: openmp/libomptarget/src/device.cpp:552
+// when OFFLOAD_SUCCESS is returned, it means either the event has been
+// fullfiled without error or the event has not been not fullfiled and
+// AsyncInfo.Event is not nullptr.
----------------
"fullfiled" --> "fulfilled"
"has not been not fullfiled" --> "has not been fulfilled"



================
Comment at: openmp/libomptarget/src/interface.cpp:323
 
 EXTERN int __tgt_target_nowait_mapper(
     ident_t *loc, int64_t device_id, void *host_ptr, int32_t arg_num,
----------------
This single-team API function needs the same patching you applied to `__tgt_target_teams_nowait_mapper`.


================
Comment at: openmp/libomptarget/src/omptarget.cpp:24
+  char *EnvStr = getenv("LIBOMPTARGET_USE_NOWAIT_EVENT");
+  return EnvStr ? std::stoi(EnvStr) : 0;
+}();
----------------
Should we check for invalid values of this env var?


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

https://reviews.llvm.org/D107656