8277304: Java support for FP16 by smita-kamath · Pull Request #164 · openjdk/panama-vector

smita-kamath · 2021-11-16T23:52:01Z

Initial FP16 vectorAPI Java support.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8277304: Java support for FP16

Reviewers

Sandhya Viswanathan (@sviswa7 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-vector pull/164/head:pull/164
$ git checkout pull/164

Update a local copy of the PR:
$ git checkout pull/164
$ git pull https://git.openjdk.org/panama-vector pull/164/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 164

View PR using the GUI difftool:
$ git pr show -t 164

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-vector/pull/164.diff

bridgekeeper · 2021-11-16T23:52:20Z

👋 Welcome back svkamath! A progress list of the required criteria for merging this PR into vectorIntrinsics+fp16 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-11-17T00:22:22Z

Webrevs

LifeIsStrange · 2021-11-24T12:55:24Z

I could be useful to support Brainfloats in addition to standard FP16? https://en.wikipedia.org/wiki/Bfloat16_floating-point_format

jatin-bhateja · 2022-02-10T02:30:19Z

Hi Smita,
Following patch fixes the failing base testcase.
Thanks,
Jatin

diff --git a/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java b/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
index 18c0cb9a725..9def1acdd61 100644
--- a/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
+++ b/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
@@ -298,7 +298,16 @@ abstract class AbstractSpecies<E> extends jdk.internal.vm.vector.VectorSupport.V
         return makeDummyVector();
     }
     private AbstractVector<E> makeDummyVector() {
-        Object za = Array.newInstance(elementType(), laneCount);
+        Object za;
+        // FIXME: Remove following special handling for Halffloat
+        // till Valhalla integration when Halffloat will become a
+        // primitive class.
+        if (elementType() == Halffloat.class)  {
+           za = Array.newInstance(short.class, laneCount);
+        } else {
+           za = Array.newInstance(elementType(), laneCount);
+        }
+
         return dummyVector = vectorFactory.apply(za);
         // This is the only use of vectorFactory.
         // All other factory requests are routed
diff --git a/test/jdk/jdk/incubator/vector/AddTest.java b/test/jdk/jdk/incubator/vector/AddTest.java
index 7c6329a7a3c..7ce077f75ec 100644
--- a/test/jdk/jdk/incubator/vector/AddTest.java
+++ b/test/jdk/jdk/incubator/vector/AddTest.java
@@ -44,20 +44,18 @@ public class AddTest {
     static short[] a = new short[SIZE];
     static short[] b = new short[SIZE];
     static short[] c = new short[SIZE];
-
     static {
         for (int i = 0; i < SIZE; i++) {
-            a[i] = 0x3C66;
-            b[i] = 0x4066;
+            a[i] = Halffloat.valueOf((float)i);
+            b[i] = Halffloat.valueOf((float)i);
         }
     }
 
     static void workload() {
         for (int i = 0; i < a.length; i += SPECIES.length()) {
             HalffloatVector av = HalffloatVector.fromArray(SPECIES, a, i);
-            //HalffloatVector bv = HalffloatVector.fromArray(SPECIES, b, i);
-            //av.add(bv).intoArray(c, i);
-            av.intoArray(c,i);
+            HalffloatVector bv = HalffloatVector.fromArray(SPECIES, b, i);
+            av.add(bv).intoArray(c, i);
         }
     }
 
@@ -76,8 +74,14 @@ public class AddTest {
             workload();
         }
         for (int i = 0; i < a.length; i++) {
-            if (c[i] != a[i] + b[i])
+            Halffloat hfa = new Halffloat(a[i]);
+            Halffloat hfb = new Halffloat(b[i]);
+            Halffloat hfc = new Halffloat(c[i]);
+
+            if (hfc.floatValue() != (hfa.floatValue() + hfb.floatValue())) {
+                System.out.println("RES: " + hfc.floatValue() + " EXPECTED: " + (hfa.floatValue() + hfb.floatValue()));
                 throw new AssertionError();
+            }
         }
 
         /*Arrays.fill(c, 0.0f);
@@ -89,5 +93,6 @@ public class AddTest {
             if (c[i] != a[i] + b[i])
                 throw new AssertionError();
         }*/
+        System.out.println("PASSED");
     }
 }

…moves redundant code from classes

jatin-bhateja · 2022-06-13T18:01:46Z

convert0 routine should be extended for half float types.
https://github.com/openjdk/panama-vector/pull/164/files#diff-06250048c1319bd72ad78c30f6c8ed06448201505b7a8d59757dc7dd0b78753eL659

