On Fri, 17 Feb 2023 04:18:30 GMT, Joe Darcy <da...@openjdk.org> wrote:
>> Working down the porting list, next stop, atan2.
>
> Joe Darcy has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains three additional commits since
> the last revision:
>
> - Update regression test.
> - Merge branch 'master' into JDK-8302028
> - JDK-8302028: Port fdlibm atan2 to Java
src/java.base/share/classes/java/lang/FdLibm.java line 447:
> 445: ly = __LO(y);
> 446: if (((ix | ((lx | -lx) >> 31)) > 0x7ff0_0000)||
> 447: ((iy |((ly | - ly) >> 31)) > 0x7ff0_0000)) // x or y
> is NaN
I think that `>> 31` must be replaced by `>>> 31`.
src/java.base/share/classes/java/lang/FdLibm.java line 458:
> 456: case 0, 1 -> y; // atan(+/-0, +anything)
> = +/-0
> 457: case 2 -> Math.PI + tiny; // atan(+0, -anything)
> = pi
> 458: default -> -Math.PI - tiny; // atan(-0, -anything)
> = -pi
The original switch statement and this switch expression are semantically
equivalent only because of our knowledge that `m` can only assume values 0, 1,
2, or 3. This requires more reasoning than a more verbatim copy of the original
statement. Not sure if it is worth.
The same holds for the switch expressions below.
test/jdk/java/lang/Math/Atan2Tests.java line 214:
> 212: /*
> 213: * If both arguments are negative infinity, then the result
> is the
> 214: * double value closest to -3*pi/4.
Perhaps add a note explaining that high precision computation shows that
`-3*PI/4.0` is indeed the `double` closest to -3*pi/4.
Since `PI` is an approximation in the first place, and since there are other
two operations (in particular, the multiplication by 3), the claim is not
evident.
test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 451:
> 449: ly = __LO(y);
> 450: if(((ix|((lx|-lx)>>31))>0x7ff00000)||
> 451: ((iy|((ly|-ly)>>31))>0x7ff00000)) /* x or y is NaN */
I think that `>>31` must be replaced by `>>>31`.
test/jdk/java/lang/StrictMath/FdlibmTranslit.java line 480:
> 478: switch(m) {
> 479: case 0: return zero ; /* atan(+...,+INF) */
> 480: case 1: return -1.0*zero ; /* atan(-...,+INF) */
The file in the [netlib repo](https://netlib.org/fdlibm/e_atan2.c) has `-zero`
rather than `-1.0*zero`.
-------------
PR: https://git.openjdk.org/jdk/pull/12608
