This is an archive of the discontinued LLVM Phabricator instance.

Minor code cleanup
ClosedPublic

Authored by tlwilmar on Jan 8 2018, 12:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlwilmar created this revision.Jan 8 2018, 12:20 PM
This revision is now accepted and ready to land.Jan 9 2018, 5:03 AM
protze.joachim added inline comments.
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 added inline comments.Jan 9 2018, 5:38 AM
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 added inline comments.Jan 9 2018, 6:04 AM
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 added inline comments.Jan 9 2018, 6:56 AM
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 added inline comments.Jan 9 2018, 12:15 PM
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 added inline comments.Jan 9 2018, 12:51 PM
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 accepted this revision.Jan 10 2018, 4:37 AM

Ok, I think, I now understood the change.

This revision was automatically updated to reflect the committed changes.