[Openmp-commits] [PATCH] D64218: [OpenMP][NFCI] Cleanup the target synchronization implementation

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/sync.cpp:18
+EXTERN int32_t __kmpc_cancel_barrier(kmp_Ident *Loc, int32_t TID) {
+  __kmpc_impl_barrier</* IsCancellable */ true, /* IsSimple */ false,
+                      /* IsSPMD */ false>(Loc, TID);
----------------
We don't support cancellation in the GPU runtime currently, so I think it is better to set `IsCancellable` to `false` to make it clear that cancellation is not supported yet.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/sync.cpp:18
+EXTERN int32_t __kmpc_cancel_barrier(kmp_Ident *Loc, int32_t TID) {
+  __kmpc_impl_barrier</* IsCancellable */ true, /* IsSimple */ false,
+                      /* IsSPMD */ false>(Loc, TID);
----------------
ABataev wrote:
> We don't support cancellation in the GPU runtime currently, so I think it is better to set `IsCancellable` to `false` to make it clear that cancellation is not supported yet.
I think I got the idea for these params. But it is better to give them some other names, currently they might confuse users.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/sync.cpp:37
+EXTERN void __kmpc_barrier_simple_generic(kmp_Ident *Loc, int32_t TID) {
+  __kmpc_impl_barrier</* IsCancellable */ false, /* IsSimple */ true,
+                      /* IsSPMD */ false>(Loc, TID);
----------------
Not sure that this instantiation provides the same afunctionality as the original implementation. Originally, it did not check for ghe number of active threads, just synced all non-SPMD threads unconditionally.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/sync.cpp:56
+/// as ENTERING_PREDICATE.
+#define REGION_DELIMITERS(NAME, ENTERING_PREDICATE)                            \
+                                                                               \
----------------
Can we invent something else here, not the macros?


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/sync.cpp:88
+///
+/// FIXME: Warps are a detail we should get rid of here.
+EXTERN int32_t __kmpc_warp_active_thread_mask() {
----------------
This is impossible to get rid of it for NVPTX runtime, at least. Better to make it NVPTX specific function.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.h:55
+  if (InSPMD) {
+    PRINT(LD_SYNC, "call kmpc%s_barrier%s_spmd\n",
+          IsCancellable ? "_cancel" : "", IsSimple ? "_simple" : "");
----------------
This is not correct. `kmpc_barrier` can be called even in SPMD mode. Generally speaking, the `spmd` suffix also must be generated dynamically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64218