[PATCH] D41831: Minor code cleanup

[Terry Wilmarth via Phabricator via llvm-commits]

tlwilmar added inline comments.


================
Comment at: runtime/src/kmp_tasking.cpp:990
+    task->td_dephash = NULL;
+  }
 }
----------------
protze.joachim wrote:
> AndreyChurbanov wrote:
> > protze.joachim wrote:
> > > 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.
> > Sorry, I haven't got what is the data race you are talking about.
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D41831