[Openmp-commits] [PATCH] D145831: [OpenMP][libomptarget] Add support for critical regions in AMD GPU device offloading

Mon Mar 20 19:13:59 PDT 2023

dhruvachak added inline comments.

================
Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:283
+  if (mapping::getThreadIdInWarp() == LowestActiveThread) {
+    fenceKernel(atomic::release);
+    while (!atomicCAS((uint32_t *)Lock, UNSET, SET, atomic::relaxed,
----------------
doru1004 wrote:
> dhruvachak wrote:
> > Why do we need the release fence in setLock before the CAS?
> You have to release all of the ordinary stores before you write to the lock.
In theory, setLock should have acquire semantics and unsetLock should have release semantics. So the setLock does not really need the release fence. But this patch is consistent between the set and the unset, namely both use acq_rel semantics. So I am ok with the current state of the patch on this aspect.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145831