[Openmp-commits] [PATCH] D44260: [OpenMP][libomptarget] Add global memory data sharing support for master-worker sharing.

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:352
+// UseSharedMemory is set to true, the runtime will attempt to use shared memory
+// as long as the size requested is fits the pre-allocated size.
+//
----------------
remove `is`


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:382
+      // The leftover data space will not be used.
+      uintptr_t ExistingSlotSize = (uintptr_t)ExistingSlot->DataEnd -
+                                   (uintptr_t)(&ExistingSlot->Data[0]);
----------------
use `ptrdiff_t` for the difference between 2 `uintptr_t'.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:418
+//
+// When the pop operation removed the last global memory slot,
+// reclaim all outstanding global memory slots since it is
----------------
removed --> removes


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:459
+EXTERN void __kmpc_begin_sharing_variables(void ***GlobalArgs, size_t nArgs) {
+  // Ensure length of 100 args to force global memory usage.
+  omptarget_nvptx_globalArgs.EnsureSize(nArgs);
----------------
where does 100 come from?


================
Comment at: libomptarget/deviceRTLs/nvptx/src/data_sharing.cu:476
+// is how the workers will get a reference to the globalized variable. The
+// members of this list will passed to the outlined parallel function
+// preserving the order.
----------------
will be passed


================
Comment at: libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:99-100
+  void **args;
+  // starts off as MAX_SHARED_ARGS but can increase in size.
+  uint32_t nArgs;
+};
----------------
`Init()` is never called throughout the code, so `nArgs` is never initialized to `MAX_SHARED_ARGS`. Add a constructor to this class which will call `Init()`.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.h:278
     worker_rootS[wid].Next = 0;
+    master_rootS[wid].Prev = 0;
     return (__kmpc_data_sharing_slot *)&worker_rootS[wid];
----------------
`worker_rootS[wid].Prev = 0;`?


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D44260