This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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:

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?

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.
Index: libgcc/config/arm/linux-atomic.c
===================================================================
--- libgcc/config/arm/linux-atomic.c	(revision 196648)
+++ libgcc/config/arm/linux-atomic.c	(working copy)
@@ -97,19 +97,19 @@ FETCH_AND_OP_WORD (nand, ~, &)
     return (RETURN & mask) >> shift;					\
   }
 
-SUBWORD_SYNC_OP (add,   , +, unsigned short, 2, oldval)
-SUBWORD_SYNC_OP (sub,   , -, unsigned short, 2, oldval)
-SUBWORD_SYNC_OP (or,    , |, unsigned short, 2, oldval)
-SUBWORD_SYNC_OP (and,   , &, unsigned short, 2, oldval)
-SUBWORD_SYNC_OP (xor,   , ^, unsigned short, 2, oldval)
-SUBWORD_SYNC_OP (nand, ~, &, unsigned short, 2, oldval)
-
-SUBWORD_SYNC_OP (add,   , +, unsigned char, 1, oldval)
-SUBWORD_SYNC_OP (sub,   , -, unsigned char, 1, oldval)
-SUBWORD_SYNC_OP (or,    , |, unsigned char, 1, oldval)
-SUBWORD_SYNC_OP (and,   , &, unsigned char, 1, oldval)
-SUBWORD_SYNC_OP (xor,   , ^, unsigned char, 1, oldval)
-SUBWORD_SYNC_OP (nand, ~, &, unsigned char, 1, oldval)
+SUBWORD_SYNC_OP (add,   , +, short, 2, oldval)
+SUBWORD_SYNC_OP (sub,   , -, short, 2, oldval)
+SUBWORD_SYNC_OP (or,    , |, short, 2, oldval)
+SUBWORD_SYNC_OP (and,   , &, short, 2, oldval)
+SUBWORD_SYNC_OP (xor,   , ^, short, 2, oldval)
+SUBWORD_SYNC_OP (nand, ~, &, short, 2, oldval)
+
+SUBWORD_SYNC_OP (add,   , +, signed char, 1, oldval)
+SUBWORD_SYNC_OP (sub,   , -, signed char, 1, oldval)
+SUBWORD_SYNC_OP (or,    , |, signed char, 1, oldval)
+SUBWORD_SYNC_OP (and,   , &, signed char, 1, oldval)
+SUBWORD_SYNC_OP (xor,   , ^, signed char, 1, oldval)
+SUBWORD_SYNC_OP (nand, ~, &, signed char, 1, oldval)
 
 #define OP_AND_FETCH_WORD(OP, PFX_OP, INF_OP)				\
   int HIDDEN								\
@@ -132,19 +132,19 @@ OP_AND_FETCH_WORD (and,   , &)
 OP_AND_FETCH_WORD (xor,   , ^)
 OP_AND_FETCH_WORD (nand, ~, &)
 
-SUBWORD_SYNC_OP (add,   , +, unsigned short, 2, newval)
-SUBWORD_SYNC_OP (sub,   , -, unsigned short, 2, newval)
-SUBWORD_SYNC_OP (or,    , |, unsigned short, 2, newval)
-SUBWORD_SYNC_OP (and,   , &, unsigned short, 2, newval)
-SUBWORD_SYNC_OP (xor,   , ^, unsigned short, 2, newval)
-SUBWORD_SYNC_OP (nand, ~, &, unsigned short, 2, newval)
-
-SUBWORD_SYNC_OP (add,   , +, unsigned char, 1, newval)
-SUBWORD_SYNC_OP (sub,   , -, unsigned char, 1, newval)
-SUBWORD_SYNC_OP (or,    , |, unsigned char, 1, newval)
-SUBWORD_SYNC_OP (and,   , &, unsigned char, 1, newval)
-SUBWORD_SYNC_OP (xor,   , ^, unsigned char, 1, newval)
-SUBWORD_SYNC_OP (nand, ~, &, unsigned char, 1, newval)
+SUBWORD_SYNC_OP (add,   , +, short, 2, newval)
+SUBWORD_SYNC_OP (sub,   , -, short, 2, newval)
+SUBWORD_SYNC_OP (or,    , |, short, 2, newval)
+SUBWORD_SYNC_OP (and,   , &, short, 2, newval)
+SUBWORD_SYNC_OP (xor,   , ^, short, 2, newval)
+SUBWORD_SYNC_OP (nand, ~, &, short, 2, newval)
+
+SUBWORD_SYNC_OP (add,   , +, signed char, 1, newval)
+SUBWORD_SYNC_OP (sub,   , -, signed char, 1, newval)
+SUBWORD_SYNC_OP (or,    , |, signed char, 1, newval)
+SUBWORD_SYNC_OP (and,   , &, signed char, 1, newval)
+SUBWORD_SYNC_OP (xor,   , ^, signed char, 1, newval)
+SUBWORD_SYNC_OP (nand, ~, &, signed char, 1, newval)
 
 int HIDDEN
 __sync_val_compare_and_swap_4 (int *ptr, int oldval, int newval)
@@ -181,7 +181,7 @@ __sync_val_compare_and_swap_4 (int *ptr,
 	actual_oldval = *wordptr;					\
 									\
 	if (__builtin_expect (((actual_oldval & mask) >> shift) !=      \
-                              (unsigned int) oldval, 0))                \
+                              ((unsigned int) oldval & MASK_##WIDTH), 0)) \
           return (actual_oldval & mask) >> shift;			\
 									\
 	actual_newval = (actual_oldval & ~mask)				\
@@ -195,8 +195,8 @@ __sync_val_compare_and_swap_4 (int *ptr,
       }									\
   }
 
-SUBWORD_VAL_CAS (unsigned short, 2)
-SUBWORD_VAL_CAS (unsigned char,  1)
+SUBWORD_VAL_CAS (short,       2)
+SUBWORD_VAL_CAS (signed char, 1)
 
 typedef unsigned char bool;
 
@@ -217,8 +217,8 @@ __sync_bool_compare_and_swap_4 (int *ptr
     return (oldval == actual_oldval);					\
   }
 
-SUBWORD_BOOL_CAS (unsigned short, 2)
-SUBWORD_BOOL_CAS (unsigned char,  1)
+SUBWORD_BOOL_CAS (short,       2)
+SUBWORD_BOOL_CAS (signed char, 1)
 
 void HIDDEN
 __sync_synchronize (void)
@@ -260,8 +260,8 @@ __sync_lock_test_and_set_4 (int *ptr, in
     return (oldval & mask) >> shift;					\
   }
 
-SUBWORD_TEST_AND_SET (unsigned short, 2)
-SUBWORD_TEST_AND_SET (unsigned char,  1)
+SUBWORD_TEST_AND_SET (short,       2)
+SUBWORD_TEST_AND_SET (signed char, 1)
 
 #define SYNC_LOCK_RELEASE(TYPE, WIDTH)					\
   void HIDDEN								\

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]