[Openmp-commits] [PATCH] D62393: [OPENMP][NVPTX]Mark parallel level counter as volatile.

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D62393#1539057 <https://reviews.llvm.org/D62393#1539057>, @jfb wrote:

> In D62393#1539040 <https://reviews.llvm.org/D62393#1539040>, @hfinkel wrote:
>
> > In D62393#1539014 <https://reviews.llvm.org/D62393#1539014>, @ABataev wrote:
> >
> > > In D62393#1539012 <https://reviews.llvm.org/D62393#1539012>, @jfb wrote:
> > >
> > > > FWIW we already support `-fms-volatile`, so there's precedent if you wanted `-fnv-volatile` (however terrible that is).
> > >
> > >
> > > Most probably, we don't need this option, clang should emit correct code for volatile vars in Cuda mode automatically.
> >
> >
> > +1
> >
> > That having been said, maybe this is a very simple change because it is the same as (or very similar to) what MSVolatile does?
>
>
> My point exactly: MS volatile promotes volatile to `seq_cst`, it's trivial to copy and promote to `relaxed` and isn't too objectionable because we've got precedent.
>
> Further consideration: you say you want to match the nvcc semantics... but it sounds like it doesn't actually have great / documented semantics, and they might changes. Should this `volatile`->`relaxed` be opt-in or opt-out? Is that the only thing we'd want to guarantee?


I think, for now we can support this unconditionally. Later, if the semantics will change, we can check for the cuda version and disable this semantics for newer versions. seems to me, volatile->relaxed must be enough.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62393