| Message ID | 20160720074813.GA31574@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On ARM soft-float, the float to double conversion doesn't convert a sNaN > to qNaN as the IEEE Std 754 standard mandates: > > "Under default exception handling, any operation signaling an invalid > operation exception and for which a floating-point result is to be > delivered shall deliver a quiet NaN." > > Given the soft float ARM code ignores exceptions and always provides a > result, a float to double conversion of a signaling NaN should return a > quiet NaN. Fix this in extendsfdf2. > > gcc/ChangeLog: > > PR target/59833 > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. > > gcc/testsuite/ChangeLog: > > * gcc.dg/pr59833.c: New testcase. Ok - assuming this was tested appropriately with no regressions. Thanks for following up. Ramana > --- > gcc/testsuite/gcc.dg/pr59833.c | 18 ++++++++++++++++++ > libgcc/config/arm/ieee754-df.S | 10 +++++++--- > 2 files changed, 25 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr59833.c > > diff --git a/gcc/testsuite/gcc.dg/pr59833.c b/gcc/testsuite/gcc.dg/pr59833.c > new file mode 100644 > index 0000000..e0e4ed5 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr59833.c > @@ -0,0 +1,18 @@ > +/* { dg-do run { target { *-*-linux* *-*-gnu* } } } */ > +/* { dg-options "-O0 -lm" } */ > +/* { dg-require-effective-target issignaling } */ > + > +#define _GNU_SOURCE > +#include <math.h> > + > +int main (void) > +{ > + float sNaN = __builtin_nansf (""); > + double x = (double) sNaN; > + if (issignaling(x)) > + { > + __builtin_abort(); > + } > + > + return 0; > +} > diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S > index a2aac70..1ecaa9d 100644 > --- a/libgcc/config/arm/ieee754-df.S > +++ b/libgcc/config/arm/ieee754-df.S > @@ -507,11 +507,15 @@ ARM_FUNC_ALIAS aeabi_f2d extendsfdf2 > eorne xh, xh, #0x38000000 @ fixup exponent otherwise. > RETc(ne) @ and return it. > > - teq r2, #0 @ if actually 0 > - do_it ne, e > - teqne r3, #0xff000000 @ or INF or NAN > + bics r2, r2, #0xff000000 @ isolate mantissa > + do_it eq @ if 0, that is ZERO or INF, > RETc(eq) @ we are done already. > > + teq r3, #0xff000000 @ check for NAN > + do_it eq, t > + orreq xh, xh, #0x00080000 @ change to quiet NAN > + RETc(eq) @ and return it. > + > @ value was denormalized. We can normalize it now. > do_push {r4, r5, lr} > .cfi_adjust_cfa_offset 12 @ CFA is now sp + previousOffset + 12 > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
On 2016-07-20 10:10, Ramana Radhakrishnan wrote: > On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On ARM soft-float, the float to double conversion doesn't convert a sNaN > > to qNaN as the IEEE Std 754 standard mandates: > > > > "Under default exception handling, any operation signaling an invalid > > operation exception and for which a floating-point result is to be > > delivered shall deliver a quiet NaN." > > > > Given the soft float ARM code ignores exceptions and always provides a > > result, a float to double conversion of a signaling NaN should return a > > quiet NaN. Fix this in extendsfdf2. > > > > gcc/ChangeLog: > > > > PR target/59833 > > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/pr59833.c: New testcase. > > > Ok - assuming this was tested appropriately with no regressions. Given it only touches arm code, I only tested it on arm and I have seen no regression. That said I wouldn't be surprised if the new testcase fails on some other architectures. Also I have done the copyright assignment for GCC, but I do not have SVN write access. Could someone please commit this for me? Thanks, Aurelien
On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2016-07-20 10:10, Ramana Radhakrishnan wrote: >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN >> > to qNaN as the IEEE Std 754 standard mandates: >> > >> > "Under default exception handling, any operation signaling an invalid >> > operation exception and for which a floating-point result is to be >> > delivered shall deliver a quiet NaN." >> > >> > Given the soft float ARM code ignores exceptions and always provides a >> > result, a float to double conversion of a signaling NaN should return a >> > quiet NaN. Fix this in extendsfdf2. >> > >> > gcc/ChangeLog: >> > >> > PR target/59833 >> > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. >> > >> > gcc/testsuite/ChangeLog: >> > >> > * gcc.dg/pr59833.c: New testcase. >> >> >> Ok - assuming this was tested appropriately with no regressions. > > Given it only touches arm code, I only tested it on arm and I have seen > no regression. That said I wouldn't be surprised if the new testcase > fails on some other architectures. I was assuming you tested it on ARM :) In this case given the change is only in the backend I would have expected this patch to have been tested for soft-float ARM or an appropriate multilib. Saying what configuration the patch was tested on is useful for the audit trail. For e.g. it's no use testing this patch on armhf ( i.e. --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by default the test would never generate the call to the library function but I'm sure you know all that anyway. I don't know if this would pass or fail on other architectures - if it fails it indicates a bug in their ports and for them to fix up as appropriate. > > Also I have done the copyright assignment for GCC, but I do not have SVN > write access. Could someone please commit this for me? I will take care of it. Thanks again for following up on the patch. Thanks, Ramana > > Thanks, > Aurelien > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
On 2016-07-20 11:04, Ramana Radhakrishnan wrote: > On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2016-07-20 10:10, Ramana Radhakrishnan wrote: > >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN > >> > to qNaN as the IEEE Std 754 standard mandates: > >> > > >> > "Under default exception handling, any operation signaling an invalid > >> > operation exception and for which a floating-point result is to be > >> > delivered shall deliver a quiet NaN." > >> > > >> > Given the soft float ARM code ignores exceptions and always provides a > >> > result, a float to double conversion of a signaling NaN should return a > >> > quiet NaN. Fix this in extendsfdf2. > >> > > >> > gcc/ChangeLog: > >> > > >> > PR target/59833 > >> > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > * gcc.dg/pr59833.c: New testcase. > >> > >> > >> Ok - assuming this was tested appropriately with no regressions. > > > > Given it only touches arm code, I only tested it on arm and I have seen > > no regression. That said I wouldn't be surprised if the new testcase > > fails on some other architectures. > > I was assuming you tested it on ARM :) In this case given the change > is only in the backend I would have expected this patch to have been > tested for soft-float ARM or an appropriate multilib. Saying what > configuration the patch was tested on is useful for the audit trail. > For e.g. it's no use testing this patch on armhf ( i.e. > --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by > default the test would never generate the call to the library function > but I'm sure you know all that anyway. Indeed I should have given more details. I tested it on a Debian armel machine, and I configured GCC the same way as the Debian package, that is using --with-arch=armv4t --with-float=soft. I built it once with the new test but without the fix and a second time with both the test and the fix. I have verified that the test fails in the first case and pass in the second case. > I don't know if this would pass or fail on other architectures - if it > fails it indicates a bug in their ports and for them to fix up as > appropriate. > > > > > > Also I have done the copyright assignment for GCC, but I do not have SVN > > write access. Could someone please commit this for me? > > I will take care of it. Thanks again for following up on the patch. Thanks! Aurelien
On Wed, Jul 20, 2016 at 11:14 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2016-07-20 11:04, Ramana Radhakrishnan wrote: >> On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> > On 2016-07-20 10:10, Ramana Radhakrishnan wrote: >> >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN >> >> > to qNaN as the IEEE Std 754 standard mandates: >> >> > >> >> > "Under default exception handling, any operation signaling an invalid >> >> > operation exception and for which a floating-point result is to be >> >> > delivered shall deliver a quiet NaN." >> >> > >> >> > Given the soft float ARM code ignores exceptions and always provides a >> >> > result, a float to double conversion of a signaling NaN should return a >> >> > quiet NaN. Fix this in extendsfdf2. >> >> > >> >> > gcc/ChangeLog: >> >> > >> >> > PR target/59833 >> >> > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. >> >> > >> >> > gcc/testsuite/ChangeLog: >> >> > >> >> > * gcc.dg/pr59833.c: New testcase. >> >> >> >> >> >> Ok - assuming this was tested appropriately with no regressions. >> > >> > Given it only touches arm code, I only tested it on arm and I have seen >> > no regression. That said I wouldn't be surprised if the new testcase >> > fails on some other architectures. >> >> I was assuming you tested it on ARM :) In this case given the change >> is only in the backend I would have expected this patch to have been >> tested for soft-float ARM or an appropriate multilib. Saying what >> configuration the patch was tested on is useful for the audit trail. >> For e.g. it's no use testing this patch on armhf ( i.e. >> --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by >> default the test would never generate the call to the library function >> but I'm sure you know all that anyway. > > Indeed I should have given more details. I tested it on a Debian armel > machine, and I configured GCC the same way as the Debian package, that > is using --with-arch=armv4t --with-float=soft. > > I built it once with the new test but without the fix and a second time > with both the test and the fix. I have verified that the test fails in > the first case and pass in the second case. Thanks for the info - what about all the other regression tests ? Did you do a full make check and ensure that no other tests regressed in comparison ? Patches need to be tested against the entire regression testsuite and not just what was added. regards Ramana
On 2016-07-20 11:22, Ramana Radhakrishnan wrote: > On Wed, Jul 20, 2016 at 11:14 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2016-07-20 11:04, Ramana Radhakrishnan wrote: > >> On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > >> > On 2016-07-20 10:10, Ramana Radhakrishnan wrote: > >> >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > >> >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN > >> >> > to qNaN as the IEEE Std 754 standard mandates: > >> >> > > >> >> > "Under default exception handling, any operation signaling an invalid > >> >> > operation exception and for which a floating-point result is to be > >> >> > delivered shall deliver a quiet NaN." > >> >> > > >> >> > Given the soft float ARM code ignores exceptions and always provides a > >> >> > result, a float to double conversion of a signaling NaN should return a > >> >> > quiet NaN. Fix this in extendsfdf2. > >> >> > > >> >> > gcc/ChangeLog: > >> >> > > >> >> > PR target/59833 > >> >> > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. > >> >> > > >> >> > gcc/testsuite/ChangeLog: > >> >> > > >> >> > * gcc.dg/pr59833.c: New testcase. > >> >> > >> >> > >> >> Ok - assuming this was tested appropriately with no regressions. > >> > > >> > Given it only touches arm code, I only tested it on arm and I have seen > >> > no regression. That said I wouldn't be surprised if the new testcase > >> > fails on some other architectures. > >> > >> I was assuming you tested it on ARM :) In this case given the change > >> is only in the backend I would have expected this patch to have been > >> tested for soft-float ARM or an appropriate multilib. Saying what > >> configuration the patch was tested on is useful for the audit trail. > >> For e.g. it's no use testing this patch on armhf ( i.e. > >> --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by > >> default the test would never generate the call to the library function > >> but I'm sure you know all that anyway. > > > > Indeed I should have given more details. I tested it on a Debian armel > > machine, and I configured GCC the same way as the Debian package, that > > is using --with-arch=armv4t --with-float=soft. > > > > I built it once with the new test but without the fix and a second time > > with both the test and the fix. I have verified that the test fails in > > the first case and pass in the second case. > > Thanks for the info - what about all the other regression tests ? Did > you do a full make check and ensure that no other tests regressed in > comparison ? Patches need to be tested against the entire regression > testsuite and not just what was added. Yes, I compared the testsuite result between the two runs, and there are identical beside this new test (hence my "I have seen no regression" in my first answer). Aurelien
On Wed, Jul 20, 2016 at 1:40 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2016-07-20 11:22, Ramana Radhakrishnan wrote: >> On Wed, Jul 20, 2016 at 11:14 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> > On 2016-07-20 11:04, Ramana Radhakrishnan wrote: >> >> On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> >> > On 2016-07-20 10:10, Ramana Radhakrishnan wrote: >> >> >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> >> >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN >> >> >> > to qNaN as the IEEE Std 754 standard mandates: >> >> >> > >> >> >> > "Under default exception handling, any operation signaling an invalid >> >> >> > operation exception and for which a floating-point result is to be >> >> >> > delivered shall deliver a quiet NaN." >> >> >> > >> >> >> > Given the soft float ARM code ignores exceptions and always provides a >> >> >> > result, a float to double conversion of a signaling NaN should return a >> >> >> > quiet NaN. Fix this in extendsfdf2. >> >> >> > >> >> >> > gcc/ChangeLog: >> >> >> > >> >> >> > PR target/59833 >> >> >> > * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN. >> >> >> > >> >> >> > gcc/testsuite/ChangeLog: >> >> >> > >> >> >> > * gcc.dg/pr59833.c: New testcase. >> >> >> >> >> >> >> >> >> Ok - assuming this was tested appropriately with no regressions. >> >> > >> >> > Given it only touches arm code, I only tested it on arm and I have seen >> >> > no regression. That said I wouldn't be surprised if the new testcase >> >> > fails on some other architectures. >> >> >> >> I was assuming you tested it on ARM :) In this case given the change >> >> is only in the backend I would have expected this patch to have been >> >> tested for soft-float ARM or an appropriate multilib. Saying what >> >> configuration the patch was tested on is useful for the audit trail. >> >> For e.g. it's no use testing this patch on armhf ( i.e. >> >> --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by >> >> default the test would never generate the call to the library function >> >> but I'm sure you know all that anyway. >> > >> > Indeed I should have given more details. I tested it on a Debian armel >> > machine, and I configured GCC the same way as the Debian package, that >> > is using --with-arch=armv4t --with-float=soft. >> > >> > I built it once with the new test but without the fix and a second time >> > with both the test and the fix. I have verified that the test fails in >> > the first case and pass in the second case. >> >> Thanks for the info - what about all the other regression tests ? Did >> you do a full make check and ensure that no other tests regressed in >> comparison ? Patches need to be tested against the entire regression >> testsuite and not just what was added. > > Yes, I compared the testsuite result between the two runs, and there are > identical beside this new test (hence my "I have seen no regression" in > my first answer). > Thanks now fixed on trunk. Ramana > Aurelien > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
diff --git a/gcc/testsuite/gcc.dg/pr59833.c b/gcc/testsuite/gcc.dg/pr59833.c new file mode 100644 index 0000000..e0e4ed5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr59833.c @@ -0,0 +1,18 @@ +/* { dg-do run { target { *-*-linux* *-*-gnu* } } } */ +/* { dg-options "-O0 -lm" } */ +/* { dg-require-effective-target issignaling } */ + +#define _GNU_SOURCE +#include <math.h> + +int main (void) +{ + float sNaN = __builtin_nansf (""); + double x = (double) sNaN; + if (issignaling(x)) + { + __builtin_abort(); + } + + return 0; +} diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S index a2aac70..1ecaa9d 100644 --- a/libgcc/config/arm/ieee754-df.S +++ b/libgcc/config/arm/ieee754-df.S @@ -507,11 +507,15 @@ ARM_FUNC_ALIAS aeabi_f2d extendsfdf2 eorne xh, xh, #0x38000000 @ fixup exponent otherwise. RETc(ne) @ and return it. - teq r2, #0 @ if actually 0 - do_it ne, e - teqne r3, #0xff000000 @ or INF or NAN + bics r2, r2, #0xff000000 @ isolate mantissa + do_it eq @ if 0, that is ZERO or INF, RETc(eq) @ we are done already. + teq r3, #0xff000000 @ check for NAN + do_it eq, t + orreq xh, xh, #0x00080000 @ change to quiet NAN + RETc(eq) @ and return it. + @ value was denormalized. We can normalize it now. do_push {r4, r5, lr} .cfi_adjust_cfa_offset 12 @ CFA is now sp + previousOffset + 12