diff mbox

RTABI half-precision conversion functions (ping)

Message ID 20120719132816.2b51c0bc@octopus
State New
Headers show

Commit Message

Julian Brown July 19, 2012, 12:28 p.m. UTC 
Hi,

The patch that Andrew Stubbs sent upstream here:

http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02130.html

seems to have become stalled after Ramana's question here:

http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00291.html

This was discussed with ARM personnel (i.e. Lee Smith) at the time the
patch was created. AFAICT, our reasoning went like this (Paul, please
correct me if I'm just making stuff up): we wish to use aliases for the
old and new names for the helpers. The existing
__gnu_{f2h,h2f}_{ieee,alternative} helpers take unsigned short
arguments, and therefore assume (on function entry) that the values
contained in those arguments are zero-extended (although, due to the
vagaries of libcall handling in GCC, that may not actually be true --
at the time, this was a latent bug, and may in fact still be). The
__aeabi_* helpers are defined to pass/return unadorned "short" values,
intending to convey that only the low-order 16 bits of half-float
values contain meaningful data.

But, that means EABI-conformant callers are also perfectly entitled to
sign-extend half-float values before calling our helper functions
(although GCC itself won't do that). Using "unsigned int" and taking
care to only examine the low-order bits of the value in the helper
function itself serves to fix the latent bug, is compatible with
existing code, allows us to be conformant with the eabi, and allows use
of aliases to make the __gnu and __aeabi functions the same.

The patch no longer applied as-is, so I've updated it (attached,
re-tested). Note that there are no longer any target-independent changes
(though I'm not certain that the symbol versions are still correct).

OK to apply?

Julian

ChangeLog

    Andrew Stubbs  <ams@codesourcery.com>
    Julian Brown  <julian@codesourcery.com>

    gcc/
    * config/arm/arm.c (arm_init_libfuncs): Change __gnu_f2h_ieee to
    __aeabi_f2h, __gnu_f2h_alternative to __aeabi_f2h_alt,
    __gnu_h2f_ieee to __aeabi_h2f, and __gnu_h2f_alternative to
    __aeabi_h2f_alt.

    libgcc/
    * config/arm/fp16.c (__gnu_f2h_internal): Change return type to
    unsigned int. Change 'sign' variable likewise.
    (__gnu_h2f_internal): Set to static inline.
    Change return type to unsigned int. Change 'sign' variable
    likewise. (ALIAS): New define.
    (__gnu_f2h_ieee): Change unsigned short to unsigned int.
    (__gnu_h2f_ieee): Likewise.
    (__gnu_f2h_alternative): Likewise.
    (__gnu_h2f_alternative): Likewise.
    (__aeabi_f2h, __aeabi_h2f): New aliases.
    (__aeabi_f2h_alt, __aeabi_h2f_alt): Likewise.
    * config/arm/libgcc-bpabi.ver (__aeabi_f2h, __aeabi_f2h_alt)
    (__aeabi_h2f, __aeabi_h2f_alt): Set versions.
    * config/arm/sfp-machine.h (__extendhfsf2): Set to __aeabi_h2f.
    (__truncsfhf2): Set to __aeabi_f2h.
    * config/arm/t-bpabi (LIB2ADD_ST): Move fp16.c ...
    (LIB2ADD): ... to here.
    * config/arm/t-symbian (LIB2ADD_ST): Move fp16.c ...
    (LIB2ADD): ... to here.

    gcc/testsuite/
    * g++.dg/ext/arm-fp16/arm-fp16-ops-5.C: Check for __aeabi_h2f
    and __aeabi_f2h.
    * g++.dg/ext/arm-fp16/arm-fp16-ops-6.C: Likewise.
    * gcc.dg/torture/arm-fp16-ops-5.c: Likewise.
    * gcc.dg/torture/arm-fp16-ops-6.c: Likewise.

Comments

Paul Brook July 19, 2012, 12:54 p.m. UTC | #1 
> Hi,
> 
> The patch that Andrew Stubbs sent upstream here:
> 
> http://gcc.gnu.org/ml/gcc-patches/2011-04/msg02130.html
> 
> seems to have become stalled after Ramana's question here:
> 
> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00291.html
> 
> This was discussed with ARM personnel (i.e. Lee Smith) at the time the
> patch was created. AFAICT, our reasoning went like this (Paul, please
> correct me if I'm just making stuff up): we wish to use aliases for the
> old and new names for the helpers. The existing
> __gnu_{f2h,h2f}_{ieee,alternative} helpers take unsigned short
> arguments, and therefore assume (on function entry) that the values
> contained in those arguments are zero-extended (although, due to the
> vagaries of libcall handling in GCC, that may not actually be true --
> at the time, this was a latent bug, and may in fact still be). The
> __aeabi_* helpers are defined to pass/return unadorned "short" values,
> intending to convey that only the low-order 16 bits of half-float
> values contain meaningful data.

Yes, that matches my understanding.

> But, that means EABI-conformant callers are also perfectly entitled to
> sign-extend half-float values before calling our helper functions
> (although GCC itself won't do that). Using "unsigned int" and taking
> care to only examine the low-order bits of the value in the helper
> function itself serves to fix the latent bug, is compatible with
> existing code, allows us to be conformant with the eabi, and allows use
> of aliases to make the __gnu and __aeabi functions the same.

As long as LTO never sees this mismatch we should be fine :-)  AFAIK we don't 
curently have any way of expressing the actual ABI.
 
> The patch no longer applied as-is, so I've updated it (attached,
> re-tested). Note that there are no longer any target-independent changes
> (though I'm not certain that the symbol versions are still correct).
> 
> OK to apply?

I think this deserves a comment in the source.  Otherwise it's liable to get 
"fixed" in the future :-) Something allong the lines of 
"While the EABI describes the arguments to the half-float helper routines as 
'short', it does not require that they be extended to full register width.  
The normal ABI requres that the caller sign/zero extend short values to 32 
bit.  We use unsigned int arguments to prevent the gcc making assumptions 
about the high half of the register."

Paul
diff mbox

Patch

commit 03ffa771d52daeee8bf757bcdc2d982aa458aebd
Author: Julian Brown <jbrown@mentor.com>
Date:   Wed Jul 18 08:43:12 2012 -0700

    EABI half-precision helper function names.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index ff46dd9..c22c2b7 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -1238,12 +1238,12 @@  arm_init_libfuncs (void)
       /* Conversions.  */
       set_conv_libfunc (trunc_optab, HFmode, SFmode,
 			(arm_fp16_format == ARM_FP16_FORMAT_IEEE
-			 ? "__gnu_f2h_ieee"
-			 : "__gnu_f2h_alternative"));
+			 ? "__aeabi_f2h"
+			 : "__aeabi_f2h_alt"));
       set_conv_libfunc (sext_optab, SFmode, HFmode,
 			(arm_fp16_format == ARM_FP16_FORMAT_IEEE
-			 ? "__gnu_h2f_ieee"
-			 : "__gnu_h2f_alternative"));
+			 ? "__aeabi_h2f"
+			 : "__aeabi_h2f_alt"));
 
       /* Arithmetic.  */
       set_optab_libfunc (add_optab, HFmode, NULL);
