[Openmp-commits] [PATCH] D60918: [OPENMP][NVPTX]Correctly handle L2 parallelism in SPMD mode.

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/spmd_parallel_regions.cpp:18
                    ? omp_is_initial_device()
                    : 1;
       ParallelLevel1 =
----------------
I'm confused by the ternary operator (also the one below).

If the first target thread executes this critical region, `isHost` is `-1` and it will be set to `omp_is_initial_device()`. The second thread comes along and will set it to `omp_is_initial_device` because it was, at any point in time, either `-1` or `omp_is_initial_device`. And so on. So why the ternary operator?


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/spmd_parallel_regions.cpp:27
+        L2 = omp_get_level();
+        Count += omp_get_level(); // (9-5)*10*2 = 80
+      }
----------------
I don't get the calculation in the comment. Could you please elaborate?

In general, I'm confused because you rewrite the test again.
Was the old test broken? If not, why don't we keep both? If it was, can you explain why?

I have more and more the feeling changes are added and modified faster than anyone actually understands them, leading to "tests" that simply verify the current behavior.


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/spmd_parallel_regions.cpp:30
 #pragma omp critical
-    ParallelLevel2 = (ParallelLevel2 < 0 || ParallelLevel2 == 2) ? L2 : 1;
+      ParallelLevel2 = (ParallelLevel2 < 0 || ParallelLevel2 == 2) ? L2 : 1;
+    } else {
----------------
Why do we need this ternary operator, shouldn't ParallelLevel2 always be set to L2?


================
Comment at: libomptarget/deviceRTLs/nvptx/test/parallel/spmd_parallel_regions.cpp:38
     printf("Runtime error, isHost=%d\n", isHost);
   }
 
----------------
As far as I can tell, a value of -1 for isHost will not cause the test to fail. `(bool)-1` is `true` and you never verify the "Runtime error" is not printed.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60918