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

Alexey Bataev via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Jun 24 08:07:27 PDT 2019

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`.

  rOMP OpenMP



More information about the Openmp-commits mailing list