[Openmp-commits] [PATCH] D62397: [OPENMP][NVPTX]Relax flush directive.

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev marked an inline comment as done.
ABataev added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/flush.c:23
+#pragma omp flush(flag, data)
+      while (flag < 1) {
+#pragma omp flush(flag, data)
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > grokos wrote:
> > > > > jdoerfert wrote:
> > > > > > There is a race in this program.
> > > > > > 
> > > > > > After the flush in thread 0 happened, and after the flush prior to the while happened, both thread 0 and 32 will access flag without proper synchronization.
> > > > > I thinks that's the purpose of this test: to make sure that, despite the race on `flag`, memory fencing is enforced for `data`.
> > > > Exactly, just like I said before this is slightly reworked example for flush directive from OpenMP examples.
> > > > I thinks that's the purpose of this test: to make sure that, despite the race on flag, memory fencing is enforced for data.
> > > 
> > > Having a race on `flag`, especially since it is used in a control condition for the flush, makes the program as a whole UB. We cannot reliably test based on UB.
> > This is slightly modified test from the official OpenMP document (see https://www.openmp.org/wp-content/uploads/openmp-examples-4.5.0.pdf, 8.1 The OpenMP Memory Model). If you think the test is not correct, you need to discuss it with OpenMP committee and create a ticket for this problem to fix the example.
> > The value of `flag` is undefined because of the data race and I think this was intended.
> I will talk to the relevant people in the OpenMP ARB but that should not concern us here.
> Whatever someone else thought would make a good test case should not impact us here.
> 
> UB is nothing to rely on and I fail to see why the race that causes it is an important part of the test case we need to keep.
I agree. That's why I reworked the test and usу atomics for `flag`.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62397