[lworld+fp16] RFR: 8334432: Refine Float16.fma [v6]

[Raffaello Giulietti]

On Fri, 2 Aug 2024 01:52:13 GMT, Joe Darcy <darcy at openjdk.org> wrote:

>> Adding comments and test cases for Float16.fma.
>
> Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix typo in test comments.

src/java.base/share/classes/java/lang/Float16.java line 913:

> 911:          * representable in the Float16 format range from the
> 912:          * subnormal 2^(-24), MIN_VALUE, to 2^15, the leading bit
> 913:          * position of MAX_VALUE.

It's unclear what is meant by "bit position".

src/java.base/share/classes/java/lang/Float16.java line 917:

> 915:          * Consequently, a double can hold the exact sum of any two
> 916:          * Float16 values as the maximum difference in exponents of
> 917:          * Float16 values less than the precision width of double.

This is correct, but doesn't seem to be relevant for what follows. We don't have a sum between two `Float16` values here.

src/java.base/share/classes/java/lang/Float16.java line 937:

> 935:          *
> 936:          * For the product a*b of Float16 inputs, the range of
> 937:          * exponents for nonzero finite results goes from 2^(-28)

Suggestion:

         * exponents for nonzero finite results goes from 2^(-48)

src/java.base/share/classes/java/lang/Float16.java line 940:

> 938:          * (from MIN_VALUE squared) to 2^31 (from the exact value of
> 939:          * MAX_VALUE squared). This full range of exponent positions,
> 940:          * (31 -(-28) + 1 ) = 60 exceeds the precision of

Suggestion:

         * (31 -(-48) + 1 ) = 80 exceeds the precision of

src/java.base/share/classes/java/lang/Float16.java line 954:

> 952:          * = 0x1.ffdp16 will certainly overflow (under round to
> 953:          * nearest) since adding in c = -MAX_VALUE will still be above
> 954:          * the overflow threshold.

Suggestion:

         * Therefore, for any product greater than or equal to 0x1.ffep15 + MAX_VALUE
         * = 0x1.ffdp16, the "fma" will certainly overflow (under round to
         * nearest), since adding in c = -MAX_VALUE will still be above
         * or at the overflow threshold.

src/java.base/share/classes/java/lang/Float16.java line 971:

> 969:          * precision to hold down to the smallest subnormal bit
> 970:          * position, 15 - (-24) + 1 = 40 < 53. If the product was
> 971:          * large and overflowed when the third operand was added, this

In `double` arithmetic, as here, `a * b + c` cannot overflow, so this is a bit confusing.

src/java.base/share/classes/java/lang/Float16.java line 981:

> 979:          * The smallest exponent possible in a product is 2^(-48).
> 980:          * For moderately sized Float16 values added to the product,
> 981:          * with a leading exponent of about 4, the sum will not be

Suggestion:

         * with an exponent of about 4, the sum will not be

src/java.base/share/classes/java/lang/Float16.java line 984:

> 982:          * exact. Therefore, an analysis is needed to determine if the
> 983:          * double-rounding is benign or would lead to a different
> 984:          * final Float16 result. Double rounding an lead to a

Suggestion:

         * final Float16 result. Double rounding can lead to a

test/jdk/java/lang/Float16/BasicFloat16ArithTests.java line 386:

> 384:          */
> 385:         for(int i = 0; i <= Short.MAX_VALUE; i++ ) {
> 386:             // Start by just checking positive values...

The test does, in fact, also check the negated value.
It doesn't check negative zero, though.

Maybe it's simpler to loop like so

        for(int i = Short.MIN_VALUE; i <= Short.MAX_VALUE; i++ ) {

and drop the logic for the negated value.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711956807
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711956771
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711956690
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711961014
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711956573
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711974163
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711976147
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1711977126
PR Review Comment: https://git.openjdk.org/valhalla/pull/1143#discussion_r1712000660