[Openmp-commits] [PATCH] D46185: [OpenMP] Allow nvptx sm_30 to be used as an offloading device

[George Rokos via Phabricator via Openmp-commits]

grokos added a comment.

I don't think this code does atomic max. Actually, I don't think it does anything at all. First off, `old_value` and `elem` are variables which do not change inside the while-loop. So the first loop condition is either always true or always false. Secondly, `atomicCAS` returns the value that existed at address `Buffer` (regardless of whether or not the CAS operation succeeded) and not a boolean result. Also, `old_value` must be updated before each CAS attempt because some other thread may have changed it.

A correct implementation would look somewhat like this:

  uint64_t old_value = *Buffer;
  uint64_t expected_old_value;
  do {
    expected_old_value = old_value;
    old_value = atomicCAS((unsigned long long *) Buffer,
                          (unsigned long long) expected_old_value,
                          (unsigned long long) max(expected_old_value, elem));
  } while (expected_old_value != old_value);

What this code does is: check that the value at `Buffer` is still what I expect it to be, i.e. no other thread has changed it, and replace it with `max(old_value, elem)`.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D46185