Re: [PATCH] c-family: Use CAS loop instead of RMW atomics on small _BitInt with padding on targets which need to extend [PR124948]
Torbjorn SVENSSON <[email protected]> Mon, 3 Aug 2026 13:50:18 +0200
| Newsgroups | gmane.comp.gcc.patches |
|---|---|
| Message-ID | <[email protected]> |
Hi Jakub,
This change introduces new failures for arm-none-eabi using thumb/arch=armv6s-m/cpu=cortex-m0/float-abi=soft/fpu=auto.
Testing torture/bitint-100.c, -O0
doing compile
Executing on host: /build/install-native/bin/arm-none-eabi-gcc /build/gcc_src/gcc/testsuite/gcc.dg/torture/bitint-100.c -mthumb -march=armv6s-m -mcpu=cortex-m0 -mfloat-abi=soft -mfpu=auto -dumpbase "" -fdiagnostics-plain-output -O0 -std=c23 -pedantic-errors --specs=rdimon.specs -Wl,--start-group -lc -lm -Wl,--end-group --specs=nosys.specs -Wl,--allow-multiple-definition -Wl,-u,_isatty,-u,_fstat -Wl,-wrap,exit -Wl,-wrap,_exit -Wl,-wrap,main -Wl,-wrap,abort -Wl,gcc_tg.o -lm -T qemu.ld -o ./bitint-100.exe (timeout = 800)
spawn -ignore SIGHUP /build/install-native/bin/arm-none-eabi-gcc /build/gcc_src/gcc/testsuite/gcc.dg/torture/bitint-100.c -mthumb -march=armv6s-m -mcpu=cortex-m0 -mfloat-abi=soft -mfpu=auto -dumpbase -fdiagnostics-plain-output -O0 -std=c23 -pedantic-errors --specs=rdimon.specs -Wl,--start-group -lc -lm -Wl,--end-group --specs=nosys.specs -Wl,--allow-multiple-definition -Wl,-u,_isatty,-u,_fstat -Wl,-wrap,exit -Wl,-wrap,_exit -Wl,-wrap,main -Wl,-wrap,abort -Wl,gcc_tg.o -lm -T qemu.ld -o ./bitint-100.exe
pid is 1478576 -1478576
/build/install-native/arm-none-eabi/bin/ld: /tmp/ccgSaSgH.o: in function `f1':
bitint-100.c:(.text+0x5a): undefined reference to `__atomic_compare_exchange_4'
/build/install-native/arm-none-eabi/bin/ld: (__atomic_compare_exchange_4): Unknown destination type (ARM/Thumb) in /tmp/ccgSaSgH.o
bitint-100.c:(.text+0x5a): dangerous relocation: unsupported relocation
/build/install-native/arm-none-eabi/bin/ld: /tmp/ccgSaSgH.o: in function `f2':
bitint-100.c:(.text+0xa4): undefined reference to `__atomic_compare_exchange_4'
/build/install-native/arm-none-eabi/bin/ld: (__atomic_compare_exchange_4): Unknown destination type (ARM/Thumb) in /tmp/ccgSaSgH.o
bitint-100.c:(.text+0xa4): dangerous relocation: unsupported relocation
...
collect2: error: ld returned 1 exit status
status 1
compiler exited with status 1
FAIL: gcc.dg/torture/bitint-100.c -O0 (test for excess errors)
Same failure exist on both trunk and releases/gcc-16.
Kind regards,
Torbjörn
On 2026-07-09 10:00, Jakub Jelinek wrote:
> On Wed, Jul 08, 2026 at 11:54:31AM +0200, Jakub Jelinek wrote:
>> On Wed, Jul 08, 2026 at 10:25:30AM +0200, Jakub Jelinek wrote:
>>> I'm afraid this approach is incorrect.
>>>
>>> The problem is that in GIMPLE pointer casts are useless, so what exact
>>> pointee you get is quite random, and it can be completely unrelated type.
>>> If one e.g. does
>>> void
>>> foo (unsigned _BitInt(17) *p, bool x)
>>> {
>>> if (x)
>>> __atomic_add_fetch (p, 0x0fff0, 0);
>>> else
>>> __atomic_add_fetch ((unsigned long *) p, 0x0fff0, 0);
>>> }
>>> you can get unsigned _BitInt(17) "incorrectly" in both cases, while
>>> void
>>> bar (unsigned long *p, bool x)
>>> {
>>> if (x)
>>> __atomic_add_fetch ((unsigned _BitInt(17) *) p, 0x0fff0, 0);
>>> else
>>> __atomic_add_fetch (p, 0x0fff0, 0);
>>> }
>>> in neither, etc.
>>> IMNSHO, if unsigned _BitInt(17) * pointer makes it to say __atomic_add_fetch_4,
>>> then it should be irrelevant, the required operation is still to load
>>> full 32 bits from the pointer, add another 32 bit value to it and store
>>> full 32 bits.
>>>
>>> IMNSHO this is solely about some of the type-generic atomic/sync builtins
>>> (those documented to take TYPE * arguments) and needs to be handled in the
>>> FE, likely in gcc/c-family/c-common.cc (resolve_overloaded_builtin).
>>> There already is code to transform various type-generic builtins into a CAS
>>> loop for say unsigned _BitInt(253), so I think it should be used also for
>>> the case where the type is BITINT_TYPE with any padding bits in the
>>> extend other than bitint_ext_undef mode.
>>
>> Here is a completely untested patch for that, but guess the test
>> I've posted in the last mail needs to be turned into a proper executable
>> testcase and verify (e.g. using bitintext.h macros) whether the extension
>> was right.
>
> I've added a testcase and that revealed a bug in the patch, in the
> __sync_fetch_and_* cases the patch returned the new value anyway like
> in the __sync_*_and_fetch case.
>
> The following patch fixes that.
>
> So far tested with a cross to riscv64-linux and running the test on cfarm94.
>
> Ok for trunk if this passes bootstrap/regtest on x86_64-linux and
> i686-linux?
>
> 2026-07-09 Jakub Jelinek <[email protected]>
>
> PR target/124948
> * c-common.cc (sync_resolve_size): Return -1 for fetch ops
> on _BitInt types with padding bits where the target requires
> extension into the padding bits.
> (atomic_bitint_fetch_using_cas_loop): Handle also __sync_*
> fetch builtins.
>
> * gcc.dg/torture/bitint-100.c: New test.
>
> --- gcc/c-family/c-common.cc.jj 2026-07-08 11:09:58.407725160 +0200
> +++ gcc/c-family/c-common.cc 2026-07-09 09:48:53.446148261 +0200
> @@ -7746,7 +7746,23 @@ sync_resolve_size (tree function, vec<tr
> }
>
> if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16)
> - return size;
> + {
> + /* For _BitInt with padding bits where the ABI mandates sign or
> + zero extension into the padding bits, force a CAS loop so that
> + the extension is properly performed. */
> + if (fetch
> + && BITINT_TYPE_P (type)
> + && (TYPE_PRECISION (type)
> + != GET_MODE_PRECISION (SCALAR_TYPE_MODE (type))))
> + {
> + struct bitint_info info;
> + bool ok = targetm.c.bitint_type_info (TYPE_PRECISION (type), &info);
> + gcc_assert (ok);
> + if (info.extended != bitint_ext_undef)
> + return -1;
> + }
> + return size;
> + }
>
> if (fetch && !orig_format && BITINT_TYPE_P (type))
> return -1;
> @@ -8405,7 +8421,8 @@ resolve_overloaded_atomic_store (locatio
> }
>
> /* Emit __atomic*fetch* on _BitInt which doesn't have a size of
> - 1, 2, 4, 8 or 16 bytes using __atomic_compare_exchange loop.
> + 1, 2, 4, 8 or 16 bytes (or if it has padding bits and they
> + need to be extended) using __atomic_compare_exchange loop.
> ORIG_CODE is the DECL_FUNCTION_CODE of ORIG_FUNCTION and
> ORIG_PARAMS arguments of the call. */
>
> @@ -8417,6 +8434,7 @@ atomic_bitint_fetch_using_cas_loop (loca
> {
> enum tree_code code = ERROR_MARK;
> bool return_old_p = false;
> + bool sync_p = false;
> switch (orig_code)
> {
> case BUILT_IN_ATOMIC_ADD_FETCH_N:
> @@ -8459,13 +8477,65 @@ atomic_bitint_fetch_using_cas_loop (loca
> code = BIT_IOR_EXPR;
> return_old_p = true;
> break;
> + case BUILT_IN_SYNC_ADD_AND_FETCH_N:
> + code = PLUS_EXPR;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_SUB_AND_FETCH_N:
> + code = MINUS_EXPR;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_OR_AND_FETCH_N:
> + code = BIT_IOR_EXPR;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_AND_AND_FETCH_N:
> + code = BIT_AND_EXPR;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_XOR_AND_FETCH_N:
> + code = BIT_XOR_EXPR;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_NAND_AND_FETCH_N:
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_ADD_N:
> + code = PLUS_EXPR;
> + return_old_p = true;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_SUB_N:
> + code = MINUS_EXPR;
> + return_old_p = true;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_OR_N:
> + code = BIT_IOR_EXPR;
> + return_old_p = true;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_AND_N:
> + code = BIT_AND_EXPR;
> + return_old_p = true;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_XOR_N:
> + code = BIT_XOR_EXPR;
> + return_old_p = true;
> + sync_p = true;
> + break;
> + case BUILT_IN_SYNC_FETCH_AND_NAND_N:
> + return_old_p = true;
> + sync_p = true;
> + break;
> default:
> gcc_unreachable ();
> }
>
> - if (orig_params->length () != 3)
> + if (orig_params->length () != (sync_p ? 2 : 3))
> {
> - if (orig_params->length () < 3)
> + if (orig_params->length () < (sync_p ? 2 : 3))
> error_at (loc, "too few arguments to function %qE", orig_function);
> else
> error_at (loc, "too many arguments to function %qE", orig_function);
> @@ -8480,12 +8550,14 @@ atomic_bitint_fetch_using_cas_loop (loca
>
> tree lhs_addr = (*orig_params)[0];
> tree val = convert (nonatomic_lhs_type, (*orig_params)[1]);
> - tree model = convert (integer_type_node, (*orig_params)[2]);
> + tree model
> + = sync_p ? NULL_TREE : convert (integer_type_node, (*orig_params)[2]);
> if (!c_dialect_cxx ())
> {
> lhs_addr = c_fully_fold (lhs_addr, false, NULL);
> val = c_fully_fold (val, false, NULL);
> - model = c_fully_fold (model, false, NULL);
> + if (model)
> + model = c_fully_fold (model, false, NULL);
> }
> if (TREE_SIDE_EFFECTS (lhs_addr))
> {
> @@ -8501,7 +8573,7 @@ atomic_bitint_fetch_using_cas_loop (loca
> NULL_TREE);
> add_stmt (val);
> }
> - if (TREE_SIDE_EFFECTS (model))
> + if (model && TREE_SIDE_EFFECTS (model))
> {
> tree var = create_tmp_var_raw (integer_type_node);
> model = build4 (TARGET_EXPR, integer_type_node, var, model, NULL_TREE,
> @@ -8519,6 +8591,8 @@ atomic_bitint_fetch_using_cas_loop (loca
> TREE_ADDRESSABLE (newval) = 1;
> suppress_warning (newval);
>
> + tree retval = NULL_TREE;
> +
> tree loop_decl = create_artificial_label (loc);
> tree loop_label = build1 (LABEL_EXPR, void_type_node, loop_decl);
>
> @@ -8582,21 +8656,43 @@ atomic_bitint_fetch_using_cas_loop (loca
>
> /* if (__atomic_compare_exchange (addr, &old, &new, false, model, model))
> goto done; */
> - fndecl = builtin_decl_explicit (BUILT_IN_ATOMIC_COMPARE_EXCHANGE);
> + if (sync_p)
> + fndecl = builtin_decl_explicit (BUILT_IN_SYNC_VAL_COMPARE_AND_SWAP_N);
> + else
> + fndecl = builtin_decl_explicit (BUILT_IN_ATOMIC_COMPARE_EXCHANGE);
> params->quick_push (lhs_addr);
> - params->quick_push (old_addr);
> - params->quick_push (newval_addr);
> - params->quick_push (integer_zero_node);
> - params->quick_push (model);
> - if (tree_fits_uhwi_p (model)
> - && (tree_to_uhwi (model) == MEMMODEL_RELEASE
> - || tree_to_uhwi (model) == MEMMODEL_ACQ_REL))
> - params->quick_push (build_int_cst (integer_type_node, MEMMODEL_RELAXED));
> + if (sync_p)
> + {
> + params->quick_push (old);
> + params->quick_push (newval);
> + }
> else
> - params->quick_push (model);
> + {
> + params->quick_push (old_addr);
> + params->quick_push (newval_addr);
> + params->quick_push (integer_zero_node);
> + params->quick_push (model);
> + if (tree_fits_uhwi_p (model)
> + && (tree_to_uhwi (model) == MEMMODEL_RELEASE
> + || tree_to_uhwi (model) == MEMMODEL_ACQ_REL))
> + params->quick_push (build_int_cst (integer_type_node,
> + MEMMODEL_RELAXED));
> + else
> + params->quick_push (model);
> + }
> func_call = resolve_overloaded_builtin (loc, fndecl, params);
> if (func_call == NULL_TREE)
> func_call = build_function_call_vec (loc, vNULL, fndecl, params, NULL);
> + if (sync_p)
> + {
> + if (return_old_p)
> + retval = create_tmp_var_raw (nonatomic_lhs_type);
> + else
> + retval = old;
> + func_call = build2 (MODIFY_EXPR, void_type_node, retval, func_call);
> + tree cmp = build2 (EQ_EXPR, boolean_type_node, retval, newval);
> + func_call = build2 (COMPOUND_EXPR, boolean_type_node, func_call, cmp);
> + }
>
> tree goto_stmt = build1 (GOTO_EXPR, void_type_node, done_decl);
> SET_EXPR_LOCATION (goto_stmt, loc);
> @@ -8606,6 +8702,12 @@ atomic_bitint_fetch_using_cas_loop (loca
> SET_EXPR_LOCATION (stmt, loc);
> add_stmt (stmt);
>
> + if (sync_p && return_old_p)
> + {
> + stmt = build2_loc (loc, MODIFY_EXPR, void_type_node, old, retval);
> + add_stmt (stmt);
> + }
> +
> /* goto loop; */
> goto_stmt = build1 (GOTO_EXPR, void_type_node, loop_decl);
> SET_EXPR_LOCATION (goto_stmt, loc);
> --- gcc/testsuite/gcc.dg/torture/bitint-100.c.jj 2026-07-09 08:25:06.764615958 +0200
> +++ gcc/testsuite/gcc.dg/torture/bitint-100.c 2026-07-09 09:19:20.063962240 +0200
> @@ -0,0 +1,197 @@
> +/* PR target/124948 */
> +/* { dg-do run { target bitint } } */
> +/* { dg-options "-std=c23 -pedantic-errors" } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "*" } { "-O0" "-O2" } } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "-flto" } { "" } } */
> +
> +_BitInt(17) a;
> +unsigned _BitInt(17) b;
> +
> +#include "../bitintext.h"
> +
> +[[gnu::noipa]] _BitInt(17)
> +f1 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __atomic_add_fetch (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f2 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __atomic_sub_fetch (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f3 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __atomic_fetch_add (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f4 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __atomic_fetch_sub (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f5 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __sync_add_and_fetch (p, q);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f6 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __sync_sub_and_fetch (p, q);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f7 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __sync_fetch_and_add (p, q);
> +}
> +
> +[[gnu::noipa]] _BitInt(17)
> +f8 (_BitInt(17) *p, _BitInt(32) q)
> +{
> + return __sync_fetch_and_sub (p, q);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f9 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __atomic_add_fetch (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f10 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __atomic_sub_fetch (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f11 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __atomic_fetch_add (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f12 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __atomic_fetch_sub (p, q, __ATOMIC_RELAXED);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f13 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __sync_add_and_fetch (p, q);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f14 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __sync_sub_and_fetch (p, q);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f15 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __sync_fetch_and_add (p, q);
> +}
> +
> +[[gnu::noipa]] unsigned _BitInt(17)
> +f16 (unsigned _BitInt(17) *p, unsigned _BitInt(32) q)
> +{
> + return __sync_fetch_and_sub (p, q);
> +}
> +
> +int
> +main ()
> +{
> + _BitInt(17) c;
> + unsigned _BitInt(17) d;
> + a = 64295wb;
> + BEXTC (a);
> + c = f1 (&a, 187040987wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != -65534wb || c != a)
> + __builtin_abort ();
> + c = f2 (&a, 394264674wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != 65440wb || c != a)
> + __builtin_abort ();
> + c = f3 (&a, 840434595wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != -64701wb || c != 65440wb)
> + __builtin_abort ();
> + c = f4 (&a, 591403122wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != 60113wb || c != -64701wb)
> + __builtin_abort ();
> + c = f5 (&a, 1215571163wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != -61524wb || c != a)
> + __builtin_abort ();
> + c = f6 (&a, 1913664021wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != 56727wb || c != a)
> + __builtin_abort ();
> + c = f7 (&a, 858931586wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != -57575wb || c != 56727wb)
> + __builtin_abort ();
> + c = f8 (&a, 286413601wb);
> + BEXTC (a);
> + BEXTC (c);
> + if (a != 52216wb || c != -57575wb)
> + __builtin_abort ();
> + b = 77593uwb;
> + BEXTC (b);
> + d = f9 (&b, 858861337uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 24114uwb || d != b)
> + __builtin_abort ();
> + d = f10 (&b, 1431383831uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 77595uwb || d != b)
> + __builtin_abort ();
> + d = f11 (&b, 305231735uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 42642uwb || d != 77595uwb)
> + __builtin_abort ();
> + d = f12 (&b, 591497352uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 73226uwb || d != 42642uwb)
> + __builtin_abort ();
> + d = f13 (&b, 1985058934uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 46720uwb || d != b)
> + __builtin_abort ();
> + d = f14 (&b, 840018893uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 68275uwb || d != b)
> + __builtin_abort ();
> + d = f15 (&b, 1698807006uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 51089uwb || d != 68275uwb)
> + __builtin_abort ();
> + d = f16 (&b, 877788892uwb);
> + BEXTC (b);
> + BEXTC (d);
> + if (b != 51381uwb || d != 51089uwb)
> + __builtin_abort ();
> +}
>
>
> Jakub
>