diff --git a/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-5.C b/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-5.C
index 92bc8a9..738d26d 100644
--- a/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-5.C
+++ b/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-5.C
@@ -13,3 +13,5 @@ 
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h\[a-z\]*_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h2f_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_f2h_ieee" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_h2f" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_f2h" } } */
diff --git a/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-6.C b/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-6.C
index ae40b1e..a0e09c8 100644
--- a/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-6.C
+++ b/gcc/testsuite/g++.dg/ext/arm-fp16/arm-fp16-ops-6.C
@@ -13,3 +13,5 @@ 
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h\[a-z\]*_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h2f_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_f2h_ieee" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_h2f" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_f2h" } } */
diff --git a/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-5.c b/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-5.c
index 92bc8a9..738d26d 100644
--- a/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-5.c
+++ b/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-5.c
@@ -13,3 +13,5 @@ 
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h\[a-z\]*_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h2f_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_f2h_ieee" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_h2f" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_f2h" } } */
diff --git a/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-6.c b/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-6.c
index ae40b1e..a0e09c8 100644
--- a/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-6.c
+++ b/gcc/testsuite/gcc.dg/torture/arm-fp16-ops-6.c
@@ -13,3 +13,5 @@ 
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h\[a-z\]*_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_h2f_ieee" } } */
 /* { dg-final { scan-assembler-not "\tbl\t__gnu_f2h_ieee" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_h2f" } } */
+/* { dg-final { scan-assembler-not "\tbl\t__aeabi_f2h" } } */
diff --git a/libgcc/config/arm/fp16.c b/libgcc/config/arm/fp16.c
index 936caeb..a49401d 100644
--- a/libgcc/config/arm/fp16.c
+++ b/libgcc/config/arm/fp16.c
@@ -22,10 +22,10 @@ 
    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-static inline unsigned short
+static inline unsigned int
 __gnu_f2h_internal(unsigned int a, int ieee)
 {
-  unsigned short sign = (a >> 16) & 0x8000;
+  unsigned int sign = (a >> 16) & 0x8000;
   int aexp = (a >> 23) & 0xff;
   unsigned int mantissa = a & 0x007fffff;
   unsigned int mask;
@@ -95,10 +95,10 @@  __gnu_f2h_internal(unsigned int a, int ieee)
   return sign | (((aexp + 14) << 10) + (mantissa >> 13));
 }
 
