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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert requested changes to this revision.
jdoerfert added inline comments.
This revision now requires changes to proceed.


================
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:
> 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.


================
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);
----------------
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.)


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62397