Re: RTABI half-precision conversion functions (ping)
On Thu, 29 May 2014 11:16:52 +0100 Julian Brown wrote: > On Thu, 19 Jul 2012 14:47:54 +0100 > Julian Brown wrote: > > > On Thu, 19 Jul 2012 13:54:57 +0100 > > Paul Brook 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
Re: RTABI half-precision conversion functions (ping)
On Thu, 19 Jul 2012 14:47:54 +0100 Julian Brown wrote: > On Thu, 19 Jul 2012 13:54:57 +0100 > Paul Brook 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
Re: RTABI half-precision conversion functions (ping)
On Thu, 19 Jul 2012 13:54:57 +0100
Paul Brook 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?
Juliancommit f858cdd91188784794418b3456d06172df654dc3
Author: Julian Brown
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
---
Re: RTABI half-precision conversion functions (ping)
> 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
RTABI half-precision conversion functions (ping)
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
Julian Brown
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.
commit 03ffa771d52daeee8bf757bcdc2d982aa458aebd
Author: Julian Brown
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" } } */
/* {
