[OpenMP][deviceRTLs] Fix wrong return value of `__kmpc_is_spmd_exec_mode`

[tianshilei1992]

Summary

D110279 introduced a bug to the device runtime. In __kmpc_parallel_51, we detect
whether we are already in parallel region by __kmpc_parallel_level() > __kmpc_is_spmd_exec_mode().
It is based on the assumption that:

• In SPMD mode, parallel level is initialized to 1.
• In generic mode, parallel level is initialized to 0.
• __kmpc_is_spmd_exec_mode returns 1 for SPMD mode, 0 otherwise.

Because the return value type of __kmpc_is_spmd_exec_mode is int8_t, there
was an implicit cast from bool to int8_t. We can make sure it is either 0 or
1 since C++14. In D110279, the return value is the result of an and operation,
which is 2 in SPMD mode. This breaks the assumption in __kmpc_parallel_51.

[carlo.bertolli]

LGTM.

Thanks for fixing this. It would be nice if you could add a comment where it is used at the beginning of parallel_51, stating what are the initial value of parallelLevel (kmpc_parallel_level) and those of kmpc_is_spmd_exec_mode in the two cases of generic and spmd modes (assuming generic spmd mode is handled as a spmd). Something like:

• In SPMD mode, parallelLevel (queried by kmpc_parallel_level()) is set to 1 at kernel initialization. kmpc_is_spmd_exec_mode returns 1: we will need to serialize a nested parallel region only when we increment parallelLevel by one (value will be 2) when encountering the nested region.

• In generic mode, parallelLevel is set to 0 at kernel initialization. kmpc_Is_spmd_exec_mode returns 0 in this case: we will need to serialized a nested parallel region only when we increment parallelLevel by one (value will be 1) when encountering the nested region.

[dpalermo]

I applied this change to see if it fixed the following (since this routine is called when deciding to serialize nested parallel regions):

https://bugs.llvm.org/show_bug.cgi?id=52169
Bug 52169 - [OpenMP] Wrong answers with nested 'omp parallel for'

...but I am still seeing the same behavior/wrong answers.

This patch looks good to me. There is just something else remaining that is still causing a problem with nested parallel.

[tianshilei1992]

Are they still broken before D110279?

[tianshilei1992]

update comment

openmp/libomptarget/deviceRTLs/common/src/omptarget.cu

if (threadId == 0) {
     // Enqueue omp state object for use by another team.
     int slot = usedSlotIdx;
     omptarget_nvptx_device_State[slot].Enqueue(
         omptarget_nvptx_threadPrivateContext);
   }
 }
 
 // Return true if the current target region is executed in SPMD mode.
+// NOTE: This function has to return 1 for SPMD mode, and 0 for generic mode.
+// That's because `__kmpc_parallel_51` checks if it's already in parallel region
+// by comparision between the parallel level and the return value of this
+// function.
 EXTERN int8_t __kmpc_is_spmd_exec_mode() {
-  return execution_param & OMP_TGT_EXEC_MODE_SPMD;
+  return (execution_param & OMP_TGT_EXEC_MODE_SPMD) == OMP_TGT_EXEC_MODE_SPMD;
 }
 
 EXTERN int8_t __kmpc_is_generic_main_thread(kmp_int32 Tid) {
   return !__kmpc_is_spmd_exec_mode() && __kmpc_is_generic_main_thread_id(Tid);
 }
 
 NOINLINE EXTERN int8_t __kmpc_is_generic_main_thread_id(kmp_int32 Tid) {
   return GetMasterThreadID() == Tid;