On 7/21/26 10:02, Jakub Jelinek wrote:
External email: Use caution opening links or attachments
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
Yes those alternatives could be used.
I personally like the helper -- in my other upstream patches I introduce
extra complexity around which flags need to be checked and things got a
bit confusing. (Acknowledge that future patches aren't a great reason
to have this in the current code).
Would you be OK with keeping the helper but having an equality
comparison in that helper instead of the ternary?
