[Openmp-commits] [PATCH] D64217: [OpenMP][NFCI] Cleanup the target state queue implementation

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/state-queue.h:1
-//===--------- statequeue.h - NVPTX OpenMP GPU State Queue ------- CUDA -*-===//
+//===--- state-queue.h --- OpenMP target state queue ------------*- C++ -*-===//
 //
----------------
ABataev wrote:
> I think it is better to mark it as Cuda, not C/C++, since it contains some Cuda specific constructs.
I though I removed all of them, if I haven't we should. The idea is that this "core logic" code should _not_ be CUDA code but something else, e.g., C++ seems natural.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/state-queue.h:26
 
-template <typename ElementType, uint32_t SIZE> class omptarget_nvptx_Queue {
-private:
-  ElementType elements[SIZE];
-  volatile ElementType *elementQueue[SIZE];
-  volatile uint32_t head;
-  volatile uint32_t ids[SIZE];
-  volatile uint32_t tail;
+template <typename ElementType, uint32_t SIZE> class omptarget_state_queue {
+  ElementType Elements[SIZE];
----------------
ABataev wrote:
> Maybe it is better to name it in LLVM style, without underscores etc.?
I like the idea for for internal types. It will make it more consistent and the external interface also easier to spot.


================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.h:19
+/// value of the pointee.
+template <typename T> T __kmpc_impl_atomic_add(T *Ptr, T Val) {
+  return atomicAdd(Ptr, Val);
----------------
ABataev wrote:
> Why do we need these functions? IMHO, they do not add anything useful, just increase code complexity.
They add a level of abstraction around the CUDA implementation, here the CUDA functions `atomicAdd` and `atomicExch`. In the AMD `target_impl.h` the `__kmpc_impl_atomic_exchange` would maybe be implemented with an explicit compare-exchange loop (I'm guessing here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64217