Re: [cp-patches] Bug fixes for StrictMath
Hi!
I rewrote StrictMathTest.java to look like other classes in /test folder (and
put the test there).
ChangeLog entries:
* test/Makefile.am: Add java.lang.reflect and java.lang to SUBDIRS.
* test/java.lang/.cvsignore: New file.
* test/java.lang/Makefile.am: Likewise.
* test/java.lang/StrictMathTest.java: Likewise.
* test/java.lang.reflect/ArrayTest.java: (main(String[])): Comment
out loadLibrary() call (since this should be done by VMArray
implementation).
Sat, 24 Jul 2010 12:47:01 +0400 Ivan Maidanski :
> Hello, Andrew!
>
> 1. As for .diff extensions - what should be tunable in a browser which app to
> use for opening the file.
>
> 2. As for testing for zero sign - I can't agree with you because:
> - Sun writes "Floating-point positive zero and floating-point negative
> zero compare as equal, but there are other operations that can distinguish
> them; for example, dividing 1.0 by 0.0 produces positive infinity, but
> dividing 1.0 by -0.0 produces negative infinity."
> (http://java.sun.com/docs/books/jvms/second_edition/html/Overview.doc.html);
> - your consideration about the asm code produced for doubleToRawLongBits are
> valid for machines which use the same FP formats as JVM;
> - yes it's always better to say what you mean and let the optimizer worry
> about optimization if the latter is capable of doing such things but
> doubleToBits(x)<0 and 1/x<0 are equally far from what I mean (x==-0) in
> that comment.
>
> 3. The tests cases class is attached.
>
> Regards.
>
>
> -
> Original message:
>
> On 07/20/2010 07:06 PM, Ivan Maidanski wrote:
> > Hello, Andrew!
> >
> >> On 06/29/2010 10:22 AM, Ivan Maidanski wrote:
> >>> Hi!
> >>>
> >>> I've fixed a number of bugs in StrictMath class (the RI here
> is fdlibm of some version).
> >>>
> >>> ChangeLog entries:
> >>>
> >>> * java/lang/StrictMath.java:
> >>> (acos(double)): Bug fix for x <= -1/2 case.
> >>> (atan(double)): Fix documentation typo.
> >>> (pow(double,double)): Fix a comment; put y == 1/2 case handling
> after
> >>> x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0);
> return -0
> >>> (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify
> >>> expression for "negative" variable.
> >>> (IEEEremainder(double,double)): Bug fix for x == -0 and x ==
> -|y|
> >>> cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2.
> >>> (remPiOver2(double[],double[],int,int)): Reset
> "recompute" at the
> >>> beginning of every do/while iteration.
> >>> (tan(double,double,boolean)): Negate the result if x <=
> -0.6744.
> >>
> >> Hi Ivan,
> >>
> >> Firstly please attach patches as type text; that way people can read
> >> them in their mailers.
> >
> > I don't understand. I attach patches with .diff extension. Shall I
> use .txt?
>
> I think they had a mime-type of application/binary.
>
> >> Can you please send test cases that pass with this patch?
> >
> > Ok. I'll write them a bit later...
> >
> >>
> >> diff -ru CVS/classpath/java/lang/StrictMath.java
> updated/classpath/java/lang/StrictMath.java
> >> --- CVS/classpath/java/lang/StrictMath.java2010-06-29
> 10:11:06.0 +0400
> >> +++ updated/classpath/java/lang/StrictMath.java2010-06-29
> 12:51:50.0 +0400
> >> @@ -1,5 +1,6 @@
> >> /* java.lang.StrictMath -- common mathematical functions, strict Java
> >> - Copyright (C) 1998, 2001, 2002, 2003, 2006 Free Software
> Foundation, Inc.
> >> + Copyright (C) 1998, 2001, 2002, 2003, 2006, 2010
> >> + Free Software Foundation, Inc.
> >>
> >> This file is part of GNU Classpath.
> >>
> >> @@ -456,9 +457,10 @@
> >> double r = x - (PI_L / 2 - x * (p / q));
> >> return negative ? PI / 2 + r : PI / 2 - r;
> >> }
> >> +double z = (1 - x) * 0.5;
> >> if (negative) // x<=-0.5.
> >> {
> >> -double z = (1 + x) * 0.5;
> >> +// z = (1+orig_x)*0.5
> >>
> >> Please don't leave commented lines in code. If they're
> wrong, please
> >> take them out.
> >
> > This is not a commented code - it's a reference C code from fdlibm.
> It might be better to use some words
> instead of a formular like "x is not modified in fdlibm unlike in this
> implementation so we use 1-x (where
> x==-orig_x) instead of 1+x".
> >
> >
> >>
> >> @@ -1554,10 +1553,18 @@
> >> else if (yisint == 1)
> >> ax = -ax;
> >> }
> >> +else
> >> + {
> >> +// Check for x == -0 with odd y.
> >> +if (1 / x < 0
> >>
> >> Why use "1 / x < 0" ?
> >
>
> > What's else to use? The alternative is Double.doubleToRawLongBits(x)
> > < 0 but (1/x)<0 is cheaper (recall that |x| is zero).
>
> In
>
> class T
> {
> boolean foo(double x)
> {
> return Double.doubleToRawLongBits(x) < 0;
> }
>
> boolean bar(double x)
> {
> return (1/x) < 0;
> }
> }
>
> gcj gives you
>
> T.foo(double)boolean:
> movsd %xmm0, -16(%rsp)
> mo
Re: [cp-patches] Bug fixes for StrictMath
Mon, 26 Jul 2010 10:23:15 +0100 Andrew Haley : > On 07/24/2010 09:47 AM, Ivan Maidanski wrote: > > Hello, Andrew! > > Hi, > > > 1. As for .diff extensions - what should be tunable in a browser > > which app to use for opening the file. > > All I was saying is: please don't mark text attachments as binary. This is out of my control - I just attach that file and nothing more. How could I mark it? > Most mail readers will not display such attachments. This means that > fewer people will see the patch. I apologies. Sorry. > > > 2. As for testing for zero sign - I can't agree with you because: > > > - Sun writes "Floating-point positive zero and floating-point > > negative zero compare as equal, but there are other operations that > > can distinguish them; for example, dividing 1.0 by 0.0 produces > > positive infinity, but dividing 1.0 by -0.0 produces negative > > infinity." > > (http://java.sun.com/docs/books/jvms/second_edition/html/Overview.doc.html); > > > - your consideration about the asm code produced for > > doubleToRawLongBits are valid for machines which use the same FP > > formats as JVM; > > Which is all of them! :-) Not all. E.g., some ARM chips have swapped words order for FP doubles. But I definitely agree IEEE FP is 100% dominated. > > > - yes it's always better to say what you mean and let the optimizer > > worry about optimization if the latter is capable of doing such > > things but doubleToBits(x)<0 and 1/x<0 are equally far from what I > > mean (x==-0) in that comment. > > Well, your claim was that 1/x<0 was cheaper than doubleToBits(x)<0, > and I showed why that probably isn't true in many cases. If you still > want to keep that form, even though it isn't cheaper, that's OK. But > it's important not to base decisions on false data. Ok. I was wrong saying that. Let me not change that code (just to not resend the patch) - it's not a big deal really to use any code there. Thanks for the explanations. > > Andrew.
Re: [cp-patches] Bug fixes for StrictMath
On 07/24/2010 09:47 AM, Ivan Maidanski wrote: > Hello, Andrew! Hi, > 1. As for .diff extensions - what should be tunable in a browser > which app to use for opening the file. All I was saying is: please don't mark text attachments as binary. Most mail readers will not display such attachments. This means that fewer people will see the patch. > 2. As for testing for zero sign - I can't agree with you because: > - Sun writes "Floating-point positive zero and floating-point > negative zero compare as equal, but there are other operations that > can distinguish them; for example, dividing 1.0 by 0.0 produces > positive infinity, but dividing 1.0 by -0.0 produces negative > infinity." > (http://java.sun.com/docs/books/jvms/second_edition/html/Overview.doc.html); > - your consideration about the asm code produced for > doubleToRawLongBits are valid for machines which use the same FP > formats as JVM; Which is all of them! :-) > - yes it's always better to say what you mean and let the optimizer > worry about optimization if the latter is capable of doing such > things but doubleToBits(x)<0 and 1/x<0 are equally far from what I > mean (x==-0) in that comment. Well, your claim was that 1/x<0 was cheaper than doubleToBits(x)<0, and I showed why that probably isn't true in many cases. If you still want to keep that form, even though it isn't cheaper, that's OK. But it's important not to base decisions on false data. Andrew.
Re: [cp-patches] Bug fixes for StrictMath
Hello, Andrew!
1. As for .diff extensions - what should be tunable in a browser which app to
use for opening the file.
2. As for testing for zero sign - I can't agree with you because:
- Sun writes "Floating-point positive zero and floating-point negative zero
compare as equal, but there are other operations that can distinguish them; for
example, dividing 1.0 by 0.0 produces positive infinity, but dividing 1.0 by
-0.0 produces negative infinity."
(http://java.sun.com/docs/books/jvms/second_edition/html/Overview.doc.html);
- your consideration about the asm code produced for doubleToRawLongBits are
valid for machines which use the same FP formats as JVM;
- yes it's always better to say what you mean and let the optimizer worry about
optimization if the latter is capable of doing such things but
doubleToBits(x)<0 and 1/x<0 are equally far from what I mean (x==-0) in that
comment.
3. The tests cases class is attached.
Regards.
-
Original message:
On 07/20/2010 07:06 PM, Ivan Maidanski wrote:
> Hello, Andrew!
>
>> On 06/29/2010 10:22 AM, Ivan Maidanski wrote:
>>> Hi!
>>>
>>> I've fixed a number of bugs in StrictMath class (the RI here is fdlibm of
>>> some version).
>>>
>>> ChangeLog entries:
>>>
>>> * java/lang/StrictMath.java:
>>> (acos(double)): Bug fix for x <= -1/2 case.
>>> (atan(double)): Fix documentation typo.
>>> (pow(double,double)): Fix a comment; put y == 1/2 case handling after
>>> x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0); return -0
>>> (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify
>>> expression for "negative" variable.
>>> (IEEEremainder(double,double)): Bug fix for x == -0 and x == -|y|
>>> cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2.
>>> (remPiOver2(double[],double[],int,int)): Reset "recompute" at the
>>> beginning of every do/while iteration.
>>> (tan(double,double,boolean)): Negate the result if x <= -0.6744.
>>
>> Hi Ivan,
>>
>> Firstly please attach patches as type text; that way people can read
>> them in their mailers.
>
> I don't understand. I attach patches with .diff extension. Shall I use .txt?
I think they had a mime-type of application/binary.
>> Can you please send test cases that pass with this patch?
>
> Ok. I'll write them a bit later...
>
>>
>> diff -ru CVS/classpath/java/lang/StrictMath.java
>> updated/classpath/java/lang/StrictMath.java
>> --- CVS/classpath/java/lang/StrictMath.java 2010-06-29 10:11:06.0
>> +0400
>> +++ updated/classpath/java/lang/StrictMath.java 2010-06-29
>> 12:51:50.0 +0400
>> @@ -1,5 +1,6 @@
>> /* java.lang.StrictMath -- common mathematical functions, strict Java
>> - Copyright (C) 1998, 2001, 2002, 2003, 2006 Free Software Foundation, Inc.
>> + Copyright (C) 1998, 2001, 2002, 2003, 2006, 2010
>> + Free Software Foundation, Inc.
>>
>> This file is part of GNU Classpath.
>>
>> @@ -456,9 +457,10 @@
>> double r = x - (PI_L / 2 - x * (p / q));
>> return negative ? PI / 2 + r : PI / 2 - r;
>> }
>> +double z = (1 - x) * 0.5;
>> if (negative) // x<=-0.5.
>> {
>> -double z = (1 + x) * 0.5;
>> +// z = (1+orig_x)*0.5
>>
>> Please don't leave commented lines in code. If they're wrong, please
>> take them out.
>
> This is not a commented code - it's a reference C code from fdlibm. It might
> be better to use some words
instead of a formular like "x is not modified in fdlibm unlike in this
implementation so we use 1-x (where
x==-orig_x) instead of 1+x".
>
>
>>
>> @@ -1554,10 +1553,18 @@
>> else if (yisint == 1)
>> ax = -ax;
>> }
>> +else
>> + {
>> +// Check for x == -0 with odd y.
>> +if (1 / x < 0
>>
>> Why use "1 / x < 0" ?
>
> What's else to use? The alternative is Double.doubleToRawLongBits(x)
> < 0 but (1/x)<0 is cheaper (recall that |x| is zero).
In
class T
{
boolean foo(double x)
{
return Double.doubleToRawLongBits(x) < 0;
}
boolean bar(double x)
{
return (1/x) < 0;
}
}
gcj gives you
T.foo(double)boolean:
movsd %xmm0, -16(%rsp)
movq-16(%rsp), %rax
shrq$63, %rax
ret
for the first, and
T.bar(double)boolean:
movsd .LC0(%rip), %xmm1
divsd %xmm0, %xmm1
movapd %xmm1, %xmm0
xorpd %xmm1, %xmm1
ucomisd %xmm0, %xmm1
seta%al
ret
for the second. Any JIT might do something similar.
IMO it's always better to say what you mean and let the optimizer
worry about optimization.
Andrew.
StrictMathTest.java
Description: Binary data
Re: [cp-patches] Bug fixes for StrictMath
On 07/20/2010 07:06 PM, Ivan Maidanski wrote:
> Hello, Andrew!
>
>> On 06/29/2010 10:22 AM, Ivan Maidanski wrote:
>>> Hi!
>>>
>>> I've fixed a number of bugs in StrictMath class (the RI here is fdlibm of
>>> some version).
>>>
>>> ChangeLog entries:
>>>
>>> * java/lang/StrictMath.java:
>>> (acos(double)): Bug fix for x <= -1/2 case.
>>> (atan(double)): Fix documentation typo.
>>> (pow(double,double)): Fix a comment; put y == 1/2 case handling after
>>> x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0); return -0
>>> (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify
>>> expression for "negative" variable.
>>> (IEEEremainder(double,double)): Bug fix for x == -0 and x == -|y|
>>> cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2.
>>> (remPiOver2(double[],double[],int,int)): Reset "recompute" at the
>>> beginning of every do/while iteration.
>>> (tan(double,double,boolean)): Negate the result if x <= -0.6744.
>>
>> Hi Ivan,
>>
>> Firstly please attach patches as type text; that way people can read
>> them in their mailers.
>
> I don't understand. I attach patches with .diff extension. Shall I use .txt?
I think they had a mime-type of application/binary.
>> Can you please send test cases that pass with this patch?
>
> Ok. I'll write them a bit later...
>
>>
>> diff -ru CVS/classpath/java/lang/StrictMath.java
>> updated/classpath/java/lang/StrictMath.java
>> --- CVS/classpath/java/lang/StrictMath.java 2010-06-29 10:11:06.0
>> +0400
>> +++ updated/classpath/java/lang/StrictMath.java 2010-06-29
>> 12:51:50.0 +0400
>> @@ -1,5 +1,6 @@
>> /* java.lang.StrictMath -- common mathematical functions, strict Java
>> - Copyright (C) 1998, 2001, 2002, 2003, 2006 Free Software Foundation, Inc.
>> + Copyright (C) 1998, 2001, 2002, 2003, 2006, 2010
>> + Free Software Foundation, Inc.
>>
>> This file is part of GNU Classpath.
>>
>> @@ -456,9 +457,10 @@
>> double r = x - (PI_L / 2 - x * (p / q));
>> return negative ? PI / 2 + r : PI / 2 - r;
>> }
>> +double z = (1 - x) * 0.5;
>> if (negative) // x<=-0.5.
>> {
>> -double z = (1 + x) * 0.5;
>> +// z = (1+orig_x)*0.5
>>
>> Please don't leave commented lines in code. If they're wrong, please
>> take them out.
>
> This is not a commented code - it's a reference C code from fdlibm. It might
> be better to use some words instead of a formular like "x is not modified in
> fdlibm unlike in this implementation so we use 1-x (where x==-orig_x) instead
> of 1+x".
>
>
>>
>> @@ -1554,10 +1553,18 @@
>> else if (yisint == 1)
>> ax = -ax;
>> }
>> +else
>> + {
>> +// Check for x == -0 with odd y.
>> +if (1 / x < 0
>>
>> Why use "1 / x < 0" ?
>
> What's else to use? The alternative is Double.doubleToRawLongBits(x)
> < 0 but (1/x)<0 is cheaper (recall that |x| is zero).
In
class T
{
boolean foo(double x)
{
return Double.doubleToRawLongBits(x) < 0;
}
boolean bar(double x)
{
return (1/x) < 0;
}
}
gcj gives you
T.foo(double)boolean:
movsd %xmm0, -16(%rsp)
movq-16(%rsp), %rax
shrq$63, %rax
ret
for the first, and
T.bar(double)boolean:
movsd .LC0(%rip), %xmm1
divsd %xmm0, %xmm1
movapd %xmm1, %xmm0
xorpd %xmm1, %xmm1
ucomisd %xmm0, %xmm1
seta%al
ret
for the second. Any JIT might do something similar.
IMO it's always better to say what you mean and let the optimizer
worry about optimization.
Andrew.
Re: [cp-patches] Bug fixes for StrictMath
Hello, Andrew!
>On 06/29/2010 10:22 AM, Ivan Maidanski wrote:
>> Hi!
>>
>> I've fixed a number of bugs in StrictMath class (the RI here is fdlibm of
>> some version).
>>
>> ChangeLog entries:
>>
>> * java/lang/StrictMath.java:
>> (acos(double)): Bug fix for x <= -1/2 case.
>> (atan(double)): Fix documentation typo.
>> (pow(double,double)): Fix a comment; put y == 1/2 case handling after
>> x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0); return -0
>> (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify
>> expression for "negative" variable.
>> (IEEEremainder(double,double)): Bug fix for x == -0 and x == -|y|
>> cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2.
>> (remPiOver2(double[],double[],int,int)): Reset "recompute" at the
>> beginning of every do/while iteration.
>> (tan(double,double,boolean)): Negate the result if x <= -0.6744.
>
>Hi Ivan,
>
>Firstly please attach patches as type text; that way people can read
>them in their mailers.
I don't understand. I attach patches with .diff extension. Shall I use .txt?
>
>Can you please send test cases that pass with this patch?
Ok. I'll write them a bit later...
>
>diff -ru CVS/classpath/java/lang/StrictMath.java
>updated/classpath/java/lang/StrictMath.java
>--- CVS/classpath/java/lang/StrictMath.java2010-06-29 10:11:06.0
>+0400
>+++ updated/classpath/java/lang/StrictMath.java2010-06-29
>12:51:50.0 +0400
>@@ -1,5 +1,6 @@
> /* java.lang.StrictMath -- common mathematical functions, strict Java
>- Copyright (C) 1998, 2001, 2002, 2003, 2006 Free Software Foundation, Inc.
>+ Copyright (C) 1998, 2001, 2002, 2003, 2006, 2010
>+ Free Software Foundation, Inc.
>
> This file is part of GNU Classpath.
>
>@@ -456,9 +457,10 @@
> double r = x - (PI_L / 2 - x * (p / q));
> return negative ? PI / 2 + r : PI / 2 - r;
> }
>+double z = (1 - x) * 0.5;
> if (negative) // x<=-0.5.
> {
>-double z = (1 + x) * 0.5;
>+// z = (1+orig_x)*0.5
>
>Please don't leave commented lines in code. If they're wrong, please
>take them out.
This is not a commented code - it's a reference C code from fdlibm. It might be
better to use some words instead of a formular like "x is not modified in
fdlibm unlike in this implementation so we use 1-x (where x==-orig_x) instead
of 1+x".
>
>@@ -1554,10 +1553,18 @@
> else if (yisint == 1)
> ax = -ax;
> }
>+else
>+ {
>+// Check for x == -0 with odd y.
>+if (1 / x < 0
>
>Why use "1 / x < 0" ?
What's else to use? The alternative is Double.doubleToRawLongBits(x) < 0 but
(1/x)<0 is cheaper (recall that |x| is zero).
>
>Andrew.
Re: [cp-patches] Bug fixes for StrictMath
On 06/29/2010 10:22 AM, Ivan Maidanski wrote:
> Hi!
>
> I've fixed a number of bugs in StrictMath class (the RI here is fdlibm of
> some version).
>
> ChangeLog entries:
>
> * java/lang/StrictMath.java:
> (acos(double)): Bug fix for x <= -1/2 case.
> (atan(double)): Fix documentation typo.
> (pow(double,double)): Fix a comment; put y == 1/2 case handling after
> x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0); return -0
> (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify
> expression for "negative" variable.
> (IEEEremainder(double,double)): Bug fix for x == -0 and x == -|y|
> cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2.
> (remPiOver2(double[],double[],int,int)): Reset "recompute" at the
> beginning of every do/while iteration.
> (tan(double,double,boolean)): Negate the result if x <= -0.6744.
Hi Ivan,
Firstly please attach patches as type text; that way people can read
them in their mailers.
Can you please send test cases that pass with this patch?
diff -ru CVS/classpath/java/lang/StrictMath.java
updated/classpath/java/lang/StrictMath.java
--- CVS/classpath/java/lang/StrictMath.java 2010-06-29 10:11:06.0
+0400
+++ updated/classpath/java/lang/StrictMath.java 2010-06-29 12:51:50.0
+0400
@@ -1,5 +1,6 @@
/* java.lang.StrictMath -- common mathematical functions, strict Java
- Copyright (C) 1998, 2001, 2002, 2003, 2006 Free Software Foundation, Inc.
+ Copyright (C) 1998, 2001, 2002, 2003, 2006, 2010
+ Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -456,9 +457,10 @@
double r = x - (PI_L / 2 - x * (p / q));
return negative ? PI / 2 + r : PI / 2 - r;
}
+double z = (1 - x) * 0.5;
if (negative) // x<=-0.5.
{
-double z = (1 + x) * 0.5;
+// z = (1+orig_x)*0.5
Please don't leave commented lines in code. If they're wrong, please
take them out.
@@ -1554,10 +1553,18 @@
else if (yisint == 1)
ax = -ax;
}
+else
+ {
+// Check for x == -0 with odd y.
+if (1 / x < 0
Why use "1 / x < 0" ?
Andrew.
[cp-patches] Bug fixes for StrictMath
Hi! I've fixed a number of bugs in StrictMath class (the RI here is fdlibm of some version). ChangeLog entries: * java/lang/StrictMath.java: (acos(double)): Bug fix for x <= -1/2 case. (atan(double)): Fix documentation typo. (pow(double,double)): Fix a comment; put y == 1/2 case handling after x == -0 case (since pow(-0, 1/2) == 0 but sqrt(-0) == -0); return -0 (or -INF) for pow(-0, 2*k) where k is non-zero integer; simplify expression for "negative" variable. (IEEEremainder(double,double)): Bug fix for x == -0 and x == -|y| cases; bug fix for |y| >= 2**-1021 and |x| > |y|/2. (remPiOver2(double[],double[],int,int)): Reset "recompute" at the beginning of every do/while iteration. (tan(double,double,boolean)): Negate the result if x <= -0.6744. Regards. classpath-ivmai-33.diff Description: Binary data
