[OpenMP][OMPT] Unnecessary execution of the while loop inside __ompt_get_task_info_internal

[vladaindjic]

Summary

__ompt_get_task_info_internal now recognizes if the task at the
requested ancestor_level doesn’t exist. This is done by recognizing
whether all tasks have been exhausted (“taskdata” doesn’t exist) during
the iteration over the hierarchy of active tasks. If so, there’s no need
to continue executing the while loop. Break the loop and return 0 code
instead.

This patch prevents the SEGFAULT that may arise if the the loop unnecessarily
continues instead (the code before the patch). Assume that the
__ompt_get_task_info_internal is called with the argument “ancestor_level == 2”,
while the initial master thread is executing the initial task.
After the while loop finishes, both “prev_lwt” and “prev_team” are equal to NULL.
If the called function should determine the “thread_num”, it will do that by
executing the assignment “*thread_num = prev_team->t.t_master_tid”.
Since “prev_team” is NULL, dereferencing it leads to a SEGFAULT.

Also, this patch may be considered as a simple performance optimization.
Assume that the hierarchy of active tasks contains two tasks.
If the __ompt_get_task_info_internal is called with the argument
“ancestor_level==1000”, there’s no need to execute more that two iterations
of the while before detecting that the task at the requested “ancestor_level”
doesn’t exist.

This patch provides the simple test cases that validates the behaviour of
the __ompt_get_task_info_internal function in case when the task doesn’t exist
at the requested “ancestor_level”.

[vladaindjic]

@protze.joachim According to the standard specification which claims the following: “If no task region exists at the specified ancestor level or the information is unavailable then the values of variables passed by reference to the entry point are undefined when ompt_get_task_info returns.”, it is not safe to use “task_data” and “parallel_data” before previously asserting that “ret == 2” inside ompt-multiplex.h test sections.

This check ("ret == 2") prevents failing the following test cases:

1. multiplex.custom_data_storage::custom_data_storage.c
2. OMPT multiplex.print::print.c

[protze.joachim]

Please update the diff with context (-U9999)

[vladaindjic]

The diffs have been updated through arc diff tool instead of web form.

@protze.joachim I was using the following command "arc diff --update <revision_id>".
Is that enought to supply the diff context?

openmp/runtime/src/ompt-specific.cpp

