Use named constant to indicate all lanes, to handle 32 and 64 wide architectures [NFC]

[JonChesterfield]

Summary

NFC. Use named constant to indicate all lanes, to handle 32 and 64 wide architectures

[grokos]

Could alternatively add a constant to target_impl if preferred.

Yes, can you do that instead? No one likes magic numbers :)

[JonChesterfield]

• Replace -1 with named constant

[JonChesterfield]

• add missing chunk

[JonChesterfield]

• add missing chunk

[ronlieb]

openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu

323

is there some applicable naming convention for named constants?

[JonChesterfield]

Done. Apologies for the multiple revisions & associated emails.

[JonChesterfield]

openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu

323

Probably, but I'm not sure what. These files are a mixture of the LLVM convention and another one. At some point I guess we should batch convert all the symbols we control to the LLVM convention.

I think a strict reading of the guidelines would be __KmpcImplAllLanes, which I struggle to read. Or possibly __KMPCImplAllLanes. Overall I quite like -1.

[grokos]

I'm wondering whether it would be better to define __kmpc_impl_all_lanes conditionally, i.e. as 0xFFFF for 32-wide and 0xFFFFFFFF for 64-wide architectures. I'm not sure I like this implicit signed-to-unsigned conversion here... Also, __kmpc_impl_lanemask_t is currently typedef'ed as uint32_t, if we want to support 64-wide lanes, then __kmpc_impl_lanemask_t must also be conditionally defined accordingly.

[JonChesterfield]

In D68369#1692354, @grokos wrote:

I'm wondering whether it would be better to define __kmpc_impl_all_lanes conditionally, i.e. as 0xFFFF for 32-wide and 0xFFFFFFFF for 64-wide architectures. I'm not sure I like this implicit signed-to-unsigned conversion here... Also, __kmpc_impl_lanemask_t is currently typedef'ed as uint32_t, if we want to support 64-wide lanes, then __kmpc_impl_lanemask_t must also be conditionally defined accordingly.

More F's. But sure, I'm happy to define it as UINT32_C(0xffffffff) for nvptx instead.

The 64 lane architecture I'm thinking of has it's own target_impl.h where the corresponding typedef is indeed to uint64_t, and all the functions in this file have different implementations. See https://github.com/ROCm-Developer-Tools/llvm-project/blob/AOMP-190918/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.h if you're curious.

[JonChesterfield]

• Initialise without integer conversion

[jdoerfert]

I'm often against variables in favor of functions but a static const one in a header with guards seems fine to me.

[JonChesterfield]

A function has the advantage that it's easier to pick a consistent name for it.

Do we think this is a reasonable change as-is? I don't mind what the variable is called so please speak up if you have a preference.

[jdoerfert]

I'm fine wit this change and the name. I'm happy to accept but I thought people that did participate in the review might want to.

[grokos]

I'm fine with the changes too. Maybe we should change the title because eventually this patch does not use -1 but rather the full representation (0xffff....).

[JonChesterfield]

Good call on the title change. Thanks, will commit.

[grokos]

I also feel the patch must be marked as NFC, there is no functional change here.

[JonChesterfield]

Thanks for the NFC recommendation. I'll tag with that where appropriate from now on.

openmp/libomptarget/deviceRTLs/nvptx/src/parallel.cu

