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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev marked 2 inline comments 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:
> > 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.


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/flush.c:28
+      // CHECK: data=42.
+      /* Values data will be 42, value of flag still undefined */
+      printf("data=%d.\n", data);
----------------
jdoerfert wrote:
> ABataev wrote:
> > grokos wrote:
> > > "Values" --> "Value of data". Also, why is the value of "flag" undefined at this point?
> > as I understand, it happens because the compiler is free to reorder read/write ops in any order without explicitly provided ordering. That's why flag can be undefined here.
> > compiler is free to reorder read/write ops in any order without explicitly provided ordering. 
> 
> Forgetting about the UB we have in this program for a moment, it is not true that a compiler is free to reorder arbitrary read/writes. I'm unsure what accesses you refer to here and how do they need to be reordered to lead to undefined values? (I could imagine this to be a side effect of the UB above which would make my point that this is not a reliable method to test things.)
This assumption was wrong, actually, the value is undefined because of the race condition.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62397