[Openmp-commits] [PATCH] D71988: [OpenMP][WIP] Make the kmp_depend_info type fit in 128 bits.

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D71988#1799791 <https://reviews.llvm.org/D71988#1799791>, @Hahnfeld wrote:

> In D71988#1799064 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D71988#1799791>, @Hahnfeld wrote:

> In D71988#1799064 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71988/new/

https://reviews.llvm.org/D71988