[OpenMP][WIP] Make the kmp_depend_info type fit in 128 bits.

[jdoerfert]

Summary

NOTE: The clang codegen change is missing but this simplifies a OMPIRBuilder patch and makes the struct more compact. This is not to be commited like this!

The old type wasted 64 bits on a length value that is not used. While we
probably need to use the length value eventually, supporting a 64 bit
extend seems excessive. With this patch we have 61 bit extend (=length)
and we use the 3 highest bits for the kind encoding. Alternatively, we
could put some bits in the lower part of the base pointer.

[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

[lebedev.ri]

This & D71987 - please note that if those types are not internal to the library,
(and they are not since they are produced by clang codegen), i suspect they can not
just be changed as it would cause an API/ABI break for already-compiled code,
and even newly-compiled code with previous compiler releases.
I would expect that some versioning is needed..

[Hahnfeld]

Yes, this must not be changed for compatibility reasons. If this really brings a benefit, we need new entry functions and update the compiler(s).

[JonChesterfield]

Do we have a consensus / policy on ABI breaks?

Breaking ABI will inconvenience people mixing parts of the toolchain from different releases and different vendors. Never breaking ABI means code complexity and performance degradation.

[lebedev.ri]

In D71988#1798745, @JonChesterfield wrote:

Do we have a consensus / policy on ABI breaks?

Breaking ABI will inconvenience people mixing parts of the toolchain from different releases and different vendors. Never breaking ABI means code complexity and performance degradation.

Right now libomp.so isn't versioned, meaning only a single version can be installed at the same time
(at least that is what i see in debian). So API/ABI break as such will mean that if libomp.so
from git master is installed, clang-9-based openmp will no longer be usable, only clang from git master.
That's kinda not great to put it mildly.

[JonChesterfield]

I"d like to understand that use case. Someone compiles libomp from trunk, pastes it over the version from repos, but leaves the rest of the toolchain unchanged. Breakage seems a fair result.

I've always shipped toolchains as a cohesive package. Various binaries, libraries, header files install to some directory and reference each other. Rpath et al.

I think you're suggesting multiple compilers share a single directory containing dependencies that work with all of them. I see the cost of that setup - why would it be preferred?

[lebedev.ri]

In D71988#1798870, @JonChesterfield wrote:

I"d like to understand that use case. Someone compiles libomp from trunk, pastes it over the version from repos, but leaves the rest of the toolchain unchanged. Breakage seems a fair result.

I've always shipped toolchains as a cohesive package. Various binaries, libraries, header files install to some directory and reference each other. Rpath et al.

I think you're suggesting multiple compilers share a single directory containing dependencies that work with all of them. I see the cost of that setup - why would it be preferred?

I quite literally only stated how things are as of right now.
It is possible to install multiple clang versions sumilteniously:

$ dpkg -l | grep clang
ii  clang-10                                                    1:10~+20191227100609+c3dbd782f1e-1~exp1~20191227211208.2957 amd64        C, C++ and Objective-C compiler
ii  clang-8                                                     1:8.0.1-4                                                   amd64        C, C++ and Objective-C compiler
ii  clang-9                                                     1:9.0.1-2                                                   amd64        C, C++ and Objective-C compiler

But not so much libomp:

$ dpkg -l | grep libomp
ii  libomp-10-dev                                               1:10~+20191227100609+c3dbd782f1e-1~exp1~20191227211208.2957 amd64        LLVM OpenMP runtime - dev package
ii  libomp-10-dev-dbgsym                                        1:10~+20191227100609+c3dbd782f1e-1~exp1~20191227211208.2957 amd64        debug symbols for libomp-10-dev
ii  libomp5-10:amd64                                            1:10~+20191227100609+c3dbd782f1e-1~exp1~20191227211208.2957 amd64        LLVM OpenMP runtime
ii  libomp5-10-dbgsym:amd64                                     1:10~+20191227100609+c3dbd782f1e-1~exp1~20191227211208.2957 amd64        debug symbols for libomp5-10
$ sudo apt install libomp5-9
Reading package lists... Done
Building dependency tree       
Reading state information... Done
The following packages will be REMOVED:
  libomp-10-dev libomp-10-dev-dbgsym libomp5-10 libomp5-10-dbgsym
The following NEW packages will be installed:
  libomp5-9
0 upgraded, 1 newly installed and 39 not upgraded.
Need to get 333 kB of archives.
After this operation, 752 kB disk space will be freed.
Do you want to continue? [Y/n] ^C

Which is consistent with how libomp is installed (no major version bumps between releases!), and how clang links to it:

$ ldd <some binary built with -fopenmp> | grep omp
        libomp.so.5 => /lib/x86_64-linux-gnu/libomp.so.5 (0x00007f9b6da18000)

So it may be best to first invest into soversion bumps,etc to avoid silent earth-shattering ABI breakages.

[JonChesterfield]

Ah, right. I follow. Some groundwork to be done first then, thanks.

[jdoerfert]

I would expect that some versioning is needed..

Versioning is fine with me. Versioning the runtime would be even preferable.

If this really brings a benefit, we need new entry functions and update the compiler(s).

We go to great lengths to align to cache lines in some structs, e.g., kmp_depnode, but we are literally wasting space and alignment for others. Even if we do not adopt this, we should start to adopt some best practices.

FWIW. This was never intended to be committed like this, thus no reviewers.

[Hahnfeld]

In D71988#1799064, @jdoerfert wrote:

I would expect that some versioning is needed..

Versioning is fine with me. Versioning the runtime would be even preferable.

When would you expect bumps of the version? As an example libc++ and libstdc++ have versions, but they're not frequently bumped (once in a decade or less?) because rebuilding all your applications can be very much work. That is why I think stable interfaces for runtime libraries are very convenient whenever possible.

If this really brings a benefit, we need new entry functions and update the compiler(s).

We go to great lengths to align to cache lines in some structs, e.g., kmp_depnode, but we are literally wasting space and alignment for others. Even if we do not adopt this, we should start to adopt some best practices.

There's a difference though: kmp_depend_info_t is passed by the compiler and evaluated once by the runtime library. If I recall correctly, all information is stored in internal structures afterwards (the mentioned kmp_depnode being one of them). Thus I wonder if this change brings a measurable improvement when reading the data once is not on the critical path. Breaking the ABI on the benefit of saving a few bytes on the stack seems a bit overkill just for the sake of doing it.

[jdoerfert]

In D71988#1799791, @Hahnfeld wrote:

In D71988#1799064, @jdoerfert wrote:

If this really brings a benefit, we need new entry functions and update the compiler(s).

We go to great lengths to align to cache lines in some structs, e.g., kmp_depnode, but we are literally wasting space and alignment for others. Even if we do not adopt this, we should start to adopt some best practices.

There's a difference though: kmp_depend_info_t is passed by the compiler and evaluated once by the runtime library. If I recall correctly, all information is stored in internal structures afterwards (the mentioned kmp_depnode being one of them). Thus I wonder if this change brings a measurable improvement when reading the data once is not on the critical path. Breaking the ABI on the benefit of saving a few bytes on the stack seems a bit overkill just for the sake of doing it.

Why isn't this on the critical path? It has to be paid by the thread creating tasks, in many scenarios that is the critical path while the threads executing tasks can take a hit (though it can be the other way around). The cost is not a few bytes on the stack, never was. The cost is an additional store + load (for the i8) and an extra cache line accessed for every few kmp_depend_info_t objects.

I mentioned this before, I'll do it again: Even if we do not adopt this, we should start to adopt some best practices.

In D71988#1799791, @Hahnfeld wrote:

In D71988#1799064, @jdoerfert wrote:

I would expect that some versioning is needed..

Versioning is fine with me. Versioning the runtime would be even preferable.

When would you expect bumps of the version? As an example libc++ and libstdc++ have versions, but they're not frequently bumped (once in a decade or less?) because rebuilding all your applications can be very much work. That is why I think stable interfaces for runtime libraries are very convenient whenever possible.

If we version both, the interface and the runtime, we don't need to rebuild stuff and we can phase old interfaces out. Right now we already version the interfaces.

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 */

 */
 
 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
   struct kmp_taskgroup *parent; // parent taskgroup
   // Block of data to perform task reduction
   void *reduce_data; // reduction related info
   kmp_int32 reduce_num_data; // number of data items to reduce
 } kmp_taskgroup_t;
 
 // forward declarations
 typedef union kmp_depnode kmp_depnode_t;
 typedef struct kmp_depnode_list kmp_depnode_list_t;
 typedef struct kmp_dephash_entry kmp_dephash_entry_t;
 
 // Compiler sends us this info:
 typedef struct kmp_depend_info {
   kmp_intptr_t base_addr;
-  size_t len;
   struct {
+    kmp_uint64 len : 61;
     bool in : 1;
     bool out : 1;
     bool mtx : 1;
   } flags;
 } kmp_depend_info_t;
 