while (ancestor_level > 0) {
         } else {
           // then go for implicit tasks
           taskdata = taskdata->td_parent;
           if (team == NULL)
             return 0;
           team = team->t.t_parent;
           if (taskdata) {
             next_lwt = LWT_FROM_TEAM(taskdata->td_team);
+          } else {
+            // All tasks have been exhausted, so there's no task at the
+            // requested ancestor_level. Thus, stop iterating and return 0.
+            return 0;
           }
         }
       }
       ancestor_level--;
     }
 
     if (lwt) {
       info = &lwt->ompt_task_info;

openmp/runtime/test/ompt/taskinfo/no_task.c

+// RUN: %libomp-compile-and-run | FileCheck %s
+// REQUIRES: ompt
+
+#include "callback.h"
+#include <omp.h>
+
+#define NO_TASK_ID 0xBAD
+
+__attribute__ ((noinline)) // workaround for bug in icc
+void print_task_info_at(int ancestor_level, int id)
+{
+#pragma omp critical
+  {
+    int task_type;
+    char buffer[2048];
+    ompt_data_t *parallel_data;
+    ompt_data_t *task_data;
+    int thread_num;
+    int ret = ompt_get_task_info(ancestor_level, &task_type, &task_data, NULL,
+                       &parallel_data, &thread_num);
+    if (ret == 2) {
+      format_task_type(task_type, buffer);
+      printf("%" PRIu64 ": ret=%d ancestor_level=%d id=%d task_type=%s=%d "
+                        "parallel_id=%" PRIu64 " task_id=%" PRIu64
+                        " thread_num=%d\n",
+          ompt_get_thread_data()->value, ret, ancestor_level, id, buffer,
+          task_type, parallel_data->value, task_data->value, thread_num);
+    } else {
+      // Either task doesn't exist at the requested ancestor_level
+      // or the information about it is not available.
+      printf("%" PRIu64 ": ret=%d\n", ompt_get_thread_data()->value, ret);
+    }
+  }
+};
+
+int main()
+{
+  //initial task
+  print_task_info_at(0, 0);
+  // no task at this level
+  print_task_info_at(1, NO_TASK_ID);
+  // no task at this level
+  print_task_info_at(2, NO_TASK_ID);
+  // no task at this level
+  print_task_info_at(1000, NO_TASK_ID);
+
+#pragma omp parallel num_threads(2)
+  {
+    // region 0
+#pragma omp parallel num_threads(2)
+    {
+#pragma omp master
+#pragma omp task if(0)
+      {
+#pragma omp task if(0)
+        {
+          // info about the innermost task
+          print_task_info_at(0, 4);
+          // info about the outer task
+          print_task_info_at(1, 3);
+          // info about inner region
+          print_task_info_at(2, 2);
+          // info about outer region
+          print_task_info_at(3, 1);
+          // info about initial task
+          print_task_info_at(4, 0);
+          // no task at higher levels
+          print_task_info_at(5, NO_TASK_ID);
+          print_task_info_at(6, NO_TASK_ID);
+          print_task_info_at(1000, NO_TASK_ID);
+        }
+      }
+    }
+  }
+
+  // Check if libomp supports the callbacks for this test.
+  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_task_create'
+  // CHECK-NOT: {{^}}0: Could not register callback 'ompt_callback_implicit_task'
+
+  // CHECK: {{^}}0: NULL_POINTER=[[NULL:.*$]]
+  // CHECK: {{^}}[[MASTER_ID:[0-9]+]]: ompt_event_initial_task_begin
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=0 id=0 task_type=ompt_task_initial
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+
+  // check information inquired from the innermost explicit tasks
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=0 id=4 task_type=ompt_task_explicit
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=1 id=3 task_type=ompt_task_explicit
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=2 id=2 task_type=ompt_task_implicit
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=3 id=1 task_type=ompt_task_implicit
+  // CHECK: {{^}}[[MASTER_ID]]: ret=2 ancestor_level=4 id=0 task_type=ompt_task_initial
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+  // CHECK: {{^}}[[MASTER_ID]]: ret=0
+
+  return 0;
+}

openmp/tools/multiplex/ompt-multiplex.h

int ompt_multiplex_own_get_task_info(int ancestor_level, int *type,
                                      ompt_data_t **task_data,
                                      ompt_frame_t **task_frame,
                                      ompt_data_t **parallel_data,
                                      int *thread_num) {
   int ret = ompt_multiplex_get_task_info(ancestor_level, type, task_data,
                                          task_frame, parallel_data, thread_num);
 
 #ifndef OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_TASK_DATA
-  if (task_data)
+  // ret == 2 assures that the task at the ancestor_level exists and that the
+  // information about it is present.
+  if (ret == 2 && task_data)
     *task_data = ompt_multiplex_get_own_ompt_data(*task_data);
 #endif
 #ifndef OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_PARALLEL_DATA
-  if (parallel_data)
+  // ret == 2 assures that the task at the ancestor_level exists and that the
+  // information about it is present.
+  if (ret == 2 && parallel_data)
     *parallel_data = ompt_multiplex_get_own_ompt_data(*parallel_data);
 #endif
   return ret;
 }
 
 int ompt_multiplex_client_get_task_info(int ancestor_level, int *type,
                                         ompt_data_t **task_data,
                                         ompt_frame_t **task_frame,
                                         ompt_data_t **parallel_data,
                                         int *thread_num) {
   int ret = ompt_multiplex_get_task_info(ancestor_level, type, task_data,
                                          task_frame, parallel_data, thread_num);
 
-  if (task_data)
+  // ret == 2 assures that the task at the ancestor_level exists and that the
+  // information about it is present.
+  if (ret == 2 && task_data)
 #ifndef OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_TASK_DATA
     *task_data = ompt_multiplex_get_client_ompt_data(*task_data);
 #else
     *task_data = OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_TASK_DATA(*task_data);
 #endif
 
-  if (parallel_data)
+  // ret == 2 assures that the task at the ancestor_level exists and that the
+  // information about it is present.
+  if (ret == 2 && parallel_data)
 #ifndef OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_PARALLEL_DATA
     *parallel_data = ompt_multiplex_get_client_ompt_data(*parallel_data);
 #else
     *parallel_data =
         OMPT_MULTIPLEX_CUSTOM_GET_CLIENT_PARALLEL_DATA(*parallel_data);
 #endif
   return ret;
 }