On Thu, May 28, 2026 at 09:07:23AM +0100, Matthew Malcomson wrote:
> Working on PR119588. From that PR:
> We've seen on some internal workloads (NVPL BLAS running GEMM routine
> on a small matrix) that the overhead of a `#pragma omp parallel`
> statement when running with a high number of cores (72 or 144) is much
> higher with the libgomp implementation than with LLVM's libomp.
>
> In a program which has both some work that can be handled with high
> parallelism (so OMP is running with many threads) and a large number
> of small pieces of work that need to be performed with low overhead,
> this has been seen to cause a significant overhead when accumulated.
Thanks for working on this.
> ------------------------------
> Points that I would like feedback on:
> - Is this extra complexity OK, especially for the futex_waitv
> fallback?
> - I have thought about creating yet another target (in the same way
> that the linux/ target is for kernels where `futex` is available).
> Would this approach be favoured?
I think I'd prefer separate config/linux and config/linux/futex_waitv/
with no futex_waitv fallback in the latter, let configure detect if
SYS_futex_waitv is available and in that case prepend linux/futex_waitv
to config_path (and have --enable-linux-futex-waitv to let users override,
so one can test --disable-linux-futex-waitv --enable-linux-futex etc.
even when the syscall exists).
config/linux/futex_waitv would override of course only the files it
needs (bar.h, bar.c, wait.h, add futex_waitv.h) and rest would be
shared.
> - Are the changes in `gomp_team_start` significant enough for other
> targets that it's worth bringing it into the linux/ target?
> Similar to what I see the nvptx/ target has done for
> `gomp_team_start` Could have some `ifdef` for the alternate barrier
> API rather than try to modify all the other interfaces to match a
> slightly modified API.
Please avoid ifdefs if possible, so yes, if some argument is not needed
on other targets, have inline wrapper have it unused where it is not needed.
I'll note this patch is not self-contained because it lacks all the
config/{gcn,nvptx,rtems,etc.} changes that are required by this patch,
so either those changes should be moved from the 3rd patch into this one,
or both patches need to be committed together, we shouldn't leave trunk
broken in between revisions.
> libgomp/ChangeLog:
>
> * barrier.c (GOMP_barrier, GOMP_barrier_cancel): Pass team_id
> to barrier functions.
> * config/linux/bar.c (gomp_barrier_ensure_last): New.
> (gomp_assert_and_increment_flag): New.
> (gomp_team_barrier_ensure_last): New.
> (gomp_assert_and_increment_cancel_flag): New.
> (gomp_team_barrier_ensure_cancel_last): New.
Instead of saying just New., please state what it is, New function.,
New macro. (or Define.), New type. etc.
> (gomp_barrier_wait_end): Extra assertions.
Add extra assertions. ?
>
> void
> -gomp_barrier_wait_end (gomp_barrier_t *bar, gomp_barrier_state_t state)
> +gomp_assert_and_increment_flag (gomp_barrier_t *bar, unsigned id, unsigned
> gens)
This is too long, please wrap it.
> + else
> + {
> + gomp_assert (orig == gen, "Original flag %u != generation of %u\n",
> orig,
> + gen);
> + }
Please avoid {}s around a single statement body. Now, this is clearly
because the gomp_assert macro is bad:
+#if _LIBGOMP_CHECKING_
+#define gomp_assert(EXPR, MSG, ...)
\
+ if (!(EXPR))
\
+ gomp_fatal ("%s:%d " MSG, __FILE__, __LINE__, __VA_ARGS__)
+#else
+#define gomp_assert(...)
+#endif
it really should be wrapped in do ... while (0) in both cases, so that
missing ; after it is diagnosed, it can be used inside of else alone,
or else if (cond) gomp_assert (...); else etc.
> + /* When running as a logical "final" barrier the `CANCELLED` flag
> + can get set on the barrier if we were in a cancellable region
> + whose only barrier was the implicit "final" barrier at the end
> + of the region. Ensure we update `gstate` in this case to
> + avoid any unnecessary busy-waiting when nothing is actually
> + changing. */
> + gomp_assert (!(gstate & BAR_CANCELLED) || (gen & BAR_CANCELLED),
> + "BAR_CANCELLED state cleared by non-primary thread"
> + " bar->generation: %u state: %u",
> + gen, gstate);
Formatting, the later lines should be aligned below !. So
gomp_assert (!(gstate & BAR_CANCELLED) || (gen & BAR_CANCELLED),
"BAR_CANCELLED state cleared by non-primary thread"
" bar->generation: %u state: %u", gen, gstate);
> + gstate |= gen & BAR_CANCELLED;
> +
> + gomp_assert (
> + !(gen & BAR_WAITING_FOR_TASK),
> + "BAR_WAITING_FOR_TASK set by non-primary thread gen=%d", gen);
> + gomp_assert (
> + !gomp_barrier_state_is_incremented (gen, gstate, BAR_INCR),
> + "Global state incremented by non-primary thread gen=%d", gen);
Please avoid this formatting style, ( shouldn't come at the end of line
unless really necessary, it is too ugly.
gomp_assert (!(gen & BAR_WAITING_FOR_TASK),
"BAR_WAITING_FOR_TASK set by non-primary thread "
"gen=%d", gen);
gomp_assert (!gomp_barrier_state_is_incremented (gen, gstate,
BAR_INCR),
"Global state incremented by non-primary thread "
"gen=%d", gen);
looks nicer.
> +
> +static inline void
> +gomp_barrier_init (gomp_barrier_t *bar, unsigned count)
> +{
> + bar->threadgens
> + = gomp_aligned_alloc (64, sizeof (bar->threadgens[0]) * count);
> + for (unsigned i = 0; i < count; ++i)
> + {
> + bar->threadgens[i].gen = 0;
> + bar->threadgens[i].cgen = 0;
> + }
I'm a little bit worried about this extra threadgens allocation.
Can't it be allocated together with the rest of team?
size_t extra = sizeof (team->ordered_release[0])
+ sizeof (team->implicit_task[0]);
#ifdef GOMP_USE_ALIGNED_WORK_SHARES
team = gomp_aligned_alloc (__alignof (struct gomp_team),
sizeof (*team) + nthreads * extra);
#else
team = team_malloc (sizeof (*team) + nthreads * extra);
#endif
#ifndef HAVE_SYNC_BUILTINS
gomp_mutex_init (&team->work_share_list_free_lock);
#endif
gomp_barrier_init (&team->barrier, nthreads);
gomp_mutex_init (&team->task_lock);
Through some extra inline function add it as another extra,
__alignof (struct gomp_team) is already necessarily 64 byte
aligned because it contains gomp_barrier_t.
There is the reinit case.
And, does it have to be allocated at all even if nthreads
is really small (say <= 16 or so)?
Has performance been compared even in the low number of threads
cases (i.e. shouldn't we dynamically choose between the hyper strategy
and the linear one)? Not everybody even now has lots of CPUs and
OpenMP is used even in the low number of threads cases, 2, 4, 8, 16...
Jakub
