[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#1698621 <https://reviews.llvm.org/D62393#1698621>, @JonChesterfield wrote:

> In D62393#1698559 <https://reviews.llvm.org/D62393#1698559>, @ABataev wrote:
>
> > It means that you missed something. Initially, patch marked the whole array as volatile but after some discussion it was decided to use ptx instructions directly to avoid some side effects.
>
>
> I don't think so. Phabricator doesn't make the discussion easy to follow relative to the changes in code, but I'm pretty sure the sequence was:
>
> - Mark the array volatile in the initial patch
> - Long discussion about volatile vs atomic
> - Agreement that the control flow requires atomic semantics
> - Discussion about how to express that in cuda
> - Suggestion to "declare the variable volatile, and then use inline asm ld.volatile/st.volatile to increment it"
> - Drop the volatile qualification on the array and use inline asm to increment it
> - Write a comment saying a volatile access is used to avoid dangerous optimizations
> - We start this exchange
>
>   The byte array should be volatile qualified, as it was in your initial patch. And you should use inline asm or equivalent to prevent memory reordering for ptx. And the comment should probably reflect that the code requires both ptx volatile and the constraint on memory order.


Not again! We don't need to make it atomic, we have memory and threads barriers. But cuda8 sometimes just misses them and moves accesses to the variables across the barriers though it should not. The simplest and less intrusive way to prevent it is to mark those mem accesses as volatile (or atomic, whoch is too expensive). And this is what we do in this patch.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62393