[OMPT] Fix wrong parent_task_id in serialized parallel_begin with GCC

[Hahnfeld]

Summary

Without this patch a simple #pragma omp parallel num_threads(1) leads to

ompt_event_parallel_begin: parent_task_id=3, [...], parallel_id=2, [...]
ompt_event_parallel_end: parallel_id=2, task_id=4, [...]

[jlpeyton]

LGTM.

[jmellorcrummey]

I haven't had the time to analyze this patch in detail. To me, it looks like it fixes a symptom (an incorrect argument passed to a callback) rather than an underlying cause (information about the top task frame may be improperly updated).

If the information for the task at depth 0 before the serialized parallel is what should be presented to the parallel begin callback, then is the information at depth 0 wrong? Remember, a tool using asynchronous sampling could observe this task at any time - either before or after the serialized parallel.

[Hahnfeld]

In D16714#344529, @jmellorcrummey wrote:

I haven't had the time to analyze this patch in detail. To me, it looks like it fixes a symptom (an incorrect argument passed to a callback) rather than an underlying cause (information about the top task frame may be improperly updated).

If the information for the task at depth 0 before the serialized parallel is what should be presented to the parallel begin callback, then is the information at depth 0 wrong? Remember, a tool using asynchronous sampling could observe this task at any time - either before or after the serialized parallel.

Not sure if I completely get your point...
What I have just tested is that the ids reported by ompt_get_{parallel,task}_id are correct when sampling user code. I think this is because there is a lightweight taskteam constructed on line 417 (with the patch applied)

[Hahnfeld]

Move callback up to execute in correct context.

This way, there will only be wrong ids reported between __kmp_serialized_parallel and the creation of the lightweight task.
I don't know if this can be fixed because the task is already brought to execution in __kmp_serialized_parallel so we would have to assign the correct id somewehere in there...

[Hahnfeld]

ping

[omalyshe]

Is it OK that ompt_parallel_id_new() is called several times for one region? First in line 393 and then inside kmp_serialized_parallel() it can be called once or twice.

[Hahnfeld]

In D16714#362790, @omalyshe wrote:

Is it OK that ompt_parallel_id_new() is called several times for one region? First in line 393 and then inside kmp_serialized_parallel() it can be called once or twice.

While this isn't good from a performance perspective I think this won't hurt for correctness: It will skip some ids but contiguous ids aren't required by the standard.
On the other hand I don't know if this may be required in some cases, I don't always fully understand the code paths in the runtime library...

[jlpeyton]

Olga, John,

Does this look ok now?

[omalyshe]

LGTM.

openmp/trunk/runtime/src/kmp_gsupport.c

#endif
         thr->th.ompt_thread_info.state = ompt_state_work_parallel;
     }
 #endif
 }
 
 static void
 __kmp_GOMP_serialized_parallel(ident_t *loc, kmp_int32 gtid, void (*task)(void *))
 {
-    __kmp_serialized_parallel(loc, gtid);
-
 #if OMPT_SUPPORT
+    ompt_parallel_id_t ompt_parallel_id;
     if (ompt_enabled) {
-        ompt_task_id_t ompt_task_id = __ompt_get_task_id_internal(0);
-        ompt_frame_t  *ompt_frame = __ompt_get_task_frame_internal(0);
-        kmp_info_t *thr = __kmp_threads[gtid];
-
-        ompt_parallel_id_t ompt_parallel_id = __ompt_parallel_id_new(gtid);
-        ompt_task_id_t my_ompt_task_id = __ompt_task_id_new(gtid);
+        ompt_task_info_t *task_info = __ompt_get_taskinfo(0);
+        task_info->frame.exit_runtime_frame = NULL;
 
-        ompt_frame->exit_runtime_frame = NULL;
+        ompt_parallel_id = __ompt_parallel_id_new(gtid);
 
         // parallel region callback
         if (ompt_callbacks.ompt_callback(ompt_event_parallel_begin)) {
             int team_size = 1;
             ompt_callbacks.ompt_callback(ompt_event_parallel_begin)(
-                ompt_task_id, ompt_frame, ompt_parallel_id,
+                task_info->task_id, &task_info->frame, ompt_parallel_id,
                 team_size, (void *) task,
                 OMPT_INVOKER(fork_context_gnu));
         }
+    }
+#endif
+
+    __kmp_serialized_parallel(loc, gtid);
+
+#if OMPT_SUPPORT
+    if (ompt_enabled) {
+        kmp_info_t *thr = __kmp_threads[gtid];
+
+        ompt_task_id_t my_ompt_task_id = __ompt_task_id_new(gtid);
 
         // set up lightweight task
         ompt_lw_taskteam_t *lwt = (ompt_lw_taskteam_t *)
             __kmp_allocate(sizeof(ompt_lw_taskteam_t));
         __ompt_lw_taskteam_init(lwt, thr, gtid, (void *) task, ompt_parallel_id);
         lwt->ompt_task_info.task_id = my_ompt_task_id;
         lwt->ompt_task_info.frame.exit_runtime_frame = 0;
         __ompt_lw_taskteam_link(lwt, thr);