On Thu, Jul 23, 2026 at 03:18:49PM +0100, Matthew Malcomson wrote:
> > 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?
I'm ok with the helper, but would prefer if the two uses would not use
that helper, i.e. b
while ((gen & -BAR_INCR) != state + BAR_INCR);
because I'm afraid the compiler can't figure out that state at that point
is equal to (state & -BAR_INCR) and so the masking is not needed there.
Or use the helper after
/* At this point the low bits of state are known to be unset. */
if (state & -BAR_INCR)
__builtin_unreachable ();
if it allows the masking to be optimized away.
Jakub
