[OpenMP][NFC] Add a couple of TODOs to the runtime

[jdoerfert]

Summary

The runtime contains various cases of functions and variables/members
hat are not used or which seem to be overly complicated. This patch
points two such cases out so we can remove them later on (with the
appropriate changes in the code generation).

[merge_guards_bot]

Unit tests: pass. 61134 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[JonChesterfield]

openmp/runtime/src/kmp.h

2122

This does look strange.

Am I right in thinking that the two instances of the union are both used, one for the priority field and the other used for the destructors field? I haven't checked the corresponding codegen in clang.

[jdoerfert]

I'll look into how much work versioning for this type is, especially after we do not expose it to the frontends anymore.

openmp/runtime/src/kmp.h

2122

That is basically how I understand this as well. None, one, or both can be used, depending if you have a priority and/or destructors.

[protze.joachim]

openmp/runtime/src/kmp.h

2245

It's at least used by OMPT to identify an undeferred task.
Ignoring undeferred tasks can be performance critical for tools.

[jdoerfert]

openmp/runtime/src/kmp.h

2245

I think it is only written here:

// Detachable tasks are not proxy tasks yet but could be in the future. Doing 
// the tasking setup
// when that happens is too late.
if (flags->proxy == TASK_PROXY || flags->detachable == TASK_DETACHABLE) {
  if (flags->proxy == TASK_PROXY) {
    flags->tiedness = TASK_UNTIED;
    flags->merged_if0 = 1;                                                                                                                                                                                                          
  }

Couldn't you use the above conditions to replace merged_if0 ?

[protze.joachim]

The value merged_if0 is initialized in __kmp_task_alloc from the flags argument:
https://github.com/llvm/llvm-project/blob/master/openmp/runtime/src/kmp_tasking.cpp#L1287

[protze.joachim]

openmp/runtime/src/kmp.h

2245

Ok, I checked the OMPT code again. The flag is actually used to signal ompt_task_mergeable. So I agree, we can remove this flag from my perspective.

openmp/runtime/src/kmp.h

 /*! \file */
 /*
  * kmp.h -- KPTS runtime header file.
  */
 
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef KMP_H
 #define KMP_H
 
 #include "kmp_config.h"
 
 /* #define BUILD_PARALLEL_ORDERED 1 */
 
 /* This fix replaces gettimeofday with clock_gettime for better scalability on
    the Altix.  Requires user code to be linked with -lrt. */
 //#define FIX_SGI_CLOCK
 
 /* Defines for OpenMP 3.0 tasking and auto scheduling */

 @ingroup BASIC_TYPES
 @{
 */
 
 /*!
  */
 typedef kmp_int32 (*kmp_routine_entry_t)(kmp_int32, void *);
 
+// TODO: This needs to be removed. See the TODO in kmp_task.
 typedef union kmp_cmplrdata {
   kmp_int32 priority; /**< priority specified by user for the task */
   kmp_routine_entry_t
       destructors; /* pointer to function to invoke deconstructors of
                       firstprivate C++ objects */
   /* future data */
 } kmp_cmplrdata_t;
 
 /*  sizeof_kmp_task_t passed as arg to kmpc_omp_task call  */
 /*!
  */
 typedef struct kmp_task { /* GEH: Shouldn't this be aligned somehow? */
   void *shareds; /**< pointer to block of pointers to shared vars   */
   kmp_routine_entry_t
       routine; /**< pointer to routine to call for executing task */
   kmp_int32 part_id; /**< part id for the task                          */
+  // TODO: We should *not* have two union members but two explicit members:
+  //       One for the priority, one for the destructors. That would not only
+  //       make a kmp_task fit in a single cache line (which it does not now!)
+  //       it would also make it way easier to deal with than an encoding only
+  //       described in some comments.
+  //       Once the clang codegen is gone, or someone finds the time to fix it,
+  //       the following to fields need to be replaced by:
+  //  kmp_int32 priority;
+  //  kmp_routine_entry_t destructors;
   kmp_cmplrdata_t
       data1; /* Two known optional additions: destructors and priority */
   kmp_cmplrdata_t data2; /* Process destructors first, priority second */
   /* future data */
   /*  private vars  */
 } kmp_task_t;
 
 /*!
 @}
 */
 
 typedef struct kmp_taskgroup {
   std::atomic<kmp_int32> count; // number of allocated and incomplete tasks
   std::atomic<kmp_int32>
       cancel_request; // request for cancellation of this taskgroup
typedef struct kmp_task_stack {
   kmp_taskdata_t **ts_top; // pointer to the top of stack
   kmp_int32 ts_entries; // number of entries on the stack
 } kmp_task_stack_t;
 
 #endif // BUILD_TIED_TASK_STACK
 
 typedef struct kmp_tasking_flags { /* Total struct must be exactly 32 bits */
   /* Compiler flags */ /* Total compiler flags must be 16 bits */
   unsigned tiedness : 1; /* task is either tied (1) or untied (0) */
   unsigned final : 1; /* task is final(1) so execute immediately */
+  // TODO: merged_if0 does not seem to be used in any meaningful way. Is it
+  // needed?
   unsigned merged_if0 : 1; /* no __kmpc_task_{begin/complete}_if0 calls in if0
                               code path */
   unsigned destructors_thunk : 1; /* set if the compiler creates a thunk to
                                      invoke destructors from the runtime */
   unsigned proxy : 1; /* task is a proxy task (it will be executed outside the
                          context of the RTL) */
   unsigned priority_specified : 1; /* set if the compiler provides priority
                                       setting for the task */
   unsigned detachable : 1; /* 1 == can detach */
   unsigned reserved : 9; /* reserved for compiler use */