[Openmp-commits] [PATCH] D107496: [OpenMP] Fix task wait doesn't work as expected in serialized team

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

You are right with your statement, that the two are not completely equivalent.

Consider the following sequence:

  #pragma omp task detach(e) depend(out:a)
  {}
  #pragma omp taskwait depend(in:a)
  #pragma omp task
  {}

Your approach will enforce reference counting of child tasks until the end of the parallel region (remember that also `main` has an implicit inactive parallel region!). So, you will in/decrement the counter also for the last task.

My approach will stop reference counting, as soon as no other task is alive in the team and the task is really serialized. The race condition for at `td_allocated_child_tasks>0` and then possibly incrementing from `td_allocated_child_tasks=0` is actually harmless, because the same task will decrement the value at the end of it's execution.

For a really serialized task (which does no reference counting), no other task can be created during it's execution, so that `td_allocated_child_tasks` cannot be >0 at the end of execution.
Similar reasoning holds for your approach: for a serialized task, neither of the two flags in task_team can be set while the task executes and they are never set back to 0, aren't they?

The only requirement is, that the same set of flags is checked for the creation and end of task execution; and it is also important, that the values cannot change between the two encountering. I think, we should collect the flags into a macro/function to make sure we keep them symmetric.

With more and more conditions to this "fast path" for serialized tasks: when does the number of conditions and checks become more expensive than the operations we try to avoid - atomic writes to some variable with no contention?



================
Comment at: openmp/runtime/src/kmp_tasking.cpp:1411
     if (flags->hidden_helper) {
       taskdata->td_flags.task_serial = FALSE;
       // Increment the number of hidden helper tasks to be executed
----------------
Btw: How can this be valid? From my understanding, `task_serial` marks the task as undeferred.  Execution cannot continue before the task finished execution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107496/new/

https://reviews.llvm.org/D107496