if (threadId < threadsInTeam) {
     // handle parallel levels.
     // In Cuda9+ in non-SPMD mode we have either 1 worker thread or the whole
     // warp. If only 1 thread is active, not need to reconverge the threads.
     // If we have the whole warp, reconverge all the threads in the warp before
     // actually trying to change the parallel level. Otherwise, parallel level
     // can be changed incorrectly because of threads divergence.
     bool IsActiveParallelRegion = threadsInTeam != 1;
     IncParallelLevel(IsActiveParallelRegion,
-                     IsActiveParallelRegion ? 0xFFFFFFFF : 1u);
+                     IsActiveParallelRegion ? -1 : 1u);
   }
 
   return isActive;
 }
 
 EXTERN void __kmpc_kernel_end_parallel() {
   // pop stack
   PRINT0(LD_IO | LD_PAR, "call to __kmpc_kernel_end_parallel\n");
EXTERN void __kmpc_kernel_end_parallel() {
   // handle parallel levels.
   // In Cuda9+ in non-SPMD mode we have either 1 worker thread or the whole
   // warp. If only 1 thread is active, not need to reconverge the threads.
   // If we have the whole warp, reconverge all the threads in the warp before
   // actually trying to change the parallel level. Otherwise, parallel level can
   // be changed incorrectly because of threads divergence.
     bool IsActiveParallelRegion = threadsInTeam != 1;
     DecParallelLevel(IsActiveParallelRegion,
-                     IsActiveParallelRegion ? 0xFFFFFFFF : 1u);
+                     IsActiveParallelRegion ? -1 : 1u);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // support for parallel that goes sequential
 ////////////////////////////////////////////////////////////////////////////////
 
 EXTERN void __kmpc_serialized_parallel(kmp_Ident *loc, uint32_t global_tid) {
   PRINT0(LD_IO, "call to __kmpc_serialized_parallel\n");

openmp/libomptarget/deviceRTLs/nvptx/src/reduction.cu

 
 EXTERN
 void __kmpc_nvptx_end_reduce(int32_t global_tid) {}
 
 EXTERN
 void __kmpc_nvptx_end_reduce_nowait(int32_t global_tid) {}
 
 EXTERN int32_t __kmpc_shuffle_int32(int32_t val, int16_t delta, int16_t size) {
-  return __kmpc_impl_shfl_down_sync(0xFFFFFFFF, val, delta, size);
+  return __kmpc_impl_shfl_down_sync(-1, val, delta, size);
 }
 
 EXTERN int64_t __kmpc_shuffle_int64(int64_t val, int16_t delta, int16_t size) {
    uint32_t lo, hi;
    __kmpc_impl_unpack(val, lo, hi);
-   hi = __kmpc_impl_shfl_down_sync(0xFFFFFFFF, hi, delta, size);
-   lo = __kmpc_impl_shfl_down_sync(0xFFFFFFFF, lo, delta, size);
+   hi = __kmpc_impl_shfl_down_sync(-1, hi, delta, size);
+   lo = __kmpc_impl_shfl_down_sync(-1, lo, delta, size);
    return __kmpc_impl_pack(lo, hi);
 }
 
 INLINE static void gpu_regular_warp_reduce(void *reduce_data,
                                            kmp_ShuffleReductFctPtr shflFct) {
   for (uint32_t mask = WARPSIZE / 2; mask > 0; mask /= 2) {
     shflFct(reduce_data, /*LaneId - not used= */ 0,
             /*Offset = */ mask, /*AlgoVersion=*/0);

 }
 
 EXTERN
 int32_t __kmpc_nvptx_simd_reduce_nowait(int32_t global_tid, int32_t num_vars,
                                         size_t reduce_size, void *reduce_data,
                                         kmp_ShuffleReductFctPtr shflFct,
                                         kmp_InterWarpCopyFctPtr cpyFct) {
   __kmpc_impl_lanemask_t Liveness = __kmpc_impl_activemask();
-  if (Liveness == 0xffffffff) {
+  if (Liveness == -1) {
     gpu_regular_warp_reduce(reduce_data, shflFct);
     return GetThreadIdInBlock() % WARPSIZE ==
            0; // Result on lane 0 of the simd warp.
   } else {
     return gpu_irregular_simd_reduce(
         reduce_data, shflFct); // Result on the first active lane.
   }
 }
if (NumThreads > WARPSIZE) {
 
     if (WarpId == 0)
       gpu_irregular_warp_reduce(reduce_data, shflFct, WarpsNeeded,
                                 BlockThreadId);
   }
   return BlockThreadId == 0;
 #else
   __kmpc_impl_lanemask_t Liveness = __kmpc_impl_activemask();
-  if (Liveness == 0xffffffff) // Full warp
+  if (Liveness == -1) // Full warp
     gpu_regular_warp_reduce(reduce_data, shflFct);
   else if (!(Liveness & (Liveness + 1))) // Partial warp but contiguous lanes
     gpu_irregular_warp_reduce(reduce_data, shflFct,
                               /*LaneCount=*/__kmpc_impl_popc(Liveness),
                               /*LaneId=*/GetThreadIdInBlock() % WARPSIZE);
   else if (!isRuntimeUninitialized) // Dispersed lanes. Only threads in L2
                                     // parallel region may enter here; return
                                     // early.
#else
   // Load from scratchpad and reduce.
   char *scratchpad = GetTeamsReductionScratchpad();
   ldFct(reduce_data, scratchpad, ThreadId, NumTeams, /*Load only*/ 0);
   for (uint32_t i = NumThreads + ThreadId; i < NumTeams; i += NumThreads)
     ldFct(reduce_data, scratchpad, i, NumTeams, /*Load and reduce*/ 1);
 
   // Reduce across warps to the warp master.
   __kmpc_impl_lanemask_t Liveness = __kmpc_impl_activemask();
-  if (Liveness == 0xffffffff) // Full warp
+  if (Liveness == -1) // Full warp
     gpu_regular_warp_reduce(reduce_data, shflFct);
   else // Partial warp but contiguous lanes
     gpu_irregular_warp_reduce(reduce_data, shflFct,
                               /*LaneCount=*/__kmpc_impl_popc(Liveness),
                               /*LaneId=*/ThreadId % WARPSIZE);
 
   // When we have more than [warpsize] number of threads
   // a block reduction is performed here.