On Thu, May 28, 2026 at 09:12:40AM +0100, Matthew Malcomson wrote:
> --- a/libgomp/team.c
> +++ b/libgomp/team.c
> @@ -158,6 +158,23 @@ gomp_thread_start (void *xdata)
> if (!gomp_barrier_can_hold (&team->barrier) || thr->fn == NULL)
> gomp_simple_barrier_wait (&pool->threads_dock);
>
> + /* Early exit because if `thr->fn == NULL` then we can't guarantee
> + the `thr->ts.team` hasn't been freed by the primary thread racing
> + ahead. Hence we don't want to write to it. */
> + if (thr->fn == NULL)
> + break;
If you do this, then
> + team = thr->ts.team;
> + thr->ts.work_share = &team->work_shares[0];
> + thr->ts.last_work_share = NULL;
> +#ifdef HAVE_SYNC_BUILTINS
> + thr->ts.single_count = 0;
> +#endif
> + thr->ts.static_trip = 0;
> + thr->task = &team->implicit_task[thr->ts.team_id];
> + gomp_init_task (thr->task, team->task_parent_init,
> + &team->task_icv_init);
> + thr->task->taskgroup = team->task_taskgroup;
> +
> local_fn = thr->fn;
> local_data = thr->data;
> thr->fn = NULL;
}
while (local_fn);
after this is pointless, should be then
}
while (1);
> @@ -437,6 +454,15 @@ gomp_team_start (void (*fn) (void *), void *data,
> unsigned nthreads,
> thr->task->taskgroup = taskgroup;
> team->implicit_task[0].icv.nthreads_var = nthreads_var;
> team->implicit_task[0].icv.bind_var = bind_var;
> + /* Copy entirely rather than copy pointer so that we have an immutable copy
> + for the secondary threads to take from for the time-frame that said
> + secondary threads are executing on. Can't copy from
> + `team->implicit_task[0].icv` because primary thread could have adjusted
> + its per-task ICV by the time the secondary thread gets around to
> + initialising things. */
> + team->task_icv_init = team->implicit_task[0].icv;
> + team->task_parent_init = task;
> + team->task_taskgroup = taskgroup;
This is really costly, the first new stmt alone is copying 40 bytes on
64-bit arches.
> if (nthreads == 1)
> return;
In the nthreads == 1 case it is definitely a waste of time, so at least
it should be moved after this.
Now, you only make use of this in it in the
if (!nested)
...
/* Release existing idle threads. */
for (; i < n; ++i)
case. So, for that case I wonder if the above 3 statements shouldn't be
done also only in the if (!nested) case, and perhaps even guarded with
if (n > 1).
Another possibility is to do it also for the newly launched threads
(including the nested case which currently doesn't use thread pool (maybe we
should reconsider that at some point)).
So, that is trying to get rid of
start_data->ts.work_share = &team->work_shares[0];
start_data->ts.last_work_share = NULL;
and
#ifdef HAVE_SYNC_BUILTINS
start_data->ts.single_count = 0;
#endif
start_data->ts.static_trip = 0;
and
start_data->task = &team->implicit_task[i];
gomp_init_task (start_data->task, task, icv);
team->implicit_task[i].icv.nthreads_var = nthreads_var;
team->implicit_task[i].icv.bind_var = bind_var;
start_data->task->taskgroup = taskgroup;
Of course, in that case the above 3 stmts would need to be done
unconditionally after the if (nthreads == 1) return; early out and
in gomp_thread_start would need to be done at a different point,
before local_fn (local_data); call (in both data->nested and !data->nested
cases).
> @@ -725,26 +751,15 @@ gomp_team_start (void (*fn) (void *), void *data,
> unsigned nthreads,
> else
> nthr = pool->threads[i];
> __atomic_store_n (&nthr->ts.team, team, MEMMODEL_RELEASE);
> - nthr->ts.work_share = &team->work_shares[0];
> - nthr->ts.last_work_share = NULL;
> nthr->ts.team_id = i;
> nthr->ts.level = team->prev_ts.level + 1;
> nthr->ts.active_level = thr->ts.active_level;
> nthr->ts.place_partition_off = place_partition_off;
> nthr->ts.place_partition_len = place_partition_len;
> nthr->ts.def_allocator = thr->ts.def_allocator;
> -#ifdef HAVE_SYNC_BUILTINS
> - nthr->ts.single_count = 0;
> -#endif
> - nthr->ts.static_trip = 0;
> nthr->num_teams = thr->num_teams;
> nthr->team_num = thr->team_num;
> - nthr->task = &team->implicit_task[i];
> nthr->place = place;
> - gomp_init_task (nthr->task, task, icv);
> - team->implicit_task[i].icv.nthreads_var = nthreads_var;
> - team->implicit_task[i].icv.bind_var = bind_var;
> - nthr->task->taskgroup = taskgroup;
> nthr->fn = fn;
> nthr->data = data;
> team->ordered_release[i] = &nthr->release;
> --
> 2.43.0
Jakub
