This is an archive of the discontinued LLVM Phabricator instance.

__kmp_free_task: Fix for serial explicit tasks producing proxy tasks
ClosedPublic

Authored by Hahnfeld on Aug 3 2016, 4:13 AM.

Details

Summary

Consider the following code which may be executed by a serial team:

int dep;
#pragma omp target nowait depend(out: dep)
{
    sleep(1);
}
#pragma omp task depend(in: dep)
{
    #pragma omp target nowait
    {
        sleep(1);
    }
}

Here the explicit task may not be freed until the nested proxy task has
finished. The current code hasn't considered this and called __kmp_free_task
anyway which triggered an assert because of remaining incomplete children:

KMP_DEBUG_ASSERT( TCR_4(taskdata->td_incomplete_child_tasks) == 0 );

Diff Detail

Event Timeline

Hahnfeld updated this revision to Diff 66641.Aug 3 2016, 4:13 AM
Hahnfeld retitled this revision from to __kmp_free_task: Fix for serial explicit tasks producing proxy tasks.
Hahnfeld updated this object.
Hahnfeld added reviewers: jlpeyton, AndreyChurbanov.
Hahnfeld added a subscriber: openmp-commits.
hfinkel added a subscriber: hfinkel.Aug 3 2016, 4:19 AM

Please add your example as a regression test case.

Please add your example as a regression test case.

Hmm, that will be tricky: Currently only the Intel Compiler will generate proxy tasks for target nowait...

Please add your example as a regression test case.

Hmm, that will be tricky: Currently only the Intel Compiler will generate proxy tasks for target nowait...

In general, I'd still add the test (just with a comment that it had failed using the Intel compiler). I presume that the test suite runs with the Intel compiler too. Since this uses the target directive, however, I'm not sure it will run with Clang currently, so we might need to wait.

Please add your example as a regression test case.

Hmm, that will be tricky: Currently only the Intel Compiler will generate proxy tasks for target nowait...

In general, I'd still add the test (just with a comment that it had failed using the Intel compiler). I presume that the test suite runs with the Intel compiler too. Since this uses the target directive, however, I'm not sure it will run with Clang currently, so we might need to wait.

Hmm, I don't quite see much sense in a test that would not fail with the compilers the tests are most commonly (and automatically) run with.
Maybe we could reuse the idea of tasking/kmp_taskloop.c where the compiler generated code is manually emitted...

Please add your example as a regression test case.

Hmm, that will be tricky: Currently only the Intel Compiler will generate proxy tasks for target nowait...

In general, I'd still add the test (just with a comment that it had failed using the Intel compiler). I presume that the test suite runs with the Intel compiler too. Since this uses the target directive, however, I'm not sure it will run with Clang currently, so we might need to wait.

Hmm, I don't quite see much sense in a test that would not fail with the compilers the tests are most commonly (and automatically) run with.

We probably should have a buildbot for libomp that uses the Intel compiler so long as that's a configuration we don't want to break. Regardless, this is already an issue for lots of different reasons. We have tests that only fail with asserts enabled, we have tests for the JIT that only represent bugs on certain host platforms, etc. A test that everyone can use to verify correct behavior is best, but even if not, there is still value in it.

Maybe we could reuse the idea of tasking/kmp_taskloop.c where the compiler generated code is manually emitted...

That works too.

Hahnfeld updated this revision to Diff 66794.Aug 4 2016, 6:16 AM

Add test emulating what I think the Intel Compiler generates and which shows the issue

Maybe we could reuse the idea of tasking/kmp_taskloop.c where the compiler generated code is manually emitted...

That works too.

But it is not much fun. We should think about splitting kmp.h to make the compiler facing functions easily available in the tests which I have for now duplicated

AndreyChurbanov requested changes to this revision.Aug 4 2016, 7:33 AM
AndreyChurbanov edited edge metadata.
AndreyChurbanov added inline comments.
runtime/src/kmp_tasking.c
599

This change breaks the following code:

#pragma omp task
{
  #pragma omp task
  {
  }
}

The problem is that for a serial task its parent task is most likely still running and thus cannot be freed prematurely.
To me, the correct fix would be to remember the status of initial task at the beginning of the routine, e.g.

kmp_int32 task_serial = taskdata->td_flags.task_serial;

then in the loop check this condition:

if ( task_serial || taskdata -> td_flags.tasktype == TASK_IMPLICIT )
    return;

I think checking task_serial flag here is better than team_serial or tasking_serial (as was done before the change), because a task can be serialized even if team is active and tasking is active (e.g. no room in thread's task queue).

This revision now requires changes to proceed.Aug 4 2016, 7:33 AM
Hahnfeld added inline comments.Aug 4 2016, 8:04 AM
runtime/src/kmp_tasking.c
599

I'm afraid your proposal doesn't work as expected: I've just committed another test for nested task creation (I thought there already was one) and it will fail with this patch. Will have a deeper look at it tomorrow

Hahnfeld updated this revision to Diff 67128.Aug 8 2016, 12:14 AM
Hahnfeld edited edge metadata.

After some offline discussion with Andrey, this seems to work and doesn't leak any allocated task structs

AndreyChurbanov accepted this revision.Aug 8 2016, 2:47 AM
AndreyChurbanov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 8 2016, 2:47 AM
This revision was automatically updated to reflect the committed changes.