+static_assert(sizeof(kmp_depend_info_t) <= 2 * sizeof(uint64_t),
+              "KMP depend info type is larger than expected");
+
 // Internal structures to work with task dependencies:
 struct kmp_depnode_list {
   kmp_depnode_t *node;
   kmp_depnode_list_t *next;
 };
 
 // Max number of mutexinoutset dependencies per node
 #define MAX_MTX_DEPS 4

openmp/runtime/src/kmp_gsupport.cpp

#endif
 
   if (if_cond) {
     if (gomp_flags & 8) {
       KMP_ASSERT(depend);
       const size_t ndeps = (kmp_intptr_t)depend[0];
       const size_t nout = (kmp_intptr_t)depend[1];
       kmp_depend_info_t dep_list[ndeps];
 
       for (size_t i = 0U; i < ndeps; i++) {
         dep_list[i].base_addr = (kmp_intptr_t)depend[2U + i];
-        dep_list[i].len = 0U;
+        dep_list[i].flags.len = 0U;
         dep_list[i].flags.in = 1;
         dep_list[i].flags.out = (i < nout);
       }
       __kmpc_omp_task_with_deps(&loc, gtid, task, ndeps, dep_list, 0, NULL);
     } else {
       __kmpc_omp_task(&loc, gtid, task);
     }
   } else {