Re: [PATCH, ARM] ARM Linux kernel-assisted atomic operation helpers vs. libcall argument promotion

[Ramana Radhakrishnan <ramrad01 at arm dot com>]

On 03/15/13 18:16, Julian Brown wrote:
> Hi,
>
> At present, the libcall helpers implementing atomic operations
> (__sync_val_compare_and_swap_X) for char and short types suffer from
> a type mismatch. This is leading to test failures, i.e.:
>
> FAIL: gcc.dg/atomic-compare-exchange-1.c execution test
> FAIL: gcc.dg/atomic-compare-exchange-2.c execution test
>
> On investigation, these tests pass if the values used in the tests are
> tweaked so that they are in the range representable by both signed and
> unsigned chars, i.e. 0 to 127, rather than ~0. The failures are
> happening because libcall expansion is sign-extending sub-word-size
> arguments (e.g. EXPECTED, DESIRED in
> optabs.c:expand_atomic_compare_and_swap), but the functions
> implementing the operations are written to take unsigned arguments,
> zero-extended, and the unexpected out-of-range values cause them to
> fail.
>
> The sign-extension happens because in calls.c:emit_library_call_value_1
> we have:
>
>     mode = promote_function_mode (NULL_TREE, mode, &unsigned_p, NULL_TREE, 0);
>     argvec[count].mode = mode;
>     argvec[count].value = convert_modes (mode, GET_MODE (val), val, unsigned_p);
>     argvec[count].reg = targetm.calls.function_arg (args_so_far, mode,
>                                                     NULL_TREE, true);
>
> This calls back into arm.c:arm_promote_function_mode, which promotes
> less-than-four-byte integral values to SImode, but never modifies the
> PUNSIGNEDP argument. So, such values always get sign extended when being
> passed to libcalls.
>
> The simplest fix for this (since libcalls don't have proper tree types
> to inspect for the actual argument types) is just to define the
> linux-atomic.c functions to use signed char/short instead of unsigned
> char/unsigned short, approximately reversing the change in this earlier
> patch:

It is unfortunate that we need to reverse this change given that we'd 
like things to be as unsigned as possible but I don't see how this would 
work otherwise right now.

Changing to an unsigned interface everywhere appears to cause more 
issues than is worth fixing right now given that it possibly makes life 
more difficult in the fixed point arithmetic function.

Thanks for the clarification on irc.

> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00492.html
>
> A slight change is also required to the
> __sync_val_compare_and_swap_* implementation in order to treat the
> signed OLDVAL argument correctly (I believe the other macros are OK).
>
> Tested cross to ARM Linux, default & thumb multilibs. The
> above-mentioned tests change from FAIL to PASS. OK to apply?


Ok and this should go to all afflicted release branches modulo their 
locked(ness) and watching trunk for a day or so ( keeping an eye on 
gcc-testresults for armv5te would be good enough.)

regards
Ramana

> Thanks,
>
> Julian
>
> ChangeLog
>
>      libgcc/
>      * config/arm/linux-atomic.c (SUBWORD_SYNC_OP, SUBWORD_VAL_CAS)
>      (SUBWORD_TEST_AND_SET): Use signed char/short types instead of
>      unsigned char/unsigned short.
>      (__sync_val_compare_and_swap_{1,2}): Handle signed argument.