[PATCH] D41831: Minor code cleanup

[Andrey Churbanov via Phabricator via llvm-commits]

AndreyChurbanov added inline comments.


================
Comment at: runtime/src/kmp_runtime.cpp:5537
+  __kmp_free_implicit_task(this_th);
+  this_th->th.th_current_task = NULL;
+
----------------
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.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D41831