This is an archive of the discontinued LLVM Phabricator instance.

Fix assertion that arises when waiting for proxy tasks on runtime shutdown
ClosedPublic

Authored by Hahnfeld on Sep 10 2015, 3:18 AM.

Details

Summary

This only triggered when built in debug mode with OMPT enabled:
__kmp_wait_template expected the state of the current thread to be either ompt_state_idle or ompt_state_wait_barrier{,_implicit,_explicit}.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld updated this revision to Diff 34426.Sep 10 2015, 3:18 AM
Hahnfeld retitled this revision from to Fix assertion that arises when waiting for proxy tasks on runtime shutdown.
Hahnfeld updated this object.
Hahnfeld set the repository for this revision to rL LLVM.
AndreyChurbanov accepted this revision.Sep 10 2015, 3:43 AM
AndreyChurbanov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 10 2015, 3:43 AM
This revision was automatically updated to reflect the committed changes.

The problem is not the OMPT callbacks; the problem was the state in which they were reached.

Not making the OMPT callbacks while waiting for shutdown is a mistake. If there is waiting happening, tools want to know.

I'm not sure how the state of the runtime should be classified at kmp_runtime.c:3992 (e.g., overhead, task wait, barrier?), but undefined is not a good option. I think that there needs to be a thoughtful choice for the state in kmp_runtime.c:3992 and then corresponding changes to kmp_wait_template.h.

If the state should not be idle or barrier at kmp_runtime.c:3992, then another if/else should be added in the changed regions of kmp_wait_template.h to properly handle another state in a meaningful fashion.

The problem is not the OMPT callbacks; the problem was the state in which they were reached.

Not making the OMPT callbacks while waiting for shutdown is a mistake. If there is waiting happening, tools want to know.

I'm not sure how the state of the runtime should be classified at kmp_runtime.c:3992 (e.g., overhead, task wait, barrier?), but undefined is not a good option. I think that there needs to be a thoughtful choice for the state in kmp_runtime.c:3992 and then corresponding changes to kmp_wait_template.h.

If the state should not be idle or barrier at kmp_runtime.c:3992, then another if/else should be added in the changed regions of kmp_wait_template.h to properly handle another state in a meaningful fashion.

Sorry for not giving reasons about the decision:
First, this case only happens when a #pragma omp target nowait exceeds the runtime of the master thread:

int main()
{
	#pragma omp target nowait
	{
		sleep(10);
	}

	return EXIT_SUCCESS;
}

(In my opinion this can't do anything meaningful in real code because the results of that target region cannot be handled anywhere on the host)

When leaving the main function the runtime library begins shutdown. So in my understanding the state is neither

  • ompt_state_wait_barrier because there is only one thread waiting for the proxy tasks to finish nor
  • ompt_state_idle because the thread isn't "waiting to work on an OpenMP parallel region" as the TR describes

In the standard's view the thread is already finished because it won't do any user work in its remaining lifetime. That's also why I think tools might not be really interested in those possible events.

But maybe I've overlooked a matching state. Or we could define a new one...