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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

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.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62393