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

Johannes Doerfert via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Jun 24 07:14:12 PDT 2019


jdoerfert 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)
----------------
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.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62397





More information about the Openmp-commits mailing list