RFR: 8341260: Add Float16 to jdk.incubator.vector [v3]

[Paul Sandoz]

On Mon, 21 Oct 2024 17:07:35 GMT, Joe Darcy <darcy at openjdk.org> wrote:

>> Port of Float16 from java.lang in the lworld+fp16 branch to jdk.incubabor.vector.
>
> 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 seven additional commits since the last revision:
> 
>  - Add @since tag, respond to review feedback.
>  - Merge branch 'master' into JDK-8341260
>  - Remove comments for intrinsics per review feedback.
>  - Update with changes from lworld+fp16 Float16.
>  - Merge branch 'master' into JDK-8341260
>  - Add support for BigDecimal -> Float16 conversion.
>  - JDK-8341260: Add Float16 to jdk.incubator.vector

Looking good, although i am not expert on the floating point aspects. Some of the more substantive comments could be addressed if chosen to in subsequent PRs.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 70:

> 68: 
> 69: // Currently Float16 is a value based class but in future will be aligned with
> 70: // Enhanced Primitive Boxes described by JEP-402 (https://openjdk.org/jeps/402)

It will be aligned with JEP 401: Value Classes and Objects. `Float16` will eventually become a value class, it will not become a primitive type. JEP 402 is about treating (the existing) primitive types more like reference types. Note as per JEP 401 value class types are reference types.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 72:

> 70: // Enhanced Primitive Boxes described by JEP-402 (https://openjdk.org/jeps/402)
> 71: // @jdk.internal.MigratedValueClass
> 72: //@jdk.internal.ValueBased

We can modify the module info in `java.base` to export `jdk.internal` to `jdk.incubator.vector` then we can annotate `Float16` with `ValueBased`, and then we can get some diagnostic checking e.g., if there is an attempt to synchronize on it.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 78:

> 76:     implements Comparable<Float16> {
> 77:     private final short value;
> 78:     private static final long serialVersionUID = 16; // Not needed for a value class?

Unsure if this field needed. Perhaps a wider question to be thought and answered about later is whether we should extend from `Number`.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 89:

> 87:     * @param  bits a short value.
> 88:     */
> 89:     private Float16 (short bits ) {

Suggestion:

    private Float16(short bits) {

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 690:

> 688:     // public int hashCode()
> 689:     // public static int hashCode(Float16 value)
> 690:     // public boolean equals(Object obj)

I think we should add these methods in a subsequent PR. e.g., (although i may be ignorant of some details)

    @Override
    public int hashCode() {
        return Float16.hashCode(value);
    }

    public static int hashCode(float value) {
        return float16ToShortBits(value); 
    }

    public static int float16ToShortBits(float value) {
        if (!isNaN(value)) {
            return float16ToRawShortBits(value);
        }
        return Float16.NaN;
    }

    public boolean equals(Object obj) {
        return (obj instanceof Float16)
               && (float16ToShortBits(((Float16)obj).value) == float16ToShortBits(value));
    }

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java line 1198:

> 1196:      *
> 1197:      * @param f16 the value to be negated
> 1198:      * @jls 15.15.4 Unary Minus Operator {@code -}

In prior binary operations it is just stated `* @jls 15.4 Floating-point Expressions` can we be consistent?

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16Consts.java line 92:

> 90:                 ((EXP_BIT_MASK & SIGNIF_BIT_MASK) == 0)) &&
> 91:                 ((SIGN_BIT_MASK | MAG_BIT_MASK) == 0xFFFF));
> 92:     }

Consider moving this to a test, perhaps in a subsequent PR when addressing refactoring the Float16 tests out of `BigDecimal/DoubleFloatValueTests`.

test/jdk/jdk/incubator/vector/BasicFloat16ArithTests.java line 34:

> 32: import static jdk.incubator.vector.Float16.*;
> 33: 
> 34: public class BasicFloat16ArithTests {

Consider transforming this to a junit test, using asserts, and possibly data providers in some cases, and factoring out the FMA tests into a separate class e.g., in subsequent PR. It will then be clearer on what is tested, with what data, and what is asserted on.

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

PR Review: https://git.openjdk.org/jdk/pull/21574#pullrequestreview-2386086446
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811280538
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811287099
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811290960
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811291449
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811308765
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811301176
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811260726
PR Review Comment: https://git.openjdk.org/jdk/pull/21574#discussion_r1811267765