[Openmp-commits] [PATCH] D62199: [OPENMP][NVPTX]Fix barriers and parallel level counters, NFC.
Alexey Bataev via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Wed May 22 13:14:25 PDT 2019
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
More information about the Openmp-commits
mailing list