Power/GCC: Subword atomic operation endianness check bug fix
diff mbox

Message ID alpine.DEB.1.10.1407021047000.16254@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki July 2, 2014, 10:05 a.m. UTC
Hi,

 This change:

------------------------------------------------------------------------
r199935 | amodra | 2013-06-11 07:17:50 +0100 (Tue, 11 Jun 2013) | 4 lines

	* config/rs6000/rs6000.c (rs6000_adjust_atomic_subword): Calculate
	correct shift value in little-endian mode.

------------------------------------------------------------------------

fixed subword atomic operations on little-endian Power targets, but while 
the effect of the change happens to be correct, the way it is achieved is 
not.

 This code operates on SImode values, however refers to WORDS_BIG_ENDIAN 
to check the endianness.  This is not right, quoting our internal manual:

"[...] The memory order of bytes is defined by two
target macros, `WORDS_BIG_ENDIAN' and `BYTES_BIG_ENDIAN':

* `WORDS_BIG_ENDIAN', if set to 1, says that byte number
  zero is part of the most significant word; otherwise, it
  is part of the least significant word.

* `BYTES_BIG_ENDIAN', if set to 1, says that byte number
  zero is the most significant byte within a word;
  otherwise, it is the least significant byte within a
  word."

so WORDS_BIG_ENDIAN should only ever be referred when operating on 
multi-word values, not single-word ones such as SImode ones are.  As it 
happens both macros have the same value for the Power target, so the 
result of the check works out the same, but nevertheless it's not correct 
and can be misleading to the casual reader.

 The fix below has been regression tested with the powerpc-eabi target and 
the following multilibs:

-mcpu=603e
-mcpu=603e -msoft-float
-mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe
-mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe -msoft-float
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe -mlittle
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe -msoft-float
-mcpu=7400 -maltivec -mabi=altivec

as well as the powerpc-linux-gnu target and the following multilibs:

-mcpu=603e
-mcpu=603e -msoft-float
-mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe
-mcpu=7400 -maltivec -mabi=altivec
-mcpu=e5500 -m64

 OK to apply?

2014-07-02  Maciej W. Rozycki  <macro@codesourcery.com>

	gcc/
	* config/rs6000/rs6000.c (rs6000_adjust_atomic_subword): Use
	BYTES_BIG_ENDIAN rather than WORDS_BIG_ENDIAN to check for byte 
	endianness.

  Maciej

gcc-ppc-atomic-le.diff

Comments

Alan Modra July 2, 2014, 12:34 p.m. UTC | #1
On Wed, Jul 02, 2014 at 11:05:23AM +0100, Maciej W. Rozycki wrote:
> As it 
> happens both macros have the same value for the Power target

Thanks!  Looks good to me.  As you note above it is effectively a
documentation fix.

> --- gcc-fsf-trunk-quilt.orig/gcc/config/rs6000/rs6000.c	2014-06-10 21:46:36.628975329 +0100
> +++ gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.c	2014-06-11 16:35:08.917560846 +0100
> @@ -19891,7 +19891,7 @@ rs6000_adjust_atomic_subword (rtx orig_m
>    shift = gen_reg_rtx (SImode);
>    addr = gen_lowpart (SImode, addr);
>    emit_insn (gen_rlwinm (shift, addr, GEN_INT (3), GEN_INT (shift_mask)));
> -  if (WORDS_BIG_ENDIAN)
> +  if (BYTES_BIG_ENDIAN)
>      shift = expand_simple_binop (SImode, XOR, shift, GEN_INT (shift_mask),
>  			         shift, 1, OPTAB_LIB_WIDEN);
>    *pshift = shift;

Patch
diff mbox

Index: gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.c
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/config/rs6000/rs6000.c	2014-06-10 21:46:36.628975329 +0100
+++ gcc-fsf-trunk-quilt/gcc/config/rs6000/rs6000.c	2014-06-11 16:35:08.917560846 +0100
@@ -19891,7 +19891,7 @@  rs6000_adjust_atomic_subword (rtx orig_m
   shift = gen_reg_rtx (SImode);
   addr = gen_lowpart (SImode, addr);
   emit_insn (gen_rlwinm (shift, addr, GEN_INT (3), GEN_INT (shift_mask)));
-  if (WORDS_BIG_ENDIAN)
+  if (BYTES_BIG_ENDIAN)
     shift = expand_simple_binop (SImode, XOR, shift, GEN_INT (shift_mask),
 			         shift, 1, OPTAB_LIB_WIDEN);
   *pshift = shift;