Fix performance regression in SPEC kdtree test

[jlpeyton]

Summary

Make __ompt_implicit_task_end a static function and remove the inline part. This fixes small regression in SPEC kdtree benchmark. Also reformat some of __ompt_implicit_task_end.

[Hahnfeld]

runtime/src/kmp_wait_release.h

254–261

Are you sure that you want to remove this lookup? This means always passing NULL AFAICS

[AndreyChurbanov]

runtime/src/kmp_wait_release.h

254–261

Once the value of pId is never used it should be OK to have it NULL for now. Increased code size impacts performance of some tests, so we try to eliminate unused code when possible.

[Hahnfeld]

runtime/src/kmp_wait_release.h

262

Well, it's used in this event.

[AndreyChurbanov]

runtime/src/kmp_wait_release.h

262

Here is the complete code of the event you mentioned:

static inline void __ompt_implicit_task_end(kmp_info_t *this_thr,
                                          ompt_state_t ompt_state,
                                          ompt_data_t *tId,
                                          ompt_data_t *pId) {
int ds_tid = this_thr->th.th_info.ds.ds_tid;
if (ompt_state == ompt_state_wait_barrier_implicit) {
  this_thr->th.ompt_thread_info.state = ompt_state_overhead;
  void *codeptr = NULL;
  if (ompt_enabled.ompt_callback_sync_region_wait) {
    ompt_callbacks.ompt_callback(ompt_callback_sync_region_wait)(
        ompt_sync_region_barrier, ompt_scope_end, NULL, tId, codeptr);
  }
  if (ompt_enabled.ompt_callback_sync_region) {
    ompt_callbacks.ompt_callback(ompt_callback_sync_region)(
        ompt_sync_region_barrier, ompt_scope_end, NULL, tId, codeptr);
  }
  if (!KMP_MASTER_TID(ds_tid)) {
    if (ompt_enabled.ompt_callback_implicit_task) {
      ompt_callbacks.ompt_callback(ompt_callback_implicit_task)(
          ompt_scope_end, NULL, tId, 0, ds_tid);
    }
    // return to idle state
    this_thr->th.ompt_thread_info.state = ompt_state_idle;
  } else {
    this_thr->th.ompt_thread_info.state = ompt_state_overhead;
  }
}
}

The pId parameter is not used here.

[Hahnfeld]

runtime/src/kmp_wait_release.h

262

Ah, didn't look into the function. Can we then remove the argument and the variable completely?

[protze.joachim]

runtime/src/kmp_wait_release.h

262

I would also suggest to completely remove pId here.
The final OpenMP 5.0 spec says, that parallel_data here is always NULL, so it is not needed anymore.

[jlpeyton]

Removed pId variable

[AndreyChurbanov]

LGTM

[protze.joachim]

LGTM

runtime/src/kmp_wait_release.h

public:
   int execute_tasks(kmp_info_t *this_thr, kmp_int32 gtid, int final_spin,
                     int *thread_finished
                     USE_ITT_BUILD_ARG(void * itt_sync_obj), kmp_int32
                     is_constrained);
   */
 };
 
 #if OMPT_SUPPORT
-static inline void __ompt_implicit_task_end(kmp_info_t *this_thr,
-                                            omp_state_t omp_state,
-                                            ompt_data_t *tId,
-                                            ompt_data_t *pId) {
+OMPT_NOINLINE
+static void __ompt_implicit_task_end(kmp_info_t *this_thr,
+                                     omp_state_t omp_state, ompt_data_t *tId) {
   int ds_tid = this_thr->th.th_info.ds.ds_tid;
   if (omp_state == omp_state_wait_barrier_implicit) {
     this_thr->th.ompt_thread_info.state = omp_state_overhead;
 #if OMPT_OPTIONAL
     void *codeptr = NULL;
     if (ompt_enabled.ompt_callback_sync_region_wait) {
       ompt_callbacks.ompt_callback(ompt_callback_sync_region_wait)(
           ompt_sync_region_barrier, ompt_scope_end, NULL, tId, codeptr);
__kmp_fork_barrier  (implicit barrier at end of parallel region)
                 state := omp_state_idle
 
        Before leaving, if state = omp_state_idle
             idle-end
                 state := omp_state_overhead
 */
 #if OMPT_SUPPORT
   omp_state_t ompt_entry_state;
-  ompt_data_t *pId = NULL;
   ompt_data_t *tId;
   if (ompt_enabled.enabled) {
     ompt_entry_state = this_thr->th.ompt_thread_info.state;
     if (!final_spin || ompt_entry_state != omp_state_wait_barrier_implicit ||
         KMP_MASTER_TID(this_thr->th.th_info.ds.ds_tid)) {
       ompt_lw_taskteam_t *team =
           this_thr->th.th_team->t.ompt_serialized_team_info;
       if (team) {
-        pId = &(team->ompt_team_info.parallel_data);
         tId = &(team->ompt_task_info.task_data);
       } else {
-        pId = OMPT_CUR_TEAM_DATA(this_thr);
         tId = OMPT_CUR_TASK_DATA(this_thr);
       }
     } else {
-      pId = NULL;
       tId = &(this_thr->th.ompt_thread_info.task_data);
     }
-        if (final_spin && (__kmp_tasking_mode == tskm_immediate_exec ||
-                           this_thr->th.th_task_team == NULL)) {
       // implicit task is done. Either no taskqueue, or task-team finished
-      __ompt_implicit_task_end(this_thr, ompt_entry_state, tId, pId);
+      __ompt_implicit_task_end(this_thr, ompt_entry_state, tId);
     }
   }
 #endif
 
   // Setup for waiting
   KMP_INIT_YIELD(spins);
 
   if (__kmp_dflt_blocktime != KMP_MAX_BLOCKTIME) {
if (__kmp_tasking_mode != tskm_immediate_exec) {
                 &tasks_completed USE_ITT_BUILD_ARG(itt_sync_obj), 0);
           else
             this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
         } else {
           KMP_DEBUG_ASSERT(!KMP_MASTER_TID(this_thr->th.th_info.ds.ds_tid));
 #if OMPT_SUPPORT
           // task-team is done now, other cases should be catched above
           if (final_spin && ompt_enabled.enabled)
-            __ompt_implicit_task_end(this_thr, ompt_entry_state, tId, pId);
+            __ompt_implicit_task_end(this_thr, ompt_entry_state, tId);
 #endif
           this_thr->th.th_task_team = NULL;
           this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
         }
       } else {
         this_thr->th.th_reap_state = KMP_SAFE_TO_REAP;
       } // if
     } // if
#endif
     // TODO: If thread is done with work and times out, disband/free
   }
 
 #if OMPT_SUPPORT
   omp_state_t ompt_exit_state = this_thr->th.ompt_thread_info.state;
   if (ompt_enabled.enabled && ompt_exit_state != omp_state_undefined) {
 #if OMPT_OPTIONAL
     if (final_spin) {
-      __ompt_implicit_task_end(this_thr, ompt_exit_state, tId, pId);
+      __ompt_implicit_task_end(this_thr, ompt_exit_state, tId);
       ompt_exit_state = this_thr->th.ompt_thread_info.state;
     }
 #endif
     if (ompt_exit_state == omp_state_idle) {
       this_thr->th.ompt_thread_info.state = omp_state_overhead;
     }
   }
 #endif