[Openmp-commits] [PATCH] D62199: [OPENMP][NVPTX]Fix barriers and parallel level counters, NFC.

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added a comment.

In D62199#1512617 <https://reviews.llvm.org/D62199#1512617>, @jdoerfert wrote:

> I have a problem with this patch and this kind of patches/reviews:
>
> 1. How can a "Fix" commit be "NFC" at the same time?
> 2. The commit message basically mentions that multiple unrelated changes are combined in one patch.
> 3. I somehow doubt that it's the "ptx optimizations" that "lead to UB":
>
> > Parallel level counter should be volatile to prevent some dangerous optimiations by the ptxas. Otherwise, ptxas optimizations lead to undefined behaviour in some cases.
>
> This does not sound like a "solution" but more like a hack. With `volatile` the problem goes away so let's make it `volatile`.
>
> 4. The more patches go through with this kind of "review" the more "suspicious" this looks to me.




1. Because it is impossible to test them, that why it is NFC.
2. Yes, this is true. Bu the changes are rather small.
3. It is only your opinion.
4. Again, this is your problem, The patch was approved bythe code owner

In D62199#1512633 <https://reviews.llvm.org/D62199#1512633>, @arsenm wrote:

> No tests


These changes cannot be tested. The problem with the original code appear only in very rare situations in highly optimized code.

1. parallelLevel is accessed by many threads simultaneously and without volatile can be optimized and lead to the undefined behavior.
2. I have no idea how to test the cache flushing.

That's why this patch is marked as NFC.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62199