On Mon, 21 Oct 2024 17:07:35 GMT, Joe Darcy <da...@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
