diff mbox

RTABI half-precision conversion functions (ping)

Message ID 20120719144754.514db0e3@octopus
State New
Headers show

Commit Message

Julian Brown July 19, 2012, 1:47 p.m. UTC 
On Thu, 19 Jul 2012 13:54:57 +0100
Paul Brook <paul@codesourcery.com> wrote:

> > 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.

Let's not worry about that for now :-).

> > 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."

Here's a version with an explanatory comment. I also fixed a couple of
minor formatting nits I noticed (they don't upset the diff too much, I
don't think).

OK?

Julian

Comments

Julian Brown May 29, 2014, 10:16 a.m. UTC | #1 
On Thu, 19 Jul 2012 14:47:54 +0100
Julian Brown <julian@codesourcery.com> wrote:

> On Thu, 19 Jul 2012 13:54:57 +0100
> Paul Brook <paul@codesourcery.com> wrote:
> 
> > > 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.
> 
> Let's not worry about that for now :-).
> 
> > > 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."
> 
> Here's a version with an explanatory comment. I also fixed a couple of
> minor formatting nits I noticed (they don't upset the diff too much, I
> don't think).

It looks like this one got forgotten about. Ping?

Context:

https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00902.html
https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00912.html

This is an EABI-conformance fix.

Thanks,

Julian
Julian Brown June 24, 2014, 11:24 a.m. UTC | #2 
On Thu, 29 May 2014 11:16:52 +0100
Julian Brown <julian@codesourcery.com> wrote:

> On Thu, 19 Jul 2012 14:47:54 +0100
> Julian Brown <julian@codesourcery.com> wrote:
> 
> > On Thu, 19 Jul 2012 13:54:57 +0100
> > Paul Brook <paul@codesourcery.com> wrote:
> > 
> > > > 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.
> > 
> > Let's not worry about that for now :-).
> > 
> > > > 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."
> > 
> > Here's a version with an explanatory comment. I also fixed a couple
> > of minor formatting nits I noticed (they don't upset the diff too
> > much, I don't think).
> 
> It looks like this one got forgotten about. Ping?
> 
> Context:
> 
> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00902.html
> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00912.html
> 
> This is an EABI-conformance fix.

Ping?

Julian
diff mbox

Patch

commit f858cdd91188784794418b3456d06172df654dc3
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..0fb2fe4 100644
--- a/libgcc/config/arm/fp16.c
+++ b/libgcc/config/arm/fp16.c
@@ -22,10 +22,19 @@ 
    see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-static inline unsigned short
-__gnu_f2h_internal(unsigned int a, int ieee)
+/* Note: While the EABI (Run-time ABI for the ARM (R) Architecture, IHI 0043C)
+   describes the helper routine arguments and return types representing
+   half-float values using the type 'short', it does not require that they be
+   extended to full register width.
+   The ABI normally requires that the caller sign or zero extends short (or
+   unsigned short) values to 32 bits.  We use unsigned int arguments and return
+   types to prevent GCC making assumptions about the high halves of the
+   registers in question.  */
+
+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 +104,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 +129,33 @@  __gnu_h2f_internal(unsigned short a, int ieee)
   return sign | (((aexp + 0x70) << 23) + (mantissa << 13));
 }
 
-unsigned short
-__gnu_f2h_ieee(unsigned int a)
+#define ALIAS(src, dst) \
+  typeof (src) dst __attribute__ ((alias (#src)));
+
+unsigned int
+__gnu_f2h_ieee (unsigned int a)
 {
-  return __gnu_f2h_internal(a, 1);
+  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);
+  return __gnu_h2f_internal (a, 1);
 }
+ALIAS (__gnu_h2f_ieee, __aeabi_h2f)
 
-unsigned short
-__gnu_f2h_alternative(unsigned int x)
+unsigned int
+__gnu_f2h_alternative (unsigned int x)
 {
-  return __gnu_f2h_internal(x, 0);
+  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);
+  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