-unsigned int
-__gnu_h2f_internal(unsigned short a, int ieee)
+static inline unsigned int
+__gnu_h2f_internal(unsigned int a, int ieee)
 {
-  unsigned int sign = (unsigned int)(a & 0x8000) << 16;
+  unsigned int sign = (a & 0x00008000) << 16;
   int aexp = (a >> 10) & 0x1f;
   unsigned int mantissa = a & 0x3ff;
 
@@ -120,26 +120,33 @@  __gnu_h2f_internal(unsigned short a, int ieee)
   return sign | (((aexp + 0x70) << 23) + (mantissa << 13));
 }
 
-unsigned short
+#define ALIAS(src, dst) \
+  typeof (src) dst __attribute__ ((alias (#src)));
+
+unsigned int
 __gnu_f2h_ieee(unsigned int a)
 {
   return __gnu_f2h_internal(a, 1);
 }
+ALIAS (__gnu_f2h_ieee, __aeabi_f2h)
 
 unsigned int
-__gnu_h2f_ieee(unsigned short a)
+__gnu_h2f_ieee(unsigned int a)
 {
   return __gnu_h2f_internal(a, 1);
 }
+ALIAS (__gnu_h2f_ieee, __aeabi_h2f)
 
-unsigned short
+unsigned int
 __gnu_f2h_alternative(unsigned int x)
 {
   return __gnu_f2h_internal(x, 0);
 }
+ALIAS (__gnu_f2h_alternative, __aeabi_f2h_alt)
 
 unsigned int
-__gnu_h2f_alternative(unsigned short a)
+__gnu_h2f_alternative(unsigned int a)
 {
   return __gnu_h2f_internal(a, 0);
 }
+ALIAS (__gnu_h2f_alternative, __aeabi_h2f_alt)
diff --git a/libgcc/config/arm/libgcc-bpabi.ver b/libgcc/config/arm/libgcc-bpabi.ver
index 3ba8364..5bb5f04 100644
--- a/libgcc/config/arm/libgcc-bpabi.ver
+++ b/libgcc/config/arm/libgcc-bpabi.ver
@@ -106,3 +106,10 @@  GCC_3.5 {
 GCC_4.3.0 {
   _Unwind_Backtrace
 }
+
+GCC_4.7.0 {
+  __aeabi_f2h
+  __aeabi_f2h_alt
+  __aeabi_h2f
+  __aeabi_h2f_alt
+}
diff --git a/libgcc/config/arm/sfp-machine.h b/libgcc/config/arm/sfp-machine.h
index a89d05a..f2d7a37 100644
--- a/libgcc/config/arm/sfp-machine.h
+++ b/libgcc/config/arm/sfp-machine.h
@@ -99,7 +99,7 @@  typedef int __gcc_CMPtype __attribute__ ((mode (__libgcc_cmp_return__)));
 #define __fixdfdi	__aeabi_d2lz
 #define __fixunsdfdi	__aeabi_d2ulz
 #define __floatdidf	__aeabi_l2d
-#define __extendhfsf2	__gnu_h2f_ieee
-#define __truncsfhf2	__gnu_f2h_ieee
+#define __extendhfsf2	__aeabi_h2f
+#define __truncsfhf2	__aeabi_f2h
 
 #endif /* __ARM_EABI__ */
diff --git a/libgcc/config/arm/t-bpabi b/libgcc/config/arm/t-bpabi
index e79cbd7..adf977a 100644
--- a/libgcc/config/arm/t-bpabi
+++ b/libgcc/config/arm/t-bpabi
@@ -3,9 +3,8 @@  LIB1ASMFUNCS += _aeabi_lcmp _aeabi_ulcmp _aeabi_ldivmod _aeabi_uldivmod
 
 # Add the BPABI C functions.
 LIB2ADD += $(srcdir)/config/arm/bpabi.c \
-	   $(srcdir)/config/arm/unaligned-funcs.c
-
-LIB2ADD_ST += $(srcdir)/config/arm/fp16.c
+	   $(srcdir)/config/arm/unaligned-funcs.c \
+	   $(srcdir)/config/arm/fp16.c
 
 LIB2ADDEH = $(srcdir)/config/arm/unwind-arm.c \
   $(srcdir)/config/arm/libunwind.S \
diff --git a/libgcc/config/arm/t-symbian b/libgcc/config/arm/t-symbian
index d573157..248bf78 100644
--- a/libgcc/config/arm/t-symbian
+++ b/libgcc/config/arm/t-symbian
@@ -13,7 +13,7 @@  LIB1ASMFUNCS += \
 	_fixsfsi _fixunssfsi
 
 # Include half-float helpers.
-LIB2ADD_ST += $(srcdir)/config/arm/fp16.c
+LIB2ADD += $(srcdir)/config/arm/fp16.c
 
 # Include the gcc personality routine
 LIB2ADDEH = $(srcdir)/unwind-c.c $(srcdir)/config/arm/pr-support.c