On Wed, May 27, 2026 at 05:14:56PM +0100, Matthew Malcomson wrote:
> The function `gomp_barrier_state_is_incremented` is there in order to
> handle edge cases for when a generation gets incremented. It handles
> flags that might be on the generation number by checking `>` instead of
> `==` and it handles wraparound on the generation number.
>
> Unfortunately it had a problem where it did not handle the combination
> of flags on the state provided and wraparound on the generation number.
> I.e. if there were flags on the `state` input and the next generation
> would be *smaller* than the current generation due to unsigned integral
> overflow and hence wraparound then it could report that the generation
> has been incremented when it has not.
>
> This patch fixes that problem.
>
> I found a testcase to take too long for it to be sensible to have in the
> testsuite. My impression is that it's so long that it doesn't even make
> sense to run it when the `run_expensive_tests` effective target is set
> but would appreciate confirmation or refutation of that.
>
> This is because it takes a long time to go through a barrier enough
> times to trigger wraparound. My manual testing on a shared machine
> showed ~4 min when on the linux/ target and drastically longer built
> without futex support to test the posix/ target.
>
> Testing done as part of my patch-series:
> - Bootstrap & regtest on aarch64 and x86_64.
> - Testsuite with & without OMP_WAIT_POLICY=passive
> - With and without configure `--enable-linux-futex=no` for posix
> target.
> - Cross compilation & regtest on arm.
> - TSAN done on before and after total patch-series.
>
> libgomp/ChangeLog:
>
> * config/gcn/bar.h (gomp_barrier_state_is_incremented): Mask
> flags on previously observed state before comparison against
> current state.
8 spaces instead of tab.
> * config/linux/bar.h (gomp_barrier_state_is_incremented):
> Likewise
Missing dot (4 times).
> * config/nvptx/bar.h (gomp_barrier_state_is_incremented):
> Likewise
> * config/posix/bar.h (gomp_barrier_state_is_incremented):
> Likewise
> * config/rtems/bar.h (gomp_barrier_state_is_incremented):
> Likewise
> --- a/libgomp/config/linux/bar.h
> +++ b/libgomp/config/linux/bar.h
> @@ -172,7 +172,8 @@ static inline bool
> gomp_barrier_state_is_incremented (gomp_barrier_state_t gen,
> gomp_barrier_state_t state)
> {
> - unsigned next_state = (state & -BAR_INCR) + BAR_INCR;
> + state &= -BAR_INCR;
> + unsigned next_state = state + BAR_INCR;
> return next_state > state ? gen >= next_state : gen < state;
> }
This incremental change looks reasonable, and while at it
I think it would be useful to use __builtin_expect (next_state > state, 1)
instead of next_state > state because the next_state > state case is
more likely (99.9999998%).
But I wonder if this isn't unnecessarily complex.
The barrier generation is always incremented by BAR_INCR at a time,
we should be never comparing generation with state more than one BAR_INCR
apart (ignoring the low bits for now).
gomp_barrier_state_is_incremented is used in 3 spots:
the gomp_team_barrier_wait_end / gomp_team_barrier_wait_cancel_end
- while (gen != state + BAR_INCR);
+ while (!gomp_barrier_state_is_incremented (gen, state));
changes and in the newly added gomp_barrier_has_completed where state
comes from whatever gomp_barrier_handle_tasks has been called with.
For the two bar.c spots, I wonder if
while ((gen & -BAR_INCR) != state + BAR_INCR);
wouldn't be better than this.
We know gomp_barrier_wait_start clears the BAR_WAITING_FOR_TASK bit,
BAR_CANCELLED can be left set and the least significant bit is cleared
but maybe BAR_WAS_LAST is set.
At the
while (!gomp_barrier_state_is_incremented (gen, state));
line state doesn't have BAR_CANCELLED bit set:
state &= ~BAR_CANCELLED;
and neither BAR_WAS_LAST:
state &= ~BAR_WAS_LAST;
Now, in the remaining case I think some bits might be set, so
I think that one could be simplified into
(gen & -BAR_INCR) == (state & -BAR_INCR) + BAR_INCR;
or so.
Jakub
