Minor code cleanup

[tlwilmar]

Summary

Minor code cleanup

[AndreyChurbanov]

LGTM

[protze.joachim]

runtime/src/kmp_runtime.cpp

5537 ↗

(On Diff #128968)

Is there a reason for not putting this into the __kmp_free_implicit_task function?

[protze.joachim]

runtime/src/kmp_tasking.cpp

990 ↗

(On Diff #128968)

Moving above task=NULL here makes the data race visible. My suggestion:

kmp_taskdata_t *task = NULL;
<atomic_swap>(task, thread->th.th_current_task);
if (task && task->td_dephash) {
...

Whatever "atomic swap" function would be applicable.

[AndreyChurbanov]

runtime/src/kmp_runtime.cpp

5537 ↗

(On Diff #128968)

There are cases where the thread structure is cleaned to be re-used, then the zeroing makes sense.
And in other cases where the thread structure has its memory freed there is no need to zero the contents.

So to me it looks logical to have the assignment here.

runtime/src/kmp_tasking.cpp

990 ↗

(On Diff #128968)

Sorry, I haven't got what is the data race you are talking about.

[protze.joachim]

runtime/src/kmp_runtime.cpp

5537 ↗

(On Diff #128968)

This is the only place, where __kmp_free_implicit_task is called. So semantically, there is no difference for moving the assignment into the function.

runtime/src/kmp_tasking.cpp

990 ↗

(On Diff #128968)

Moving above assignment into the function:

void __kmp_free_implicit_task(kmp_info_t *thread) {
​  kmp_taskdata_t *task = thread->th.th_current_task;
​  if (task && task->td_dephash) {
​    __kmp_dephash_free(thread, task->td_dephash);
​    task->td_dephash = NULL;
​  }
  thread->th.th_current_task = NULL;
}

From the comment in above code I understood, that various threads might call this function with the same argument?
-> Then there would be a race between assigning NULL and the if.
Reading the comment again is the issue, that the same kmp_taskdata is reference by multiple threads, that used the task before?
-> No race, but I would still prefer to move the assignment into the function, for the case that the function will be used somewhere else.

[tlwilmar]

runtime/src/kmp_runtime.cpp

5537 ↗

(On Diff #128968)

There is another use of this function in __kmp_reap_thread that does not nullify th_current_task after it.

[tlwilmar]

runtime/src/kmp_tasking.cpp

990 ↗

(On Diff #128968)

In the context of __kmp_free_thread the assignment to NULL is safe. The context of __kmp_free_thread is either being called by a master thread allocating or changing team size under a lock, or freeing non-hot teams, so there should be no race.

[protze.joachim]

Ok, I think, I now understood the change.

openmp/trunk/runtime/src/kmp_lock.h

 extern int __kmp_release_nested_tas_lock(kmp_tas_lock_t *lck, kmp_int32 gtid);
 extern void __kmp_init_nested_tas_lock(kmp_tas_lock_t *lck);
 extern void __kmp_destroy_nested_tas_lock(kmp_tas_lock_t *lck);
 
 #define KMP_LOCK_RELEASED 1
 #define KMP_LOCK_STILL_HELD 0
 #define KMP_LOCK_ACQUIRED_FIRST 1
 #define KMP_LOCK_ACQUIRED_NEXT 0
-
+#ifndef KMP_USE_FUTEX
 #define KMP_USE_FUTEX                                                          \
   (KMP_OS_LINUX && !KMP_OS_CNK &&                                              \
    (KMP_ARCH_X86 || KMP_ARCH_X86_64 || KMP_ARCH_ARM || KMP_ARCH_AARCH64))
-
+#endif
 #if KMP_USE_FUTEX
 
 // ----------------------------------------------------------------------------
 // futex locks.  futex locks are only available on Linux* OS.
 //
 // Like non-nested test and set lock, non-nested futex locks use the memory
 // allocated by the compiler for the lock, rather than a pointer to it.
 //

openmp/trunk/runtime/src/kmp_runtime.cpp

void __kmp_free_thread(kmp_info_t *this_th) {
   }
   this_th->th.th_task_state = 0;
 
   /* put thread back on the free pool */
   TCW_PTR(this_th->th.th_team, NULL);
   TCW_PTR(this_th->th.th_root, NULL);
   TCW_PTR(this_th->th.th_dispatch, NULL); /* NOT NEEDED */
 
+  /* If the implicit task assigned to this thread can be used by other threads
+   * -> multiple threads can share the data and try to free the task at
+   * __kmp_reap_thread at exit. This duplicate use of the task data can happen
+   * with higher probability when hot team is disabled but can occurs even when
+   * the hot team is enabled */
+  __kmp_free_implicit_task(this_th);
+  this_th->th.th_current_task = NULL;
+
   // If the __kmp_thread_pool_insert_pt is already past the new insert
   // point, then we need to re-scan the entire list.
   gtid = this_th->th.th_info.ds.ds_gtid;
   if (__kmp_thread_pool_insert_pt != NULL) {
     KMP_DEBUG_ASSERT(__kmp_thread_pool != NULL);
     if (__kmp_thread_pool_insert_pt->th.th_info.ds.ds_gtid > gtid) {
       __kmp_thread_pool_insert_pt = NULL;
     }

openmp/trunk/runtime/src/kmp_tasking.cpp

 }
 
 // __kmp_free_implicit_task: Release resources associated to implicit tasks
 // when these are destroyed regions
 //
 // thread:  thread data structure corresponding to implicit task
 void __kmp_free_implicit_task(kmp_info_t *thread) {
   kmp_taskdata_t *task = thread->th.th_current_task;
-  if (task->td_dephash)
+  if (task && task->td_dephash) {
     __kmp_dephash_free(thread, task->td_dephash);
     task->td_dephash = NULL;
   }
+}
 
 // Round up a size to a power of two specified by val: Used to insert padding
 // between structures co-allocated using a single malloc() call
 static size_t __kmp_round_up_to_val(size_t size, size_t val) {
   if (size & (val - 1)) {
     size &= ~(val - 1);
     if (size <= KMP_SIZE_T_MAX - val) {
       size += val; // Round up if there is no overflow.