[Openmp-commits] [PATCH] D41831: Minor code cleanup

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: runtime/src/kmp_runtime.cpp:5537
+  __kmp_free_implicit_task(this_th);
+  this_th->th.th_current_task = NULL;
+
----------------
AndreyChurbanov wrote:
> protze.joachim wrote:
> > Is there a reason for not putting this into the `__kmp_free_implicit_task` function?
> 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.
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.


================
Comment at: runtime/src/kmp_tasking.cpp:990
+    task->td_dephash = NULL;
+  }
 }
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D41831