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

[Guansong Zhang via Phabricator via Openmp-commits]

guansong added a comment.

In https://reviews.llvm.org/D46185#1081124, @Hahnfeld wrote:

> In https://reviews.llvm.org/D46185#1081111, @grokos wrote:
>
> > 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)`.
>
>
> Fully agree with George's analysis here. The proposed implementation may be tweaked to early exit if `elem < *Buffer`.
>
> Anyway, why do you want to support `sm_30`? Is there a particular hardware that you want to support? Why does AMD care?!?


I will double check with Greg on the way atomicCAS used in the code. For the last question here. We have sm_30 machine for testing. Besides, from my perspective, I care the overall implementation of openmp, particularly the offloading part, not necessarily amdgcn code only :)


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D46185