Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On 27/07/18 17:45, Nicolas Pitre wrote: > On Fri, 27 Jul 2018, Wilco Dijkstra wrote: > >> Nicolas Pitre wrote: >> However if r4 is non-zero, the carry will be set, and the tsths will be executed. This clears the carry and sets the Z flag based on bit 20. >>> >>> No, not at all. The carry is not affected. And that's the point of the >>> tst instruction here rather than a cmp: it sets the N and Z flags but >>> leaves C alone as there is no shifter involved. >> >> No, the carry is always set by logical operations with a shifted immediate. >> It is only >> unchanged if the immediate uses a zero rotate. So any shifted immediate > 255 >> will set the carry. > > Holy cow !!! > > Who knows all the bugs I must have created in the past 25 years having > missed this particular subtlety. > > Here's the updated patch with your suggestion. > Thanks. Committed. R. > > fix.patch > > > diff --git a/libgcc/ChangeLog b/libgcc/ChangeLog > index c13bf4cb2f6..c19d05c8a2e 100644 > --- a/libgcc/ChangeLog > +++ b/libgcc/ChangeLog > @@ -1,3 +1,9 @@ > +2018-07-26 Nicolas Pitre > + > + * config/arm/ieee754-df.S: Don't shortcut denormal handling when > + exponent goes negative. Update my email address. > + * config/arm/ieee754-sf.S: Likewise. > + > 2018-07-05 James Clarke > > * configure: Regenerated. > diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S > index 8741aa99245..9a1b2dd2a4e 100644 > --- a/libgcc/config/arm/ieee754-df.S > +++ b/libgcc/config/arm/ieee754-df.S > @@ -1,7 +1,7 @@ > /* ieee754-df.S double-precision floating point support for ARM > > Copyright (C) 2003-2018 Free Software Foundation, Inc. > - Contributed by Nicolas Pitre (n...@cam.org) > + Contributed by Nicolas Pitre (n...@fluxnic.net) > > This file is free software; you can redistribute it and/or modify it > under the terms of the GNU General Public License as published by the > @@ -238,9 +238,10 @@ LSYM(Lad_a): > movsip, ip, lsl #1 > adcsxl, xl, xl > adc xh, xh, xh > - tst xh, #0x0010 > - sub r4, r4, #1 > - bne LSYM(Lad_e) > + subsr4, r4, #1 > + do_it hs > + cmphs xh, #0x0010 > + bhs LSYM(Lad_e) > > @ No rounding necessary since ip will always be 0 at this point. > LSYM(Lad_l): > diff --git a/libgcc/config/arm/ieee754-sf.S b/libgcc/config/arm/ieee754-sf.S > index d80d5e9080c..b8a81279a3c 100644 > --- a/libgcc/config/arm/ieee754-sf.S > +++ b/libgcc/config/arm/ieee754-sf.S > @@ -1,7 +1,7 @@ > /* ieee754-sf.S single-precision floating point support for ARM > > Copyright (C) 2003-2018 Free Software Foundation, Inc. > - Contributed by Nicolas Pitre (n...@cam.org) > + Contributed by Nicolas Pitre (n...@fluxnic.net) > > This file is free software; you can redistribute it and/or modify it > under the terms of the GNU General Public License as published by the > @@ -168,10 +168,11 @@ LSYM(Lad_e): > LSYM(Lad_a): > movsr1, r1, lsl #1 > adc r0, r0, r0 > - tst r0, #0x0080 > - sub r2, r2, #1 > - bne LSYM(Lad_e) > - > + subsr2, r2, #1 > + do_it hs > + cmphs r0, #0x0080 > + bhs LSYM(Lad_e) > + > @ No rounding necessary since r1 will always be 0 at this point. > LSYM(Lad_l): > >
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On Fri, 27 Jul 2018, Wilco Dijkstra wrote: > Nicolas Pitre wrote: > > >> However if r4 is non-zero, the carry will be set, and the tsths will be > >> executed. This > >> clears the carry and sets the Z flag based on bit 20. > > > > No, not at all. The carry is not affected. And that's the point of the > > tst instruction here rather than a cmp: it sets the N and Z flags but > > leaves C alone as there is no shifter involved. > > No, the carry is always set by logical operations with a shifted immediate. > It is only > unchanged if the immediate uses a zero rotate. So any shifted immediate > 255 > will set the carry. Holy cow !!! Who knows all the bugs I must have created in the past 25 years having missed this particular subtlety. Here's the updated patch with your suggestion. diff --git a/libgcc/ChangeLog b/libgcc/ChangeLog index c13bf4cb2f6..c19d05c8a2e 100644 --- a/libgcc/ChangeLog +++ b/libgcc/ChangeLog @@ -1,3 +1,9 @@ +2018-07-26 Nicolas Pitre + + * config/arm/ieee754-df.S: Don't shortcut denormal handling when + exponent goes negative. Update my email address. + * config/arm/ieee754-sf.S: Likewise. + 2018-07-05 James Clarke * configure: Regenerated. diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S index 8741aa99245..9a1b2dd2a4e 100644 --- a/libgcc/config/arm/ieee754-df.S +++ b/libgcc/config/arm/ieee754-df.S @@ -1,7 +1,7 @@ /* ieee754-df.S double-precision floating point support for ARM Copyright (C) 2003-2018 Free Software Foundation, Inc. - Contributed by Nicolas Pitre (n...@cam.org) + Contributed by Nicolas Pitre (n...@fluxnic.net) This file is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the @@ -238,9 +238,10 @@ LSYM(Lad_a): movsip, ip, lsl #1 adcsxl, xl, xl adc xh, xh, xh - tst xh, #0x0010 - sub r4, r4, #1 - bne LSYM(Lad_e) + subsr4, r4, #1 + do_it hs + cmphs xh, #0x0010 + bhs LSYM(Lad_e) @ No rounding necessary since ip will always be 0 at this point. LSYM(Lad_l): diff --git a/libgcc/config/arm/ieee754-sf.S b/libgcc/config/arm/ieee754-sf.S index d80d5e9080c..b8a81279a3c 100644 --- a/libgcc/config/arm/ieee754-sf.S +++ b/libgcc/config/arm/ieee754-sf.S @@ -1,7 +1,7 @@ /* ieee754-sf.S single-precision floating point support for ARM Copyright (C) 2003-2018 Free Software Foundation, Inc. - Contributed by Nicolas Pitre (n...@cam.org) + Contributed by Nicolas Pitre (n...@fluxnic.net) This file is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the @@ -168,10 +168,11 @@ LSYM(Lad_e): LSYM(Lad_a): movsr1, r1, lsl #1 adc r0, r0, r0 - tst r0, #0x0080 - sub r2, r2, #1 - bne LSYM(Lad_e) - + subsr2, r2, #1 + do_it hs + cmphs r0, #0x0080 + bhs LSYM(Lad_e) + @ No rounding necessary since r1 will always be 0 at this point. LSYM(Lad_l):
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Nicolas Pitre wrote: >> However if r4 is non-zero, the carry will be set, and the tsths will be >> executed. This >> clears the carry and sets the Z flag based on bit 20. > > No, not at all. The carry is not affected. And that's the point of the > tst instruction here rather than a cmp: it sets the N and Z flags but > leaves C alone as there is no shifter involved. No, the carry is always set by logical operations with a shifted immediate. It is only unchanged if the immediate uses a zero rotate. So any shifted immediate > 255 will set the carry. This is detailed in the Arm Architecture Reference Manual, eg. see the pseudo code for A32ExpandImm_C in LSL (immediate). Wilco
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On Fri, 27 Jul 2018, Nicolas Pitre wrote: > On Fri, 27 Jul 2018, Wilco Dijkstra wrote: > > > Hi Nicolas, > > > > I think your patch doesn't quite work as expected: > > > > @@ -238,9 +238,10 @@ LSYM(Lad_a): > > movsip, ip, lsl #1 > > adcsxl, xl, xl > > adc xh, xh, xh > > - tst xh, #0x0010 > > - sub r4, r4, #1 > > - bne LSYM(Lad_e) > > + subsr4, r4, #1 > > + do_it hs > > + tsths xh, #0x0010 > > + bhi LSYM(Lad_e) > > > > If the exponent in r4 is zero, the carry bit will be clear, so we don't > > execute the tsths > > and fallthrough (the denormal will be normalized and then denormalized > > again, but > > that's so rare it doesn't matter really). > > And that's what is intended. > > > However if r4 is non-zero, the carry will be set, and the tsths will be > > executed. This > > clears the carry and sets the Z flag based on bit 20. > > No, not at all. The carry is not affected. And that's the point of the > tst instruction here rather than a cmp: it sets the N and Z flags but > leaves C alone as there is no shifter involved. That being said, a cmp as you suggested would have worked too (with the bhi changed to bhs). But the patch as is produces the same behavior. Nicolas
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On Fri, 27 Jul 2018, Wilco Dijkstra wrote: > Hi Nicolas, > > I think your patch doesn't quite work as expected: > > @@ -238,9 +238,10 @@ LSYM(Lad_a): > movsip, ip, lsl #1 > adcsxl, xl, xl > adc xh, xh, xh > - tst xh, #0x0010 > - sub r4, r4, #1 > - bne LSYM(Lad_e) > + subsr4, r4, #1 > + do_it hs > + tsths xh, #0x0010 > + bhi LSYM(Lad_e) > > If the exponent in r4 is zero, the carry bit will be clear, so we don't > execute the tsths > and fallthrough (the denormal will be normalized and then denormalized again, > but > that's so rare it doesn't matter really). And that's what is intended. > However if r4 is non-zero, the carry will be set, and the tsths will be > executed. This > clears the carry and sets the Z flag based on bit 20. No, not at all. The carry is not affected. And that's the point of the tst instruction here rather than a cmp: it sets the N and Z flags but leaves C alone as there is no shifter involved. Nicolas
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Hi Nicolas, I think your patch doesn't quite work as expected: @@ -238,9 +238,10 @@ LSYM(Lad_a): movsip, ip, lsl #1 adcsxl, xl, xl adc xh, xh, xh - tst xh, #0x0010 - sub r4, r4, #1 - bne LSYM(Lad_e) + subsr4, r4, #1 + do_it hs + tsths xh, #0x0010 + bhi LSYM(Lad_e) If the exponent in r4 is zero, the carry bit will be clear, so we don't execute the tsths and fallthrough (the denormal will be normalized and then denormalized again, but that's so rare it doesn't matter really). However if r4 is non-zero, the carry will be set, and the tsths will be executed. This clears the carry and sets the Z flag based on bit 20. We will now also always fallthrough rather than take the branch if bit 20 is non-zero. This may still give the correct answer, however it would add considerable extra overhead... I think using a cmp rather than tst would work. Wilco
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Umesh Kalappa wrote: > Any more suggestions or comments on the patch ? The patch is suboptimal as it introduces 2 additional instructions in a fairly common path for a branch that is very unlikely to be taken in practice. I'm therefore proposing this alternative patch to fix the issue in an optimal way. I'm also using this opportunity to update my email address as the one currently in those files has been obsolete for more than 10 years at this point, in the hope that I get notified of similar issues directly in the future.diff --git a/libgcc/ChangeLog b/libgcc/ChangeLog index c13bf4cb2f6..c19d05c8a2e 100644 --- a/libgcc/ChangeLog +++ b/libgcc/ChangeLog @@ -1,3 +1,9 @@ +2018-07-26 Nicolas Pitre + + * config/arm/ieee754-df.S: Don't shortcut denormal handling when + exponent goes negative. Update my email address. + * config/arm/ieee754-sf.S: Likewise. + 2018-07-05 James Clarke * configure: Regenerated. diff --git a/libgcc/config/arm/ieee754-df.S b/libgcc/config/arm/ieee754-df.S index 8741aa99245..ee7a9835394 100644 --- a/libgcc/config/arm/ieee754-df.S +++ b/libgcc/config/arm/ieee754-df.S @@ -1,7 +1,7 @@ /* ieee754-df.S double-precision floating point support for ARM Copyright (C) 2003-2018 Free Software Foundation, Inc. - Contributed by Nicolas Pitre (n...@cam.org) + Contributed by Nicolas Pitre (n...@fluxnic.net) This file is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the @@ -238,9 +238,10 @@ LSYM(Lad_a): movsip, ip, lsl #1 adcsxl, xl, xl adc xh, xh, xh - tst xh, #0x0010 - sub r4, r4, #1 - bne LSYM(Lad_e) + subsr4, r4, #1 + do_it hs + tsths xh, #0x0010 + bhi LSYM(Lad_e) @ No rounding necessary since ip will always be 0 at this point. LSYM(Lad_l): diff --git a/libgcc/config/arm/ieee754-sf.S b/libgcc/config/arm/ieee754-sf.S index d80d5e9080c..640c97ed550 100644 --- a/libgcc/config/arm/ieee754-sf.S +++ b/libgcc/config/arm/ieee754-sf.S @@ -1,7 +1,7 @@ /* ieee754-sf.S single-precision floating point support for ARM Copyright (C) 2003-2018 Free Software Foundation, Inc. - Contributed by Nicolas Pitre (n...@cam.org) + Contributed by Nicolas Pitre (n...@fluxnic.net) This file is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the @@ -168,9 +168,10 @@ LSYM(Lad_e): LSYM(Lad_a): movsr1, r1, lsl #1 adc r0, r0, r0 - tst r0, #0x0080 - sub r2, r2, #1 - bne LSYM(Lad_e) + subsr2, r2, #1 + do_it hs + tsths r0, #0x0080 + bhi LSYM(Lad_e) @ No rounding necessary since r1 will always be 0 at this point. LSYM(Lad_l):
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Hi, Any more suggestions or comments on the patch ? Thank you ~Umesh On Tue, Jul 24, 2018, 2:08 PM Umesh Kalappa wrote: > Thank you All for the suggestions and we tried runing the GCC > testsuite and found that no regression with the fix and also ran the > our regressions base for conformance with no regress. > > Is ok for commit with below Changelog ? > +++ libgcc/ChangeLog(working copy) > @@ -1,3 +1,9 @@ > +2018-07-18 Umesh Kalappa > + > + PR libgcc/86512 > + * config/arm/ieee754-df.S :Don't normalise the denormal result. > + * config/arm/ieee754-sf.S:Likewise. > + > + > +++ gcc/testsuite/ChangeLog (working copy) > @@ -1,3 +1,8 @@ > +2018-07-18 Umesh Kalappa > + > + PR libgcc/86512 > + * gcc.target/arm/pr86512.c :New test. > + > > On Mon, Jul 23, 2018 at 5:24 PM, Wilco Dijkstra > wrote: > > Umesh Kalappa wrote: > > > >> We tested on the SP and yes the problem persist on the SP too and > >> attached patch will fix the both SP and DP issues for the denormal > >> resultant. > > > > The patch now looks correct to me (but I can't approve). > > > >> We bootstrapped the compiler ,look ok to us with minimal testing , > >> > >> Any floating point test-suite to test for the attached patch ? any > >> recommendations or inputs ? > > > > Running the GCC regression tests would be required since a bootstrap > isn't > > useful for this kind of change. Assuming you use Linux, building and > running > > GLIBC with the changed GCC would give additional test coverage as it > tests > > all the math library functions. > > > > I don't know of any IEEE conformance testsuites in the GNU world, which > is > > why I'm suggesting running some targeted and randomized tests. You could > > use the generic soft-float code in libgcc/soft-fp/adddf3.c to compare > the outputs. > > > > > Index: libgcc/config/arm/ieee754-df.S > === > --- libgcc/config/arm/ieee754-df.S (revision 262850) > +++ libgcc/config/arm/ieee754-df.S (working copy) > @@ -203,6 +203,7 @@ > #endif > > @ Determine how to normalize the result. > +@ if result is denormal i.e (exp)=0,then don't normalise the > result, > > > > Use a standard sentence here, eg. like: > > > > If exp is zero and the mantissa unnormalized, return a denormal. > > > > Wilco > > >
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Thank you All for the suggestions and we tried runing the GCC testsuite and found that no regression with the fix and also ran the our regressions base for conformance with no regress. Is ok for commit with below Changelog ? +++ libgcc/ChangeLog(working copy) @@ -1,3 +1,9 @@ +2018-07-18 Umesh Kalappa + + PR libgcc/86512 + * config/arm/ieee754-df.S :Don't normalise the denormal result. + * config/arm/ieee754-sf.S:Likewise. + + +++ gcc/testsuite/ChangeLog (working copy) @@ -1,3 +1,8 @@ +2018-07-18 Umesh Kalappa + + PR libgcc/86512 + * gcc.target/arm/pr86512.c :New test. + On Mon, Jul 23, 2018 at 5:24 PM, Wilco Dijkstra wrote: > Umesh Kalappa wrote: > >> We tested on the SP and yes the problem persist on the SP too and >> attached patch will fix the both SP and DP issues for the denormal >> resultant. > > The patch now looks correct to me (but I can't approve). > >> We bootstrapped the compiler ,look ok to us with minimal testing , >> >> Any floating point test-suite to test for the attached patch ? any >> recommendations or inputs ? > > Running the GCC regression tests would be required since a bootstrap isn't > useful for this kind of change. Assuming you use Linux, building and running > GLIBC with the changed GCC would give additional test coverage as it tests > all the math library functions. > > I don't know of any IEEE conformance testsuites in the GNU world, which is > why I'm suggesting running some targeted and randomized tests. You could > use the generic soft-float code in libgcc/soft-fp/adddf3.c to compare the > outputs. > > Index: libgcc/config/arm/ieee754-df.S === --- libgcc/config/arm/ieee754-df.S (revision 262850) +++ libgcc/config/arm/ieee754-df.S (working copy) @@ -203,6 +203,7 @@ #endif @ Determine how to normalize the result. +@ if result is denormal i.e (exp)=0,then don't normalise the result, > > Use a standard sentence here, eg. like: > > If exp is zero and the mantissa unnormalized, return a denormal. > > Wilco > pr86512.patch Description: Binary data
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Umesh Kalappa wrote: > We tested on the SP and yes the problem persist on the SP too and > attached patch will fix the both SP and DP issues for the denormal > resultant. The patch now looks correct to me (but I can't approve). > We bootstrapped the compiler ,look ok to us with minimal testing , > > Any floating point test-suite to test for the attached patch ? any > recommendations or inputs ? Running the GCC regression tests would be required since a bootstrap isn't useful for this kind of change. Assuming you use Linux, building and running GLIBC with the changed GCC would give additional test coverage as it tests all the math library functions. I don't know of any IEEE conformance testsuites in the GNU world, which is why I'm suggesting running some targeted and randomized tests. You could use the generic soft-float code in libgcc/soft-fp/adddf3.c to compare the outputs. >>> Index: libgcc/config/arm/ieee754-df.S >>> === >>> --- libgcc/config/arm/ieee754-df.S (revision 262850) >>> +++ libgcc/config/arm/ieee754-df.S (working copy) >>> @@ -203,6 +203,7 @@ >>> #endif >>> >>> @ Determine how to normalize the result. >>> + @ if result is denormal i.e (exp)=0,then don't normalise the result, Use a standard sentence here, eg. like: If exp is zero and the mantissa unnormalized, return a denormal. Wilco
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On Mon, Jul 23, 2018 at 12:09 PM, Umesh Kalappa
wrote:
> Hi Richard,
>
> We tested on the SP and yes the problem persist on the SP too and
> attached patch will fix the both SP and DP issues for the denormal
> resultant.
> We bootstrapped the compiler ,look ok to us with minimal testing ,
Have you run the full gcc testsuite as you should do if you want to
submit a patch and checked for no regressions ?
Ramana
>
> Any floating point test-suite to test for the attached patch ? any
> recommendations or inputs ?
>
> Thank you again
> ~Umesh
>
>
> On Mon, Jul 23, 2018 at 3:28 PM, Richard Earnshaw (lists)
> wrote:
>> So why is this only changing the double-precision implementation?
>> Surely, a problem like this will normally be common to both SP and DP
>> floating-point computations.
>>
>> R.
>>
>> On 23/07/18 08:46, Umesh Kalappa wrote:
>>> Thank you Wilco for the inputs and we agree that the fix break down
>>> for the case.
>>>
>>> Meanwhile ,attached patch will take care the inputs and we are testing
>>> the patch vigorously ,would you recommended any test-suite out there
>>> for the same ?
>>>
>>> Thank you
>>> ~Umesh
>>>
>>>
>>> On Fri, Jul 20, 2018 at 10:04 PM, Wilco Dijkstra
>>> wrote:
Umesh Kalappa wrote:
> We tried some of the normalisation numbers and the fix works and please
> could you help us with the input ,where if you see that fix breaks down.
Well try any set of inputs which require normalisation. You'll find these
no
longer get normalised and so will get incorrect results. Try basic cases
like
1.0 - 0.75 which I think will return 0.625...
A basic test would be to run old vs new on a large set of inputs to verify
there aren't any obvious differences.
Wilco
pr86512.patch
Index: libgcc/config/arm/ieee754-df.S
===
--- libgcc/config/arm/ieee754-df.S (revision 262850)
+++ libgcc/config/arm/ieee754-df.S (working copy)
@@ -203,6 +203,7 @@
#endif
@ Determine how to normalize the result.
+@ if result is denormal i.e (exp)=0,then don't normalise the result,
LSYM(Lad_p):
cmp xh, #0x0010
bcc LSYM(Lad_a)
@@ -235,6 +236,8 @@
@ Result must be shifted left and exponent adjusted.
LSYM(Lad_a):
+cmp r4,#0x0
+beq LSYM(Lad_e)
movsip, ip, lsl #1
adcsxl, xl, xl
adc xh, xh, xh
Index: gcc/testsuite/gcc.target/arm/pr86512.c
===
--- gcc/testsuite/gcc.target/arm/pr86512.c (nonexistent)
+++ gcc/testsuite/gcc.target/arm/pr86512.c (working copy)
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+/* { dg-options "-O0 -msoft-float" } */
+
+#include
+#include
+
+typedef union
+{
+double d;
+uint64_t i;
+} u;
+
+int main()
+{
+ u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult,
result;
+
+ smallesPositiveSubnormal.i = 1;
+
+ smallestPositiveNormal.i = 0x0010;
+ expectedResult.i = 0x000f;
+ result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
+
+ if (result.i != expectedResult.i)
+abort();
+
+ return 0;
+}
+
>>
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Hi Richard,
We tested on the SP and yes the problem persist on the SP too and
attached patch will fix the both SP and DP issues for the denormal
resultant.
We bootstrapped the compiler ,look ok to us with minimal testing ,
Any floating point test-suite to test for the attached patch ? any
recommendations or inputs ?
Thank you again
~Umesh
On Mon, Jul 23, 2018 at 3:28 PM, Richard Earnshaw (lists)
wrote:
> So why is this only changing the double-precision implementation?
> Surely, a problem like this will normally be common to both SP and DP
> floating-point computations.
>
> R.
>
> On 23/07/18 08:46, Umesh Kalappa wrote:
>> Thank you Wilco for the inputs and we agree that the fix break down
>> for the case.
>>
>> Meanwhile ,attached patch will take care the inputs and we are testing
>> the patch vigorously ,would you recommended any test-suite out there
>> for the same ?
>>
>> Thank you
>> ~Umesh
>>
>>
>> On Fri, Jul 20, 2018 at 10:04 PM, Wilco Dijkstra
>> wrote:
>>> Umesh Kalappa wrote:
>>>
We tried some of the normalisation numbers and the fix works and please
could you help us with the input ,where if you see that fix breaks down.
>>>
>>> Well try any set of inputs which require normalisation. You'll find these no
>>> longer get normalised and so will get incorrect results. Try basic cases
>>> like
>>> 1.0 - 0.75 which I think will return 0.625...
>>>
>>> A basic test would be to run old vs new on a large set of inputs to verify
>>> there aren't any obvious differences.
>>>
>>> Wilco
>>>
>>>
>>> pr86512.patch
>>>
>>>
>>> Index: libgcc/config/arm/ieee754-df.S
>>> ===
>>> --- libgcc/config/arm/ieee754-df.S (revision 262850)
>>> +++ libgcc/config/arm/ieee754-df.S (working copy)
>>> @@ -203,6 +203,7 @@
>>> #endif
>>>
>>> @ Determine how to normalize the result.
>>> +@ if result is denormal i.e (exp)=0,then don't normalise the result,
>>> LSYM(Lad_p):
>>> cmp xh, #0x0010
>>> bcc LSYM(Lad_a)
>>> @@ -235,6 +236,8 @@
>>>
>>> @ Result must be shifted left and exponent adjusted.
>>> LSYM(Lad_a):
>>> +cmp r4,#0x0
>>> +beq LSYM(Lad_e)
>>> movsip, ip, lsl #1
>>> adcsxl, xl, xl
>>> adc xh, xh, xh
>>> Index: gcc/testsuite/gcc.target/arm/pr86512.c
>>> ===
>>> --- gcc/testsuite/gcc.target/arm/pr86512.c (nonexistent)
>>> +++ gcc/testsuite/gcc.target/arm/pr86512.c (working copy)
>>> @@ -0,0 +1,28 @@
>>> +/* { dg-do run } */
>>> +/* { dg-options "-O0 -msoft-float" } */
>>> +
>>> +#include
>>> +#include
>>> +
>>> +typedef union
>>> +{
>>> +double d;
>>> +uint64_t i;
>>> +} u;
>>> +
>>> +int main()
>>> +{
>>> + u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult,
>>> result;
>>> +
>>> + smallesPositiveSubnormal.i = 1;
>>> +
>>> + smallestPositiveNormal.i = 0x0010;
>>> + expectedResult.i = 0x000f;
>>> + result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
>>> +
>>> + if (result.i != expectedResult.i)
>>> +abort();
>>> +
>>> + return 0;
>>> +}
>>> +
>
pr86512.patch
Description: Binary data
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
So why is this only changing the double-precision implementation?
Surely, a problem like this will normally be common to both SP and DP
floating-point computations.
R.
On 23/07/18 08:46, Umesh Kalappa wrote:
> Thank you Wilco for the inputs and we agree that the fix break down
> for the case.
>
> Meanwhile ,attached patch will take care the inputs and we are testing
> the patch vigorously ,would you recommended any test-suite out there
> for the same ?
>
> Thank you
> ~Umesh
>
>
> On Fri, Jul 20, 2018 at 10:04 PM, Wilco Dijkstra
> wrote:
>> Umesh Kalappa wrote:
>>
>>> We tried some of the normalisation numbers and the fix works and please
>>> could you help us with the input ,where if you see that fix breaks down.
>>
>> Well try any set of inputs which require normalisation. You'll find these no
>> longer get normalised and so will get incorrect results. Try basic cases like
>> 1.0 - 0.75 which I think will return 0.625...
>>
>> A basic test would be to run old vs new on a large set of inputs to verify
>> there aren't any obvious differences.
>>
>> Wilco
>>
>>
>> pr86512.patch
>>
>>
>> Index: libgcc/config/arm/ieee754-df.S
>> ===
>> --- libgcc/config/arm/ieee754-df.S (revision 262850)
>> +++ libgcc/config/arm/ieee754-df.S (working copy)
>> @@ -203,6 +203,7 @@
>> #endif
>>
>> @ Determine how to normalize the result.
>> +@ if result is denormal i.e (exp)=0,then don't normalise the result,
>> LSYM(Lad_p):
>> cmp xh, #0x0010
>> bcc LSYM(Lad_a)
>> @@ -235,6 +236,8 @@
>>
>> @ Result must be shifted left and exponent adjusted.
>> LSYM(Lad_a):
>> +cmp r4,#0x0
>> +beq LSYM(Lad_e)
>> movsip, ip, lsl #1
>> adcsxl, xl, xl
>> adc xh, xh, xh
>> Index: gcc/testsuite/gcc.target/arm/pr86512.c
>> ===
>> --- gcc/testsuite/gcc.target/arm/pr86512.c (nonexistent)
>> +++ gcc/testsuite/gcc.target/arm/pr86512.c (working copy)
>> @@ -0,0 +1,28 @@
>> +/* { dg-do run } */
>> +/* { dg-options "-O0 -msoft-float" } */
>> +
>> +#include
>> +#include
>> +
>> +typedef union
>> +{
>> +double d;
>> +uint64_t i;
>> +} u;
>> +
>> +int main()
>> +{
>> + u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult,
>> result;
>> +
>> + smallesPositiveSubnormal.i = 1;
>> +
>> + smallestPositiveNormal.i = 0x0010;
>> + expectedResult.i = 0x000f;
>> + result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
>> +
>> + if (result.i != expectedResult.i)
>> +abort();
>> +
>> + return 0;
>> +}
>> +
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Thank you Wilco for the inputs and we agree that the fix break down for the case. Meanwhile ,attached patch will take care the inputs and we are testing the patch vigorously ,would you recommended any test-suite out there for the same ? Thank you ~Umesh On Fri, Jul 20, 2018 at 10:04 PM, Wilco Dijkstra wrote: > Umesh Kalappa wrote: > >> We tried some of the normalisation numbers and the fix works and please >> could you help us with the input ,where if you see that fix breaks down. > > Well try any set of inputs which require normalisation. You'll find these no > longer get normalised and so will get incorrect results. Try basic cases like > 1.0 - 0.75 which I think will return 0.625... > > A basic test would be to run old vs new on a large set of inputs to verify > there aren't any obvious differences. > > Wilco > pr86512.patch Description: Binary data
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Umesh Kalappa wrote: > We tried some of the normalisation numbers and the fix works and please > could you help us with the input ,where if you see that fix breaks down. Well try any set of inputs which require normalisation. You'll find these no longer get normalised and so will get incorrect results. Try basic cases like 1.0 - 0.75 which I think will return 0.625... A basic test would be to run old vs new on a large set of inputs to verify there aren't any obvious differences. Wilco
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Thank you all for your comments . Wilco, We tried some of the normalisation numbers and the fix works and please could you help us with the input ,where if you see that fix breaks down. Thank you again ~Umesh On Fri, Jul 20, 2018, 7:07 PM Wilco Dijkstra wrote: > Hi Umesh, > > Looking at your patch, this would break all results which need to be > normalized. > > > Index: libgcc/config/arm/ieee754-df.S > === > --- libgcc/config/arm/ieee754-df.S (revision 262850) > +++ libgcc/config/arm/ieee754-df.S (working copy) > @@ -203,8 +203,11 @@ > #endif > > @ Determine how to normalize the result. > + @ if result is denormal i.e (exp)=0,then don't normalise the > result, > LSYM(Lad_p): > cmp xh, #0x0010 > + blt LSYM(Lad_e) > + cmp xh, #0x0010 > bcc LSYM(Lad_a) > cmp xh, #0x0020 > bcc LSYM(Lad_e) > > It seems Lad_a doesn't correctly handle the case where the result is a > denormal. For this case > the result is correct so nothing else needs to be done. This requires an > explicit test that the > exponent is zero - other cases still need to be renormalized as usual. > This code looks overly > complex so any change will require extensive testing of all the corner > cases. > > Wilco >
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Hi Umesh, Looking at your patch, this would break all results which need to be normalized. Index: libgcc/config/arm/ieee754-df.S === --- libgcc/config/arm/ieee754-df.S (revision 262850) +++ libgcc/config/arm/ieee754-df.S (working copy) @@ -203,8 +203,11 @@ #endif @ Determine how to normalize the result. + @ if result is denormal i.e (exp)=0,then don't normalise the result, LSYM(Lad_p): cmp xh, #0x0010 + blt LSYM(Lad_e) + cmp xh, #0x0010 bcc LSYM(Lad_a) cmp xh, #0x0020 bcc LSYM(Lad_e) It seems Lad_a doesn't correctly handle the case where the result is a denormal. For this case the result is correct so nothing else needs to be done. This requires an explicit test that the exponent is zero - other cases still need to be renormalized as usual. This code looks overly complex so any change will require extensive testing of all the corner cases. Wilco
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
On 18/07/18 13:31, Umesh Kalappa wrote:
Hi Nagy/Ramana,
Please help us to review the attached patch and do let me know your comments .
No regress in the gcc.target suite for arm target.
have you submitted a copyright assignment form yet?
https://gcc.gnu.org/contribute.html
Index: gcc/testsuite/ChangeLog
===
--- gcc/testsuite/ChangeLog (revision 262850)
+++ gcc/testsuite/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2018-07-18 Umesh Kalappa
+
+ PR libgcc/86512
+ * gcc.target/arm/pr86512.c :New test.
+
normally changelog should be part of the mail message and
not included in the diff as it will conflict with other changes
and the whitespace formatting around : and before the email
address is wrong.
Index: gcc/testsuite/gcc.target/arm/pr86512.c
===
--- gcc/testsuite/gcc.target/arm/pr86512.c (nonexistent)
+++ gcc/testsuite/gcc.target/arm/pr86512.c (working copy)
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-options "-O0 -msoft-float" } */
+
+#include
+#include
+#define PRIx64"llx"
this macro is not needed.
+
+typedef union
+{
+double d;
+uint64_t i;
+} u;
+
+int main()
+{
+ u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+
+ smallestPositiveNormal.i = 0x0010;
+ expectedResult.i = 0x000f;
+ result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
+
+
+ if (result.i != expectedResult.i)
+ abort();
+
+ return 0;
+}
+
Index: libgcc/ChangeLog
===
--- libgcc/ChangeLog(revision 262850)
+++ libgcc/ChangeLog(working copy)
@@ -1,3 +1,9 @@
+2018-07-18 Umesh Kalappa
+
+ PR libgcc/86512
+ * config/arm/ieee754-df.S :Don't normalise the denormal result.
+
same as above.
--- libgcc/config/arm/ieee754-df.S (revision 262850)
+++ libgcc/config/arm/ieee754-df.S (working copy)
@@ -203,8 +203,11 @@
#endif
@ Determine how to normalize the result.
+ @ if result is denormal i.e (exp)=0,then don't normalise the result,
LSYM(Lad_p):
cmp xh, #0x0010
+ blt LSYM(Lad_e)
+ cmp xh, #0x0010
bcc LSYM(Lad_a)
i think you don't need to retest the same thing,
blt won't clobber the condition codes.
as for the technical contents, sorry i have not
reviewed it in detail, the blt might not be the
right check since it's signed compare..
(but i'm not even sure why is there an assembly
implementation for this, generic c code should do
a good job.)
cmp xh, #0x0020
bcc LSYM(Lad_e)
Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Reminder !!! ~Umesh On Wed, Jul 18, 2018 at 6:01 PM, Umesh Kalappa wrote: > Hi Nagy/Ramana, > > Please help us to review the attached patch and do let me know your comments . > > No regress in the gcc.target suite for arm target. > > Thank you > ~Umesh > > On Tue, Jul 17, 2018 at 4:01 PM, Umesh Kalappa > wrote: >> Will do, thanks. >> Thanks >> >> On Tue, Jul 17, 2018, 3:24 PM Ramana Radhakrishnan >> wrote: >>> >>> On Tue, Jul 17, 2018 at 10:41 AM, Umesh Kalappa >>> wrote: >>> > Hi Nagy, >>> > >>> > Please help us with your comments on the attached patch for the issue >>> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86512) >>> > >>> > Thank you and waiting for your inputs on the same. >>> >>> >>> Patches should be sent to gcc-patches@gcc.gnu.org with a clear >>> description of what the patch hopes to >>> achieve and why this is correct, how was it tested and if a regression >>> test needs to be added - add one please. >>> Please read https://gcc.gnu.org/contribute.html before sending a patch. >>> >>> This is the wrong list to send patches to. >>> >>> regards >>> Ramana >>> > ~Umesh >>> > >>> > On Fri, Jul 13, 2018 at 1:22 PM, Umesh Kalappa >>> > wrote: >>> >> Thank you and issue raised at >>> >> gcc-patches@gcc.gnu.org >>> >> >>> >> ~Umesh >>> >> >>> >> On Thu, Jul 12, 2018 at 9:33 PM, Szabolcs Nagy >>> >> wrote: >>> >>> On 12/07/18 16:20, Umesh Kalappa wrote: >>> >>> Hi everyone, >>> >>> we have our source base ,that was compiled for armv7 on gcc8.1 with >>> soft-float and for following input >>> >>> a=0x0010 >>> b=0x0001 >>> >>> result = a - b ; >>> >>> we are getting the result as "0x000e" and with >>> -mhard-float (disabled the flush to zero mode ) we are getting the >>> result as ""0x000f" as expected. >>> >>> >>> >>> >>> please submit it as a bug report to bugzilla >>> >>> >>> >>> >>> while debugging the soft-float code,we see that ,the compiler calls >>> the intrinsic "__aeabi_dsub" with arm calling conventions i.e passing >>> "a" in r0 and r1 registers and respectively for "b". >>> >>> we are investigating the routine "__aeabi_dsub" that comes from >>> libgcc >>> for incorrect result and meanwhile we would like to know that >>> >>> a)do libgcc routines/intrinsic for float operations support or >>> consider the subnormal values ? ,if so how we can enable the same. >>> >>> Thank you >>> ~Umesh >>> >>> >>>
[Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
Hi Nagy/Ramana, Please help us to review the attached patch and do let me know your comments . No regress in the gcc.target suite for arm target. Thank you ~Umesh On Tue, Jul 17, 2018 at 4:01 PM, Umesh Kalappa wrote: > Will do, thanks. > Thanks > > On Tue, Jul 17, 2018, 3:24 PM Ramana Radhakrishnan > wrote: >> >> On Tue, Jul 17, 2018 at 10:41 AM, Umesh Kalappa >> wrote: >> > Hi Nagy, >> > >> > Please help us with your comments on the attached patch for the issue >> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86512) >> > >> > Thank you and waiting for your inputs on the same. >> >> >> Patches should be sent to gcc-patches@gcc.gnu.org with a clear >> description of what the patch hopes to >> achieve and why this is correct, how was it tested and if a regression >> test needs to be added - add one please. >> Please read https://gcc.gnu.org/contribute.html before sending a patch. >> >> This is the wrong list to send patches to. >> >> regards >> Ramana >> > ~Umesh >> > >> > On Fri, Jul 13, 2018 at 1:22 PM, Umesh Kalappa >> > wrote: >> >> Thank you and issue raised at >> >> gcc-patches@gcc.gnu.org >> >> >> >> ~Umesh >> >> >> >> On Thu, Jul 12, 2018 at 9:33 PM, Szabolcs Nagy >> >> wrote: >> >>> On 12/07/18 16:20, Umesh Kalappa wrote: >> >> Hi everyone, >> >> we have our source base ,that was compiled for armv7 on gcc8.1 with >> soft-float and for following input >> >> a=0x0010 >> b=0x0001 >> >> result = a - b ; >> >> we are getting the result as "0x000e" and with >> -mhard-float (disabled the flush to zero mode ) we are getting the >> result as ""0x000f" as expected. >> >> >>> >> >>> please submit it as a bug report to bugzilla >> >>> >> >>> >> while debugging the soft-float code,we see that ,the compiler calls >> the intrinsic "__aeabi_dsub" with arm calling conventions i.e passing >> "a" in r0 and r1 registers and respectively for "b". >> >> we are investigating the routine "__aeabi_dsub" that comes from >> libgcc >> for incorrect result and meanwhile we would like to know that >> >> a)do libgcc routines/intrinsic for float operations support or >> consider the subnormal values ? ,if so how we can enable the same. >> >> Thank you >> ~Umesh >> >> >>> pr86512.patch Description: Binary data
