[Openmp-commits] [PATCH] D135036: [OpenMP] Introduce more atomic operations into the runtime

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 accepted this revision.
tianshilei1992 added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:54
+  }                                                                            \
+  TY atomicLoad(TY *Address, atomic::OrderingTy Ordering) {                    \
+    return atomicAdd(Address, TY(0), Ordering);                                \
----------------
tianshilei1992 wrote:
> the indent is off here
nvm


================
Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:67
+    if (Val >= 0)                                                              \
+      return atomicMin((SINT_TY *)Address, utils::convertViaPun<SINT_TY>(Val), \
+                       Ordering);                                              \
----------------
tianshilei1992 wrote:
> tianshilei1992 wrote:
> > This doesn't look right. The integer value after type punning doesn't conform with the same arithmetic as the original floating-point value. Actually we already have `atomicrmw fmax/fmin` in LLVM.
> and we should also have the corresponding compiler builtins.
okay, currently clang doesn't support the floating-point builtins yet. One alternative is to use OpenMP `atomic` directive here, but I'm also fine if the currently scheme could work. We can revisit this part later when we have the support in the compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135036