On Mon, Oct 12, 2026 at 06:00:38PM +0100, [email protected] wrote:
> libgomp/ChangeLog:
>
> PR libgomp/119588
> * libgomp.h (struct gomp_team): Add fields for communication
> at team start time. New `ti` member, new `ppv` member, new
> `reused` boolean.
> * team.c (gomp_thread_start): Initialise thread-local data using
> members stored on team.
> (get_last_team): Set `team->reused` member when reusing.
> (gomp_new_team): Unset `team->reused` member when creating new
> team.
> (gomp_team_start): Primary thread stores data on team structure
> instead of copying that data to each secondary threads
> gomp_thread structure. Skip re-calculation of places on a
> reused team when inputs are the same.
>
> Signed-off-by: Matthew Malcomson <[email protected]>
Mostly LGTM, with some formatting nits below.
> + if (!gomp_barrier_can_hold (&team->barrier) || thr->exit
> + || (thr->ts.team == team && thr->ts.team_id != team_id))
If whole condition doesn't fit on one line, then it should use one line
per || or &&. So,
if (!gomp_barrier_can_hold (&team->barrier)
|| thr->exit
|| (thr->ts.team == team && thr->ts.team_id != team_id))
> + /* Usually `thr->fn` is not used or set. Is used when secondary
s/Is/It is/ ?
> + threads are "set going" without being assigned to any team.
> + See `gomp_pause_host` and `gomp_free_thread`.
> + In this case we don't want to bother reading the team
> + initialisation things because they've not been set up for us. */
> + if (thr->fn)
If it is unlikely, then please use
if (__builtin_expect (thr->fn != NULL, 0))
instead.
> + gomp_assert (
> + nthreads == old_threads_used,
> + "odd number of threads when re-using team: nthreads = %u "
> + "old_threads_used = %u\n",
> + nthreads, old_threads_used);
Please avoid ( at the end of line if possible.
gomp_assert (nthreads == old_threads_used,
"odd number of threads when re-using team: "
"nthreads = %u old_threads_used = %u\n",
nthreads, old_threads_used);
works just fine.
> + gomp_assert (
> + nthr->ts.level == 1 && team->prev_ts.level == 0,
> + "Level indicators surprising: nthr: %u, team->prev_ts: %u\n",
> + nthr->ts.level, team->prev_ts.level);
> + gomp_assert (
> + nthr->ts.active_level == 1 && thr->ts.active_level == 1,
> + "active_level indicators surprising: nthr: %u, thr: %u\n",
> + nthr->ts.active_level, thr->ts.active_level);
Similarly (2x).
Also please start the assert format string literal with lowercase (here and
other spots).
> + == ((num_exiting_threads || num_remapped_threads)
> + ? (num_exiting_threads + num_remapped_threads
> + + 1)
> + : 0))
The formatting looks weird hare, ? should be below the second ( in ((
> gomp_release_held_threads (pool, team, thr->ts.team_id);
> - if (num_new_threads || num_exiting_threads
> + if (num_new_threads || num_exiting_threads || num_remapped_threads
> || !gomp_barrier_can_hold (pool->prev_barrier))
> gomp_simple_barrier_wait (&pool->threads_dock);
I know it is preexisting, but even this should be
if (num_new_threads
|| num_exiting_threads
|| num_remapped_threads
|| !gomp_barrier_can_hold (pool->prev_barrier))
so when you touch it, please fix that up.
Jakub