jatin-bhateja · 2022-06-13T17:43:47Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+    static final short MAX_VALUE = valueOf(0x1.fffffeP+127f);
+    /* Definitions for FP16 */
+    static final short MIN_VALUE = valueOf(0x0.000002P-126f);
+    /* Definitions for FP16 */
+    static final short POSITIVE_INFINITY = valueOf(1.0f/0.0f);
+    /* Definitions for FP16 */
+    static final short NEGATIVE_INFINITY = valueOf(-1.0f/0.0f);
+    /* Definitions for FP16 */
+    static final int SIZE = 16;
+    /* Definitions for FP16 */
+    static final int BYTES = SIZE / Byte.SIZE;


All static constants should be made public on the lines of Float.MAX_VALUE.

jatin-bhateja · 2022-06-13T17:44:28Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+    static final short MAX_VALUE = valueOf(0x1.fffffeP+127f);
+    /* Definitions for FP16 */
+    static final short MIN_VALUE = valueOf(0x0.000002P-126f);


Min and max value needs to be correctly updated.

jatin-bhateja · 2022-06-13T17:45:48Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+ * @since  10/01/2021
+ */
+@SuppressWarnings("cast")
+public final class Halffloat {


Should Halffloat be a sub-class of Number and implement Comparable interfaces., like BigInteger which is another non-primitive box type.

jatin-bhateja · 2022-06-13T17:50:14Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+     * @return short value of float provided
+    */
+    public static short valueOf(float f) {
+        int val = Float.floatToIntBits(f);


As per JVM specification of d2f semantics, "A finite value too small to be represented as a float is converted
to a zero of the same sign; a finite value too large to be represented
as a float is converted to an infinity of the same sign. A double
NaN is converted to a float NaN."

I think we can apply same semantics for halffloat values if floting point argument is beyond the halffloat range.

jatin-bhateja · 2022-06-13T17:53:07Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template

+#if[short]
+                                wb_.get{#if[byte]?(:Short(}o + i * $sizeInBytes$));
+#else[short]
+                                wb_.get{#if[byte]?(:$Type$(}o + i * $sizeInBytes$));
+#end[short]


Please introduce a new abstract variable in generation script which is Short for halfloat or $Type otherwise. IT will help in folding this special handling.

jatin-bhateja · 2022-06-13T17:54:39Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-Vector.java.template

+#if[short]
+    public static final VectorSpecies<Halffloat> SPECIES_PREFERRED
+        = VectorSpecies.ofPreferred(Halffloat.class);
+#end[short]
+#else[FP]
+    public static final VectorSpecies<$Boxtype$> SPECIES_PREFERRED
+        = ($Type$Species) VectorSpecies.ofPreferred($type$.class);


Please introduce a new abstract variable in generation script which is Halffloat for halfloat case or $type otherwise. IT will help in folding this special handling.

jatin-bhateja · 2022-06-13T17:56:52Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 1994, 2017, Oracle and/or its affiliates. All rights reserved.


Copyright year should be 2022,

jatin-bhateja · 2022-06-13T17:57:52Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/IntVector.java

                return s.ldOp(wb, off, vm,
-                        (wb_, o, i) -> wb_.getInt(o + i * 4));
+                        (wb_, o, i) ->
+                            wb_.getInt(o + i * 4));
            });


Kindly remove these formatting changes from all integral type vector classes, they are anyways not impacting logic.

jatin-bhateja · 2022-06-15T14:56:24Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+    public float floatValue() {
+        int val = (int)value;
+        float f = Float.intBitsToFloat(((val&0x8000)<<16) | (((val&0x7c00)+0x1C000)<<13) | ((val&0x03FF)<<13));
+        return f;
+    }


Please add a switch to handle special case values (NaN, Inf, -Inf), i.e. HF.NaN -> Float.NaN :

jatin-bhateja · 2022-06-15T14:57:16Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+     * @return boolean value
+     */
+    public static boolean isFinite(float f) {
+        return Math.abs(f) <= Halffloat.MAX_VALUE;


We are comparing abs value with short. wont work well.

jatin-bhateja · 2022-06-15T14:58:14Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+        if (!isFinite(f)) return Halffloat.POSITIVE_INFINITY;
+
+        int val = Float.floatToIntBits(f);


Handling for NaN and NEG_INF missing.

jatin-bhateja · 2022-06-15T18:04:07Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+     * Halffloat constructor
+     * @param value short value assigned to halffloat
+     */
+     public Halffloat(short value) {


It will be useful if we can have a constructor accepting float also.

jatin-bhateja · 2022-06-15T18:05:32Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+     * @return short value of float provided
+    */
+    public static short valueOf(float f) {
+        if (!Float.isFinite(f)) return Halffloat.POSITIVE_INFINITY;


Will be appropriate if range bound checks are done against Halffloat bounds.

jatin-bhateja · 2022-06-16T01:53:12Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Halffloat.java

+        if (f < Halffloat.MIN_FLOAT_VALUE) return Halffloat.NEGATIVE_INFINITY;
+
+        int val = Float.floatToIntBits(f);
+        val = ((((val>>16)&0x8000)|((((val&0x7f800000)-0x38000000)>>13)&0x7c00)|((val>>13)&0x03ff)));


Kindly also add the link to paper.

jatin-bhateja · 2022-06-16T01:54:24Z

Initial version looks ok to me, we can do refinement in subsequent patches.

openjdk · 2022-06-16T01:56:50Z

@smita-kamath This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8277304: Java support for FP16

Reviewed-by: sviswanathan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the vectorIntrinsics+fp16 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jatin-bhateja, @sviswa7) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

smita-kamath · 2022-06-16T05:28:55Z

Reference to Fast float to halffloat conversion: http://fox-toolkit.org/ftp/fasthalffloatconversion.pdf

jatin-bhateja

Failures seen during jtreg, needs fix.

sviswa7

Looks good to me.

smita-kamath · 2022-06-22T22:54:44Z

/integrate

openjdk · 2022-06-22T22:56:02Z

@smita-kamath
Your change (at version 536b40a) is now ready to be sponsored by a Committer.

sviswa7 · 2022-06-22T23:19:32Z

/sponsor

openjdk · 2022-06-22T23:20:23Z

Going to push as commit 6b7b107.

openjdk · 2022-06-22T23:20:42Z

@sviswa7 @smita-kamath Pushed as commit 6b7b107.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

FP16 Java support

afa7567

smita-kamath changed the title ~~FP16 Java support~~ 8273322: Java support for FP16 Nov 17, 2021

Fixed space and tabs in code

cda8b06

smita-kamath changed the title ~~8273322: Java support for FP16~~ 8277304: Java support for FP16 Nov 17, 2021

smita-kamath marked this pull request as ready for review November 17, 2021 00:17

openjdk bot added the rfr Pull request is ready for review label Nov 17, 2021

smita-kamath added 2 commits June 2, 2022 09:45

Updated code as per review comment

89c872d

Modified gen-src to add Halffloat as new element type. This change re…

2479ce4

…moves redundant code from classes

openjdk bot removed the rfr Pull request is ready for review label Jun 13, 2022

jatin-bhateja reviewed Jun 13, 2022

View reviewed changes

smita-kamath added 5 commits June 13, 2022 13:34

changes to Halffloat class

102a4ee

Added new variable in gen-src that cleans up redundant code

84033ba

Code cleanup

0d391f7

Code cleanup

17aaf80

Changed file mode of gen-src.sh

ffb9a62

openjdk bot added the rfr Pull request is ready for review label Jun 14, 2022

jatin-bhateja reviewed Jun 15, 2022

View reviewed changes

Added handling for special cases

8e849c2

jatin-bhateja reviewed Jun 15, 2022

View reviewed changes

Changes to Halffloat class

4f5b106

jatin-bhateja reviewed Jun 16, 2022

View reviewed changes

jatin-bhateja approved these changes Jun 16, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 16, 2022

jatin-bhateja suggested changes Jun 16, 2022

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Jun 16, 2022

Fixing jtreg failures

536b40a

sviswa7 approved these changes Jun 22, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 22, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 22, 2022

openjdk bot added the integrated Pull request has been integrated label Jun 22, 2022

openjdk bot closed this Jun 22, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 22, 2022

		@@ -0,0 +1,112 @@
		/*
		* Copyright (c) 1994, 2017, Oracle and/or its affiliates. All rights reserved.

		if (!isFinite(f)) return Halffloat.POSITIVE_INFINITY;

		int val = Float.floatToIntBits(f);

Conversation

smita-kamath commented Nov 16, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 16, 2021

Uh oh!

mlbridge bot commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

LifeIsStrange commented Nov 24, 2021 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jatin-bhateja commented Feb 10, 2022

Uh oh!

jatin-bhateja commented Jun 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja commented Jun 16, 2022

Uh oh!

openjdk bot commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smita-kamath commented Jun 16, 2022

Uh oh!

jatin-bhateja left a comment

Choose a reason for hiding this comment

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

smita-kamath commented Jun 22, 2022

Uh oh!

openjdk bot commented Jun 22, 2022

Uh oh!

sviswa7 commented Jun 22, 2022

Uh oh!

openjdk bot commented Jun 22, 2022

Uh oh!

openjdk bot commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

smita-kamath commented Nov 16, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 17, 2021 •

edited

Loading

LifeIsStrange commented Nov 24, 2021 •

edited by bridgekeeper bot

Loading

jatin-bhateja Jun 13, 2022 •

edited

Loading

jatin-bhateja Jun 13, 2022 •

edited

Loading

jatin-bhateja Jun 15, 2022 •

edited

Loading

openjdk bot commented Jun 16, 2022 •

edited

Loading