* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
@ 2018-07-20 13:38 Wilco Dijkstra
2018-07-20 16:00 ` Umesh Kalappa
0 siblings, 1 reply; 20+ messages in thread
From: Wilco Dijkstra @ 2018-07-20 13:38 UTC (permalink / raw)
To: umesh.kalappa0, GCC Patches; +Cc: nd, Szabolcs Nagy, Ramana Radhakrishnan
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, #0x00100000
+ blt LSYM(Lad_e)
+ cmp xh, #0x00100000
bcc LSYM(Lad_a)
cmp xh, #0x00200000
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-20 13:38 [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics Wilco Dijkstra
@ 2018-07-20 16:00 ` Umesh Kalappa
2018-07-20 16:34 ` Wilco Dijkstra
0 siblings, 1 reply; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-20 16:00 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
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 <Wilco.Dijkstra@arm.com> 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, #0x00100000
> + blt LSYM(Lad_e)
> + cmp xh, #0x00100000
> bcc LSYM(Lad_a)
> cmp xh, #0x00200000
> 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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-20 16:00 ` Umesh Kalappa
@ 2018-07-20 16:34 ` Wilco Dijkstra
2018-07-23 7:46 ` Umesh Kalappa
0 siblings, 1 reply; 20+ messages in thread
From: Wilco Dijkstra @ 2018-07-20 16:34 UTC (permalink / raw)
To: Umesh Kalappa; +Cc: gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-20 16:34 ` Wilco Dijkstra
@ 2018-07-23 7:46 ` Umesh Kalappa
2018-07-23 9:58 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-23 7:46 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
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 <Wilco.Dijkstra@arm.com> 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
>
[-- Attachment #2: pr86512.patch --]
[-- Type: application/octet-stream, Size: 1370 bytes --]
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, #0x00100000
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)
movs ip, ip, lsl #1
adcs xl, 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<stdlib.h>
+#include<stdint.h>
+
+typedef union
+{
+ double d;
+ uint64_t i;
+} u;
+
+int main()
+{
+ u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+
+ smallestPositiveNormal.i = 0x0010000000000000;
+ expectedResult.i = 0x000fffffffffffff;
+ result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
+
+ if (result.i != expectedResult.i)
+ abort();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-23 7:46 ` Umesh Kalappa
@ 2018-07-23 9:58 ` Richard Earnshaw (lists)
2018-07-23 11:09 ` Umesh Kalappa
0 siblings, 1 reply; 20+ messages in thread
From: Richard Earnshaw (lists) @ 2018-07-23 9:58 UTC (permalink / raw)
To: Umesh Kalappa, Wilco Dijkstra
Cc: gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
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 <Wilco.Dijkstra@arm.com> 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, #0x00100000
>> 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)
>> movs ip, ip, lsl #1
>> adcs xl, 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<stdlib.h>
>> +#include<stdint.h>
>> +
>> +typedef union
>> +{
>> + double d;
>> + uint64_t i;
>> +} u;
>> +
>> +int main()
>> +{
>> + u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
>> +
>> + smallesPositiveSubnormal.i = 1;
>> +
>> + smallestPositiveNormal.i = 0x0010000000000000;
>> + expectedResult.i = 0x000fffffffffffff;
>> + result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
>> +
>> + if (result.i != expectedResult.i)
>> + abort();
>> +
>> + return 0;
>> +}
>> +
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-23 9:58 ` Richard Earnshaw (lists)
@ 2018-07-23 11:09 ` Umesh Kalappa
2018-07-23 11:42 ` Ramana Radhakrishnan
2018-07-23 11:54 ` Wilco Dijkstra
0 siblings, 2 replies; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-23 11:09 UTC (permalink / raw)
To: Richard Earnshaw (lists)
Cc: Wilco Dijkstra, gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 3251 bytes --]
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)
<Richard.Earnshaw@arm.com> 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 <Wilco.Dijkstra@arm.com> 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, #0x00100000
>>> 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)
>>> movs ip, ip, lsl #1
>>> adcs xl, 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<stdlib.h>
>>> +#include<stdint.h>
>>> +
>>> +typedef union
>>> +{
>>> + double d;
>>> + uint64_t i;
>>> +} u;
>>> +
>>> +int main()
>>> +{
>>> + u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
>>> +
>>> + smallesPositiveSubnormal.i = 1;
>>> +
>>> + smallestPositiveNormal.i = 0x0010000000000000;
>>> + expectedResult.i = 0x000fffffffffffff;
>>> + result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
>>> +
>>> + if (result.i != expectedResult.i)
>>> + abort();
>>> +
>>> + return 0;
>>> +}
>>> +
>
[-- Attachment #2: pr86512.patch --]
[-- Type: application/octet-stream, Size: 2236 bytes --]
Index: libgcc/config/arm/ieee754-sf.S
===================================================================
--- libgcc/config/arm/ieee754-sf.S (revision 262850)
+++ libgcc/config/arm/ieee754-sf.S (working copy)
@@ -165,7 +165,10 @@
RET
@ Result must be shifted left and exponent adjusted.
+ @ if result is denormal i.e (exp)=0,then don't normalise the result,
LSYM(Lad_a):
+ cmp r2,#0x0
+ beq LSYM(Lad_e)
movs r1, r1, lsl #1
adc r0, r0, r0
tst r0, #0x00800000
Index: libgcc/config/arm/ieee754-df.S
===================================================================
--- libgcc/config/arm/ieee754-df.S (revision 262850)
+++ libgcc/config/arm/ieee754-df.S (working copy)
@@ -234,7 +234,10 @@
RETLDM "r4, r5"
@ Result must be shifted left and exponent adjusted.
+ @ if result is denormal i.e (exp)=0,then don't normalise the result,
LSYM(Lad_a):
+ cmp r4,#0x0
+ beq LSYM(Lad_e)
movs ip, ip, lsl #1
adcs xl, 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,59 @@
+/* { dg-do run } */
+/* { dg-options "-O0 -msoft-float" } */
+
+#include<stdlib.h>
+#include<stdint.h>
+
+typedef union
+{
+ float f;
+ int i;
+} f;
+
+typedef union
+{
+ double d;
+ uint64_t i;
+}d ;
+
+int testFloat()
+{
+ f smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+ smallestPositiveNormal.i =0x00800000;
+ expectedResult.i = 0x007fffff;
+
+ result.f = smallestPositiveNormal.f - smallesPositiveSubnormal.f;
+
+ if (result.i != expectedResult.i)
+ return 1;
+
+ return 0;
+}
+
+int testDouble()
+{
+ d smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+ smallestPositiveNormal.i = 0x0010000000000000;
+ expectedResult.i = 0x000fffffffffffff;
+
+ result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
+
+ if (result.i != expectedResult.i)
+ return 1;
+
+ return 0;
+}
+
+int main()
+{
+
+ if (testFloat() || testDouble())
+ abort();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-23 11:09 ` Umesh Kalappa
@ 2018-07-23 11:42 ` Ramana Radhakrishnan
2018-07-23 11:54 ` Wilco Dijkstra
1 sibling, 0 replies; 20+ messages in thread
From: Ramana Radhakrishnan @ 2018-07-23 11:42 UTC (permalink / raw)
To: Umesh Kalappa
Cc: Richard Earnshaw (lists),
Wilco Dijkstra, gcc-patches, nd, Szabolcs Nagy,
Ramana Radhakrishnan
On Mon, Jul 23, 2018 at 12:09 PM, Umesh Kalappa
<umesh.kalappa0@gmail.com> 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)
> <Richard.Earnshaw@arm.com> 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 <Wilco.Dijkstra@arm.com> 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, #0x00100000
>>>> 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)
>>>> movs ip, ip, lsl #1
>>>> adcs xl, 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<stdlib.h>
>>>> +#include<stdint.h>
>>>> +
>>>> +typedef union
>>>> +{
>>>> + double d;
>>>> + uint64_t i;
>>>> +} u;
>>>> +
>>>> +int main()
>>>> +{
>>>> + u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
>>>> +
>>>> + smallesPositiveSubnormal.i = 1;
>>>> +
>>>> + smallestPositiveNormal.i = 0x0010000000000000;
>>>> + expectedResult.i = 0x000fffffffffffff;
>>>> + result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
>>>> +
>>>> + if (result.i != expectedResult.i)
>>>> + abort();
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-23 11:09 ` Umesh Kalappa
2018-07-23 11:42 ` Ramana Radhakrishnan
@ 2018-07-23 11:54 ` Wilco Dijkstra
2018-07-24 8:39 ` Umesh Kalappa
1 sibling, 1 reply; 20+ messages in thread
From: Wilco Dijkstra @ 2018-07-23 11:54 UTC (permalink / raw)
To: Umesh Kalappa, Richard Earnshaw
Cc: gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-23 11:54 ` Wilco Dijkstra
@ 2018-07-24 8:39 ` Umesh Kalappa
2018-07-25 17:04 ` Umesh Kalappa
0 siblings, 1 reply; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-24 8:39 UTC (permalink / raw)
To: Wilco Dijkstra
Cc: Richard Earnshaw, gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]
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 <umesh.kalappa0@gmail.com>
+
+ 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 <umesh.kalappa0@gmail.com>
+
+ PR libgcc/86512
+ * gcc.target/arm/pr86512.c :New test.
+
On Mon, Jul 23, 2018 at 5:24 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> 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
>
[-- Attachment #2: pr86512.patch --]
[-- Type: application/octet-stream, Size: 2232 bytes --]
Index: libgcc/config/arm/ieee754-sf.S
===================================================================
--- libgcc/config/arm/ieee754-sf.S (revision 262850)
+++ libgcc/config/arm/ieee754-sf.S (working copy)
@@ -165,7 +165,10 @@
RET
@ Result must be shifted left and exponent adjusted.
+ @ If exp is zero and the mantissa unnormalized, return a denormal.
LSYM(Lad_a):
+ cmp r2,#0x0
+ beq LSYM(Lad_e)
movs r1, r1, lsl #1
adc r0, r0, r0
tst r0, #0x00800000
Index: libgcc/config/arm/ieee754-df.S
===================================================================
--- libgcc/config/arm/ieee754-df.S (revision 262850)
+++ libgcc/config/arm/ieee754-df.S (working copy)
@@ -234,7 +234,10 @@
RETLDM "r4, r5"
@ Result must be shifted left and exponent adjusted.
+ @ If exp is zero and the mantissa unnormalized, return a denormal.
LSYM(Lad_a):
+ cmp r4,#0x0
+ beq LSYM(Lad_e)
movs ip, ip, lsl #1
adcs xl, 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,59 @@
+/* { dg-do run } */
+/* { dg-options "-O0 -msoft-float" } */
+
+#include<stdlib.h>
+#include<stdint.h>
+
+typedef union
+{
+ float f;
+ int i;
+} f;
+
+typedef union
+{
+ double d;
+ uint64_t i;
+}d ;
+
+int testFloat()
+{
+ f smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+ smallestPositiveNormal.i =0x00800000;
+ expectedResult.i = 0x007fffff;
+
+ result.f = smallestPositiveNormal.f - smallesPositiveSubnormal.f;
+
+ if (result.i != expectedResult.i)
+ return 1;
+
+ return 0;
+}
+
+int testDouble()
+{
+ d smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+ smallestPositiveNormal.i = 0x0010000000000000;
+ expectedResult.i = 0x000fffffffffffff;
+
+ result.d = smallestPositiveNormal.d - smallesPositiveSubnormal.d;
+
+ if (result.i != expectedResult.i)
+ return 1;
+
+ return 0;
+}
+
+int main()
+{
+
+ if (testFloat() || testDouble())
+ abort();
+
+ return 0;
+}
+
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-24 8:39 ` Umesh Kalappa
@ 2018-07-25 17:04 ` Umesh Kalappa
2018-07-26 16:36 ` Nicolas Pitre
0 siblings, 1 reply; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-25 17:04 UTC (permalink / raw)
To: Wilco Dijkstra
Cc: Richard Earnshaw (lists),
gcc-patches, nd, Szabolcs Nagy, Ramana Radhakrishnan
Hi,
Any more suggestions or comments on the patch ?
Thank you
~Umesh
On Tue, Jul 24, 2018, 2:08 PM Umesh Kalappa <umesh.kalappa0@gmail.com>
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 <umesh.kalappa0@gmail.com>
> +
> + 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 <umesh.kalappa0@gmail.com>
> +
> + PR libgcc/86512
> + * gcc.target/arm/pr86512.c :New test.
> +
>
> On Mon, Jul 23, 2018 at 5:24 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com>
> 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
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-25 17:04 ` Umesh Kalappa
@ 2018-07-26 16:36 ` Nicolas Pitre
2018-07-27 13:32 ` Wilco Dijkstra
0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2018-07-26 16:36 UTC (permalink / raw)
To: gcc-patches, Umesh Kalappa
Cc: Wilco Dijkstra, Richard Earnshaw, nd, Szabolcs Nagy,
Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
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.
[-- Attachment #2: Type: text/plain, Size: 2262 bytes --]
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 <nico@fluxnic.net>
+
+ * 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 <jrtc27@jrtc27.com>
* 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 (nico@cam.org)
+ Contributed by Nicolas Pitre (nico@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):
movs ip, ip, lsl #1
adcs xl, xl, xl
adc xh, xh, xh
- tst xh, #0x00100000
- sub r4, r4, #1
- bne LSYM(Lad_e)
+ subs r4, r4, #1
+ do_it hs
+ tsths xh, #0x00100000
+ 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 (nico@cam.org)
+ Contributed by Nicolas Pitre (nico@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):
movs r1, r1, lsl #1
adc r0, r0, r0
- tst r0, #0x00800000
- sub r2, r2, #1
- bne LSYM(Lad_e)
+ subs r2, r2, #1
+ do_it hs
+ tsths r0, #0x00800000
+ bhi LSYM(Lad_e)
@ No rounding necessary since r1 will always be 0 at this point.
LSYM(Lad_l):
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-26 16:36 ` Nicolas Pitre
@ 2018-07-27 13:32 ` Wilco Dijkstra
2018-07-27 15:50 ` Nicolas Pitre
0 siblings, 1 reply; 20+ messages in thread
From: Wilco Dijkstra @ 2018-07-27 13:32 UTC (permalink / raw)
To: Nicolas Pitre, gcc-patches, Umesh Kalappa
Cc: Richard Earnshaw, nd, Szabolcs Nagy, Ramana Radhakrishnan
Hi Nicolas,
I think your patch doesn't quite work as expected:
@@ -238,9 +238,10 @@ LSYM(Lad_a):
movs ip, ip, lsl #1
adcs xl, xl, xl
adc xh, xh, xh
- tst xh, #0x00100000
- sub r4, r4, #1
- bne LSYM(Lad_e)
+ subs r4, r4, #1
+ do_it hs
+ tsths xh, #0x00100000
+ 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-27 13:32 ` Wilco Dijkstra
@ 2018-07-27 15:50 ` Nicolas Pitre
2018-07-27 16:01 ` Nicolas Pitre
2018-07-27 16:05 ` Wilco Dijkstra
0 siblings, 2 replies; 20+ messages in thread
From: Nicolas Pitre @ 2018-07-27 15:50 UTC (permalink / raw)
To: Wilco Dijkstra
Cc: gcc-patches, Umesh Kalappa, Richard Earnshaw, nd, Szabolcs Nagy,
Ramana Radhakrishnan
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):
> movs ip, ip, lsl #1
> adcs xl, xl, xl
> adc xh, xh, xh
> - tst xh, #0x00100000
> - sub r4, r4, #1
> - bne LSYM(Lad_e)
> + subs r4, r4, #1
> + do_it hs
> + tsths xh, #0x00100000
> + 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-27 15:50 ` Nicolas Pitre
@ 2018-07-27 16:01 ` Nicolas Pitre
2018-07-27 16:05 ` Wilco Dijkstra
1 sibling, 0 replies; 20+ messages in thread
From: Nicolas Pitre @ 2018-07-27 16:01 UTC (permalink / raw)
To: Wilco Dijkstra
Cc: gcc-patches, Umesh Kalappa, Richard Earnshaw, nd, Szabolcs Nagy,
Ramana Radhakrishnan
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):
> > movs ip, ip, lsl #1
> > adcs xl, xl, xl
> > adc xh, xh, xh
> > - tst xh, #0x00100000
> > - sub r4, r4, #1
> > - bne LSYM(Lad_e)
> > + subs r4, r4, #1
> > + do_it hs
> > + tsths xh, #0x00100000
> > + 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-27 15:50 ` Nicolas Pitre
2018-07-27 16:01 ` Nicolas Pitre
@ 2018-07-27 16:05 ` Wilco Dijkstra
2018-07-27 16:45 ` Nicolas Pitre
1 sibling, 1 reply; 20+ messages in thread
From: Wilco Dijkstra @ 2018-07-27 16:05 UTC (permalink / raw)
To: Nicolas Pitre
Cc: gcc-patches, Umesh Kalappa, Richard Earnshaw, nd, Szabolcs Nagy,
Ramana Radhakrishnan
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-27 16:05 ` Wilco Dijkstra
@ 2018-07-27 16:45 ` Nicolas Pitre
2018-08-02 16:50 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2018-07-27 16:45 UTC (permalink / raw)
To: Wilco Dijkstra
Cc: gcc-patches, Umesh Kalappa, Richard Earnshaw, nd, Szabolcs Nagy,
Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 790 bytes --]
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.
[-- Attachment #2: Type: text/plain, Size: 2269 bytes --]
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 <nico@fluxnic.net>
+
+ * 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 <jrtc27@jrtc27.com>
* 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 (nico@cam.org)
+ Contributed by Nicolas Pitre (nico@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):
movs ip, ip, lsl #1
adcs xl, xl, xl
adc xh, xh, xh
- tst xh, #0x00100000
- sub r4, r4, #1
- bne LSYM(Lad_e)
+ subs r4, r4, #1
+ do_it hs
+ cmphs xh, #0x00100000
+ 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 (nico@cam.org)
+ Contributed by Nicolas Pitre (nico@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):
movs r1, r1, lsl #1
adc r0, r0, r0
- tst r0, #0x00800000
- sub r2, r2, #1
- bne LSYM(Lad_e)
-
+ subs r2, r2, #1
+ do_it hs
+ cmphs r0, #0x00800000
+ bhs LSYM(Lad_e)
+
@ No rounding necessary since r1 will always be 0 at this point.
LSYM(Lad_l):
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-27 16:45 ` Nicolas Pitre
@ 2018-08-02 16:50 ` Richard Earnshaw (lists)
0 siblings, 0 replies; 20+ messages in thread
From: Richard Earnshaw (lists) @ 2018-08-02 16:50 UTC (permalink / raw)
To: Nicolas Pitre, Wilco Dijkstra
Cc: gcc-patches, Umesh Kalappa, nd, Szabolcs Nagy, Ramana Radhakrishnan
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 <nico@fluxnic.net>
> +
> + * 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 <jrtc27@jrtc27.com>
>
> * 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 (nico@cam.org)
> + Contributed by Nicolas Pitre (nico@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):
> movs ip, ip, lsl #1
> adcs xl, xl, xl
> adc xh, xh, xh
> - tst xh, #0x00100000
> - sub r4, r4, #1
> - bne LSYM(Lad_e)
> + subs r4, r4, #1
> + do_it hs
> + cmphs xh, #0x00100000
> + 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 (nico@cam.org)
> + Contributed by Nicolas Pitre (nico@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):
> movs r1, r1, lsl #1
> adc r0, r0, r0
> - tst r0, #0x00800000
> - sub r2, r2, #1
> - bne LSYM(Lad_e)
> -
> + subs r2, r2, #1
> + do_it hs
> + cmphs r0, #0x00800000
> + bhs LSYM(Lad_e)
> +
> @ No rounding necessary since r1 will always be 0 at this point.
> LSYM(Lad_l):
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-18 12:31 Umesh Kalappa
2018-07-20 12:05 ` Umesh Kalappa
@ 2018-07-20 13:30 ` Szabolcs Nagy
1 sibling, 0 replies; 20+ messages in thread
From: Szabolcs Nagy @ 2018-07-20 13:30 UTC (permalink / raw)
To: Umesh Kalappa, gcc-patches; +Cc: nd, Ramana Radhakrishnan
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<umesh.kalappa0@gmail.com>
> +
> + 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<stdlib.h>
> +#include<stdint.h>
> +#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 = 0x0010000000000000;
> + expectedResult.i = 0x000fffffffffffff;
> + 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<umesh.kalappa0@gmail.com>
> +
> + 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, #0x00100000
> + blt LSYM(Lad_e)
> + cmp xh, #0x00100000
> 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, #0x00200000
> bcc LSYM(Lad_e)
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
2018-07-18 12:31 Umesh Kalappa
@ 2018-07-20 12:05 ` Umesh Kalappa
2018-07-20 13:30 ` Szabolcs Nagy
1 sibling, 0 replies; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-20 12:05 UTC (permalink / raw)
To: gcc-patches; +Cc: Szabolcs Nagy, nd, Ramana Radhakrishnan
Reminder !!!
~Umesh
On Wed, Jul 18, 2018 at 6:01 PM, Umesh Kalappa <umesh.kalappa0@gmail.com> 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 <umesh.kalappa0@gmail.com> wrote:
>> Will do, thanks.
>> Thanks
>>
>> On Tue, Jul 17, 2018, 3:24 PM Ramana Radhakrishnan
>> <ramana.gcc@googlemail.com> wrote:
>>>
>>> On Tue, Jul 17, 2018 at 10:41 AM, Umesh Kalappa
>>> <umesh.kalappa0@gmail.com> 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
>>> > <umesh.kalappa0@gmail.com> wrote:
>>> >> Thank you and issue raised at
>>> >> gcc-patches@gcc.gnu.org
>>> >>
>>> >> ~Umesh
>>> >>
>>> >> On Thu, Jul 12, 2018 at 9:33 PM, Szabolcs Nagy <szabolcs.nagy@arm.com>
>>> >> 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=0x0010000000000000
>>> >>>> b=0x0000000000000001
>>> >>>>
>>> >>>> result = a - b ;
>>> >>>>
>>> >>>> we are getting the result as "0x000ffffffffffffe" and with
>>> >>>> -mhard-float (disabled the flush to zero mode ) we are getting the
>>> >>>> result as ""0x000fffffffffffff" 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
>>> >>>>
>>> >>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics
@ 2018-07-18 12:31 Umesh Kalappa
2018-07-20 12:05 ` Umesh Kalappa
2018-07-20 13:30 ` Szabolcs Nagy
0 siblings, 2 replies; 20+ messages in thread
From: Umesh Kalappa @ 2018-07-18 12:31 UTC (permalink / raw)
To: gcc-patches; +Cc: Szabolcs Nagy, nd, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]
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 <umesh.kalappa0@gmail.com> wrote:
> Will do, thanks.
> Thanks
>
> On Tue, Jul 17, 2018, 3:24 PM Ramana Radhakrishnan
> <ramana.gcc@googlemail.com> wrote:
>>
>> On Tue, Jul 17, 2018 at 10:41 AM, Umesh Kalappa
>> <umesh.kalappa0@gmail.com> 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
>> > <umesh.kalappa0@gmail.com> wrote:
>> >> Thank you and issue raised at
>> >> gcc-patches@gcc.gnu.org
>> >>
>> >> ~Umesh
>> >>
>> >> On Thu, Jul 12, 2018 at 9:33 PM, Szabolcs Nagy <szabolcs.nagy@arm.com>
>> >> 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=0x0010000000000000
>> >>>> b=0x0000000000000001
>> >>>>
>> >>>> result = a - b ;
>> >>>>
>> >>>> we are getting the result as "0x000ffffffffffffe" and with
>> >>>> -mhard-float (disabled the flush to zero mode ) we are getting the
>> >>>> result as ""0x000fffffffffffff" 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
>> >>>>
>> >>>
[-- Attachment #2: pr86512.patch --]
[-- Type: application/octet-stream, Size: 2084 bytes --]
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog (revision 262850)
+++ gcc/testsuite/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2018-07-18 Umesh Kalappa <umesh.kalappa0@gmail.com>
+
+ PR libgcc/86512
+ * gcc.target/arm/pr86512.c :New test.
+
2018-07-18 Richard Biener <rguenther@suse.de>
PR debug/86523
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<stdlib.h>
+#include<stdint.h>
+#define PRIx64 "llx"
+
+typedef union
+{
+ double d;
+ uint64_t i;
+} u;
+
+int main()
+{
+ u smallestPositiveNormal, smallesPositiveSubnormal, expectedResult, result;
+
+ smallesPositiveSubnormal.i = 1;
+
+ smallestPositiveNormal.i = 0x0010000000000000;
+ expectedResult.i = 0x000fffffffffffff;
+ 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 <umesh.kalappa0@gmail.com>
+
+ PR libgcc/86512
+ * config/arm/ieee754-df.S :Don't normalise the denormal result.
+
+
2018-07-05 James Clarke <jrtc27@jrtc27.com>
* configure: Regenerated.
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, #0x00100000
+ blt LSYM(Lad_e)
+ cmp xh, #0x00100000
bcc LSYM(Lad_a)
cmp xh, #0x00200000
bcc LSYM(Lad_e)
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-08-02 16:50 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20 13:38 [Patch-86512]: Subnormal float support in armv7(with -msoft-float) for intrinsics Wilco Dijkstra
2018-07-20 16:00 ` Umesh Kalappa
2018-07-20 16:34 ` Wilco Dijkstra
2018-07-23 7:46 ` Umesh Kalappa
2018-07-23 9:58 ` Richard Earnshaw (lists)
2018-07-23 11:09 ` Umesh Kalappa
2018-07-23 11:42 ` Ramana Radhakrishnan
2018-07-23 11:54 ` Wilco Dijkstra
2018-07-24 8:39 ` Umesh Kalappa
2018-07-25 17:04 ` Umesh Kalappa
2018-07-26 16:36 ` Nicolas Pitre
2018-07-27 13:32 ` Wilco Dijkstra
2018-07-27 15:50 ` Nicolas Pitre
2018-07-27 16:01 ` Nicolas Pitre
2018-07-27 16:05 ` Wilco Dijkstra
2018-07-27 16:45 ` Nicolas Pitre
2018-08-02 16:50 ` Richard Earnshaw (lists)
-- strict thread matches above, loose matches on Subject: below --
2018-07-18 12:31 Umesh Kalappa
2018-07-20 12:05 ` Umesh Kalappa
2018-07-20 13:30 ` Szabolcs Nagy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).