Fix for OMP doacross implementation on 32-bit platforms

[AndreyChurbanov]

Summary

The bug was in the size of integer used to interpret pointer for CAS operation.
The fix is to use 32-bit integer for 32-bit platform, and 64-bit otherwise.

[Hahnfeld]

Do we need all volatile casts that force the different code paths for 32 and 64 bit? In my experiments on Power it worked without them...

runtime/src/kmp_csupport.cpp

3809 ↗

(On Diff #123312)

Should we introduce KMP_COMPARE_AND_STORE_RETPTR for this?

[AndreyChurbanov]

runtime/src/kmp_csupport.cpp

3809 ↗

(On Diff #123312)

This is the only place in the code where CAS on pointer is interested in exact return value (to perform lock-less shared allocation), so the new macro won't help much I think. So I'd vote to leave it as is.

[hbae]

LGTM

[AndreyChurbanov]

In D40171#928510, @Hahnfeld wrote:

Do we need all volatile casts that force the different code paths for 32 and 64 bit? In my experiments on Power it worked without them...

Forgot to answer this point. The second cast to volatile (in while statement) used to force the compiler to re-read the value of doacross_flags pointer at each iteration. The first cast (in CAS statement) just repeats the same cast encoded into the macros. So it is not technically necessary, but used here just in case we may some time decide to change the macros, and then this cast will work.

[Hahnfeld]

Thanks for the explanation

openmp/trunk/runtime/src/kmp_csupport.cpp

void __kmpc_doacross_init(ident_t *loc, int gtid, int num_dims,
 
   // Check if shared buffer is not occupied by other loop (idx -
   // __kmp_dispatch_num_buffers)
   if (idx != sh_buf->doacross_buf_idx) {
     // Shared buffer is occupied, wait for it to be free
     __kmp_wait_yield_4((volatile kmp_uint32 *)&sh_buf->doacross_buf_idx, idx,
                        __kmp_eq_4, NULL);
   }
+#if KMP_32_BIT_ARCH
   // Check if we are the first thread. After the CAS the first thread gets 0,
   // others get 1 if initialization is in progress, allocated pointer otherwise.
+  // Treat pointer as volatile integer (value 0 or 1) until memory is allocated.
+  flags = (kmp_uint32 *)KMP_COMPARE_AND_STORE_RET32(
+      (volatile kmp_int32 *)&sh_buf->doacross_flags, NULL, 1);
+#else
   flags = (kmp_uint32 *)KMP_COMPARE_AND_STORE_RET64(
-      (kmp_int64 *)&sh_buf->doacross_flags, NULL, (kmp_int64)1);
+      (volatile kmp_int64 *)&sh_buf->doacross_flags, NULL, 1LL);
+#endif
   if (flags == NULL) {
     // we are the first thread, allocate the array of flags
-    kmp_int64 size =
-        trace_count / 8 + 8; // in bytes, use single bit per iteration
+    size_t size = trace_count / 8 + 8; // in bytes, use single bit per iteration
     sh_buf->doacross_flags = (kmp_uint32 *)__kmp_thread_calloc(th, size, 1);
-  } else if ((kmp_int64)flags == 1) {
+  } else if (flags == (kmp_uint32 *)1) {
+#if KMP_32_BIT_ARCH
     // initialization is still in progress, need to wait
-    while ((volatile kmp_int64)sh_buf->doacross_flags == 1) {
+    while (*(volatile kmp_int32 *)&sh_buf->doacross_flags == 1)
+#else
+    while (*(volatile kmp_int64 *)&sh_buf->doacross_flags == 1LL)
+#endif
       KMP_YIELD(TRUE);
-    }
-  }
-  KMP_DEBUG_ASSERT((kmp_int64)sh_buf->doacross_flags >
-                   1); // check value of pointer
+  KMP_DEBUG_ASSERT(sh_buf->doacross_flags > (kmp_uint32 *)1); // check ptr value
   pr_buf->th_doacross_flags =
       sh_buf->doacross_flags; // save private copy in order to not
   // touch shared buffer on each iteration
   KA_TRACE(20, ("__kmpc_doacross_init() exit: T#%d\n", gtid));
 }
 
 void __kmpc_doacross_wait(ident_t *loc, int gtid, long long *vec) {
   kmp_int32 shft, num_dims, i;