On Sat, 12 Sep 2026, Jakub Jelinek wrote: > Hi! > > If the val operand due to TER is a MEM (or contains a MEM) which can alias > with mem (the memory atomically updated), we have various wrong-code > problems in the fallback expansions. > The first one can be seen on the testcase and is solved by the second/third > hunks, if the backend has an instruction to atomically update in the mode > and return the previous rather than the updated value, we perform non-atomic > update of that value with val again after the atomic insn. If val is or > overlaps with mem, we then incorrectly use an updated value rather than > the original value (of course, the code is weird because it non-atomically > reads first what is then atomically updated, so if some other thread > modifies it, what will be the result is pretty random). This patch changes > - movl v(%rip), %eax > + movl v(%rip), %edx > + movl %edx, %eax > lock xaddl %eax, v(%rip) > - addl v(%rip), %eax > + addl %edx, %eax > cmpl $18, %eax > jne .L3 > xorl %eax, %eax > ret > on x86_64 by forcing the val into pseudo in that case. Another problem > (the last 2 hunks in optabs.cc) is if there is no named pattern for it > and we fallback to compare and exchange loop. E.g. if similar testcase > uses unsigned long long instead of unsigned on x86_64 -m32 -march=x86-64. > Not included in testsuite because it actually "works" on uncontended > variable, the bug is that val is read from memory twice, once before > the loop and once inside of the loop before the cmpxchg8b instruction. > So it is actually correct unless cmpxchg8b fails the first time. > And the first hunk is to do the same thing on various other places that > use cmpxchg fallback loop. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK. Thanks, Richard. > 2026-09-12 Jakub Jelinek <[email protected]> > > PR middle-end/127336 > * optabs.cc (maybe_emit_compare_and_swap_exchange_loop, > expand_atomic_fetch_op_no_fallback, expand_atomic_fetch_op): Force > val into a pseduo if it updates to mem could change its value > and when using val after the atomic op or inside of a loop. > > * gcc.dg/pr127336.c: New test. > > --- a/gcc/optabs.cc 2026-09-08 12:52:03.000000000 +0200 > +++ b/gcc/optabs.cc 2026-09-11 18:45:50.758771839 +0200 > @@ -7354,10 +7354,17 @@ maybe_emit_compare_and_swap_exchange_loo > > if (can_compare_and_swap_p (mode, true)) > { > + rtx_insn *start = get_last_insn (); > + /* Force val into a register if it could change value when the > + atomic insn updates mem. */ > + if (reg_overlap_mentioned_p (mem, val)) > + val = force_reg (mode, val); > if (!target || !register_operand (target, mode)) > target = gen_reg_rtx (mode); > if (expand_compare_and_swap_loop (mem, target, val, NULL_RTX)) > return target; > + else > + delete_insns_since (start); > } > > return NULL_RTX; > @@ -8153,6 +8160,12 @@ expand_atomic_fetch_op_no_fallback (rtx > try that operation. */ > if (after || unused_result || optab.reverse_code != UNKNOWN) > { > + rtx_insn *start = get_last_insn (); > + /* Force val into a register if it could change value when the > + atomic insn updates mem. */ > + if (!unused_result && reg_overlap_mentioned_p (mem, val)) > + val = force_reg (mode, val); > + > /* Try the __atomic version, then the older __sync version. */ > result = maybe_emit_op (&optab, target, mem, val, true, model, !after); > if (!result) > @@ -8179,6 +8192,8 @@ expand_atomic_fetch_op_no_fallback (rtx > true, OPTAB_LIB_WIDEN); > return result; > } > + else > + delete_insns_since (start); > } > > /* No direct opcode can be generated. */ > @@ -8278,6 +8293,11 @@ expand_atomic_fetch_op (rtx target, rtx > { > rtx_insn *insn; > rtx t0 = gen_reg_rtx (mode), t1; > + rtx_insn *start = get_last_insn (); > + /* Force val into a register if it could change value when the > + atomic insn updates mem. */ > + if (reg_overlap_mentioned_p (mem, val)) > + val = force_reg (mode, val); > > start_sequence (); > > @@ -8311,6 +8331,8 @@ expand_atomic_fetch_op (rtx target, rtx > > if (t1 != NULL && expand_compare_and_swap_loop (mem, t0, t1, insn)) > return target; > + else > + delete_insns_since (start); > } > > return NULL_RTX; > --- a/gcc/testsuite/gcc.dg/pr127336.c 2026-09-11 18:54:50.342667339 +0200 > +++ b/gcc/testsuite/gcc.dg/pr127336.c 2026-09-11 18:54:39.062815861 +0200 > @@ -0,0 +1,12 @@ > +/* PR middle-end/127336 */ > +/* { dg-do run { target sync_int_long } } */ > +/* { dg-options "-O2" } */ > + > +unsigned v = 9; > + > +int > +main () > +{ > + if (__atomic_add_fetch (&v, v, __ATOMIC_RELAXED) != 18) > + __builtin_abort (); > +} > > Jakub > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri; (HRB 36809, AG Nuernberg)
