Page MenuHomePhabricator

Add support for ompt_event_task_dependences and ompt_event_task_dependence_pair
ClosedPublic

Authored by harald.servat on Nov 17 2015, 6:06 AM.

Details

Summary

The attached patch adds support for ompt_event_task_dependences and ompt_event_task_dependence_pair events from the OMPT specification [1]. These events only apply to OpenMP 4.0 and 4.1 (aka 4.5) because task dependencies were introduced in 4.0.

With respect to the changes:

ompt_event_task_dependences
According to the specification, this event is raised after the task has been created, thefore this event needs to be raised after ompt_event_task_begin (in _kmp_task_start). However, the dependencies are known at _kmpc_omp_task_with_deps which occurs before _kmp_task_start. My modifications extend the ompt_task_info_t struct in order to store the dependencies of the task when _kmpc_omp_task_with_deps occurs and then they are emitted in _kmp_task_start just after raising the ompt_event_task_begin. The deps field is allocated and valid until the event is raised and it is freed and set to null afterwards.

ompt_event_task_dependence_pair
The processing of the dependences (i.e. checking whenever a dependence is already satisfied) is done within _kmp_process_deps. That function checks every dependence and calls the _kmp_track_dependence routine which gives some support for graphical output. I used that routine to emit the dependence pair but I also needed to know the sink_task. Despite the fact that the code within KMP_SUPPORT_GRAPH_OUTPUT refers to task_sink it may be null because sink->dn.task (there's a comment regarding this) and in fact it does not point to a proper pointer value because the value is set in node->dn.task = task; after the _kmp_process_deps calls in _kmp_check_deps. I have extended the _kmp_process_deps and _kmp_track_dependence parameter list to receive the sink_task.

PS: I have removed the extra leading underscore from the kmp calls because they appeared as underlined.

[1] https://github.com/OpenMPToolsInterface/OMPT-Technical-Report/blob/target/ompt-tr.pdf

Diff Detail

Repository
rL LLVM

Event Timeline

harald.servat retitled this revision from to Add support for ompt_event_task_dependences and ompt_event_task_dependence_pair.
harald.servat updated this object.
harald.servat updated this object.
harald.servat updated this object.
harald.servat updated this object.
harald.servat updated this object.

Overall, the patch looks good. Sorry that it has taken me so long to review it.

A few minor quibbles:

The # if USE_FAST_MEMORY is used to in two places to select between allocation and free routines. I think it would be better to use it in only one place and say

#if USE_FAST_MEMORY
#define KMP_OMPT_DEPS_ALLOC kmp_fast_allocate
#define KMP_OMPT_DEPS_FREE kmp_fast_free
#else
#define KMP_OMPT_DEPS_ALLOC kmp_thread_malloc
#define KMP_OMPT_DEPS_FREE kmp_thread_free
#endif

and then use the macros defined above in the OMPT code. Note: in the above definitions, I omitted the leading underscores, which created underlined phrases in the formatting.

The comments in the ompt.h.var files shouldn't say "new task dependences" and "new task dependence pair". It would be better to say "report task dependences" and "report task dependence pair".

harald.servat edited edge metadata.

I have addressed John M. Crummey's comments.

I have added the macros KMP_OMPT_DEPS_ALLOC/KMP_OMPT_DEPS_FREE into ompt-internal.h. I also have modified the comments regarding the added events into the ompt.h.var files.

Changes LGTM as well but compilation fails with LIBOMP_OMPT_SUPPORT=ON and LIBOMP_OMP_VERSION=30 (rather theoretical combination).

(Note: kmp_taskdeps.cpp is only compiled if LIBOMP_OMP_VERSION >= 40 so you don't have to guard this)

runtime/src/kmp_tasking.c
466–481 ↗(On Diff #43515)

Please guard with #if OMP_40_ENABLED

781–782 ↗(On Diff #43515)

see above

runtime/src/ompt-event-specific.h
144–145 ↗(On Diff #43515)

see above

runtime/src/ompt-internal.h
32–33 ↗(On Diff #43515)

see above

67–75 ↗(On Diff #43515)

see above

I have addressed Jonas Hahnfeld by guarding the code with OMP_40_ENABLED.

My only concern is regarding runtime/src/ompt-event-specific.h. In that file, I have added the guard but I also provided the #else condition to mark that these events cannot be provided if OMP40 is not enabled.

Hahnfeld accepted this revision.Jan 28 2016, 1:01 AM
Hahnfeld added a reviewer: Hahnfeld.

LGTM. Shall I commit for you?

I have addressed Jonas Hahnfeld by guarding the code with OMP_40_ENABLED.

My only concern is regarding runtime/src/ompt-event-specific.h. In that file, I have added the guard but I also provided the #else condition to mark that these events cannot be provided if OMP40 is not enabled.

Good idea, this makes it explicitely clear that this is not implemented for versions prior to 4.0 (although the defines will probably never be used in this case...)

This revision is now accepted and ready to land.Jan 28 2016, 1:01 AM

LGTM. Shall I commit for you?

Yes, please. Thank you very much!

I have addressed Jonas Hahnfeld by guarding the code with OMP_40_ENABLED.

My only concern is regarding runtime/src/ompt-event-specific.h. In that file, I have added the guard but I also provided the #else condition to mark that these events cannot be provided if OMP40 is not enabled.

Good idea, this makes it explicitely clear that this is not implemented for versions prior to 4.0 (although the defines will probably never be used in this case...)

This revision was automatically updated to reflect the committed changes.