JDK-8289551: Conversions between bit representations of half precision values and floats #9422

jddarcy · 2022-07-08T06:11:22Z

Initial implementation.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request to be approved

Issues

JDK-8289551: Conversions between bit representations of half precision values and floats
JDK-8290216: Conversions between bit representations of half precision values and floats (CSR)

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer) ⚠️ Review applies to d54f9c94
John R Rose (@rose00 - Reviewer) ⚠️ Review applies to d54f9c94

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9422/head:pull/9422
$ git checkout pull/9422

Update a local copy of the PR:
$ git checkout pull/9422
$ git pull https://git.openjdk.org/jdk pull/9422/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9422

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9422.diff

…n values and floats

bridgekeeper · 2022-07-08T06:11:47Z

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

openjdk · 2022-07-08T06:13:43Z

@jddarcy The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

src/java.base/share/classes/java/lang/Float.java

jddarcy · 2022-07-13T05:52:00Z

Some notes in the current implementation in the PR: the impetus for the change is to provide a minimal level of platform support for the binary16 floating-point format defined in IEEE 754. This is done by added two methods to Float, one for binary16 -> float conversion and the other for float -> binary16 conversion. In the absence of the ability to define primitive types, short is used as a carrier for the bits of a binary16 value.

The conversion code in its current form favors readability over speed; a more performant software implementation may be possible even without intrinsification.

A different 16-bit floating-point format, bfloat, is popular in some circles. Any similar support for bfloat will be left for future work.

Please also review the companion CSR: https://bugs.openjdk.org/browse/JDK-8290216

mlbridge · 2022-07-13T06:06:23Z

Webrevs

src/java.base/share/classes/java/lang/Float.java

rgiulietti · 2022-07-13T12:22:34Z

src/java.base/share/classes/java/lang/Float.java

+                   // significand bits in the float and binary16
+                   // formats
+                   (bin16SignifBits << (FloatConsts.SIGNIFICAND_WIDTH - 11)));
+        return sign * Float.intBitsToFloat(result);


int result = (floatExpBits | // Shift left difference in the number of // significand bits in the float and binary16 // formats (bin16SignifBits << (FloatConsts.SIGNIFICAND_WIDTH - 11)));

avoids a useless | operation

rgiulietti · 2022-07-13T12:28:29Z

src/java.base/share/classes/java/lang/Float.java

+    // @IntrinsicCandidate
+    public static short floatToBinary16AsShortBits(float f) {
+        if (Float.isNaN(f)) {
+            // Arbitrary binary16 NaN value; could try to preserve the


// Arbitrary binary16 quiet NaN value; could try to preserve the

I double-checked the 754-2019 standard; unlike in some earlier versions of the standard, the 2019 edition does specify how a quiet vs signaling NaN is supposed to be encoded. Quoting from section 6.2.1:

"A quiet NaN bit string should be encoded with the first bit (d1) of the trailing significand
field T being 1. A signaling NaN bit string should be encoded with the first bit of the trailing significand
field being 0."

So the NaN bit pattern used is a quiet NaN.

In any case, I'll update this code to more faithfully pass along the bits of a Float NaN.

rgiulietti · 2022-07-13T12:38:23Z

src/java.base/share/classes/java/lang/Float.java

+        short sign_bit = (short)((doppel >> 16) & 0x8000);
+
+        // The overflow threshold is binary16 MAX_VALUE + 1/2 ulp
+        if (abs_f > (65504.0f + 16.0f) ) {


if (abs_f >= (65504.0f + 16.0f) ) {

Value exactly halfway must round to infinity.

Good catch. The rest of the code computed the right value, but the condition should be changed as suggested.

I increased test coverage around Binary16.MAX_VALUE+0.5*ulp(Binary16.MAX_VALUE).

rgiulietti · 2022-07-13T12:51:19Z

src/java.base/share/classes/java/lang/Float.java

+                // range of float).
+
+                // Significand bits as if using rounding to zero (truncation).
+                signif_bits = (short)((doppel & 0x0007f_e000) >>


signif_bits = (short)((doppel & 0x007f_e000) >>

or even

signif_bits = (short)((doppel & 0x007f_ffff) >>

32 bit hex are more readable when they have 8 hex digits

Oops; yes, I intended for the int hex constants to have at most 8 hex digits.

rgiulietti · 2022-07-13T13:11:33Z

src/java.base/share/classes/java/lang/Float.java

+            }
+
+            short result = 0;
+            result = (short)(((exp + 15) << 10) | signif_bits);


result = (short)(((exp + 15) << 10) + signif_bits);

The final exponent needs to be incremented when signif_bits == 0x400. The | is not enough for this to happen.

Good catch; I'll correct this bug in the next push.

I'll be adding a test to verify correct rounding of all the values in a binade - the set of floating-point values with the same exponent. That test reveals this issue.

rgiulietti · 2022-07-13T13:19:16Z

src/java.base/share/classes/java/lang/Float.java

+
+            short result = 0;
+            result = (short)(((exp + 15) << 10) | signif_bits);
+            return (short)(sign_bit | (0x7fff & result));


return (short)(sign_bit | result);

because result <= 0x7fff.

src/java.base/share/classes/java/lang/Float.java

rgiulietti · 2022-07-13T13:36:51Z

src/java.base/share/classes/java/lang/Float.java

+                // subnormals is [-140, -149]. Multiply abs_f down by
+                // 2^(-125) -- since (-125 = -149 - (-24)) -- so that
+                // the trailing bits of a subnormal float represent
+                // the correct trailing bits of a binary16 subnormal.


I would write intervals (ranges) in the form [low, high], so [-24, -15] and [-149, -140].

rgiulietti · 2022-07-13T14:14:04Z

src/java.base/share/classes/java/lang/Float.java

+                // the correct trailing bits of a binary16 subnormal.
+                exp = -15; // Subnormal encoding using -E_max.
+                float f_adjust = abs_f * 0x1.0p-125f;
+                signif_bits = (short)(Float.floatToRawIntBits(f_adjust) & 0x03ff);


I think the if and the exp++ can be avoided if the & 0x03ff is dropped altogether.

signif_bits = (short)Float.floatToRawIntBits(f_adjust);

The reason is the same as for the normalized case below: a carry will eventually flow into the representation for the exponent.

test/jdk/java/lang/Float/SixteenBitFormats.java

rgiulietti · 2022-07-13T15:15:00Z

test/jdk/java/lang/Float/SixteenBitFormats.java

+            }
+        }
+    }
+}


The last invocation to Integer.compare() is not correct.
For example, if bin16_1 = -1 and bin16_2 = 1, the invocation returns 1, which is incorrect.

jddarcy · 2022-07-18T20:33:30Z

I added NaN round-trip tests is a separate file since the properties being testing are not required by the specification, unlike the properties explored in the other tests. If the NaN tests need special handling on certain platforms/configurations, that can be don't without complicating the tests of most of the functionality.

rgiulietti · 2022-07-21T21:51:50Z

src/java.base/share/classes/java/lang/Float.java

+                int round =  doppel & 0x0000_1000;
+                int sticky = doppel & 0x0000_0fff;
+
+                if (round != 0 && (lsb != 0 || sticky != 0 )) {


You may want to reverse the order of (lsb != 0 || sticky != 0) to (sticky != 0 || lsb != 0) to improve the likelihood that the evaluation of || does not need to evaluate its right-hand side.
Statistically, sticky != 0 has a much higher chance to evaluate to true than lsb != 0.

src/java.base/share/classes/java/lang/Float.java

jddarcy · 2022-07-21T22:23:31Z

Thanks for the review comments thus far.

A few notes on the recently pushed update, corresponding to webrev 04.

The conversion methods have been renamed as suggested in comments received outside of the PR.

Thinking more about how a future possible value class for the binary16 format might be structured (java.math.FloatBinary16? java.math.Binary16?), even if such a class cannot extend java.lang.Number, it could still implement the conversion methods found on Number. In particular, a value class for binary16 could have a "floatValue" method as well as factory to convert from float to binary16. Therefore, there need not be any overloading concerns with the methods defined on Float for this issue.

(As an aside, it might be helpful to define a superinterface with the methods of java.lang.Number to better capture the ConvertibleToPrimitive API.)

The regression tests were augmented in several ways. One is by the introduction of an alternative implementation of the float -> binary16 conversion that uses the hardware support for float rounding to compute the proper low-order bits. This approach benchmarked as significantly slower than the existing bit-inspection approach so it is just used to double-check in testing. Before updating the PR, all 2^32 float values were converted to binary16 using both approaches and the results matched. Testing all 2^32 values all the time seems excessive, so in the PR just one exponent value is checked. For each exponent in normal range, the rounding is analagous at corresponding positions of the number line. Therefore, testing one exponent should be sufficient for testing all the different combination of bit that influence rounding.

PaulSandoz

I don't claim to be an expert in IEEE formats... the methods generally look good with thorough testing. Looking forward to the impact intrinsification will have!

PaulSandoz · 2022-07-22T22:45:47Z

test/jdk/java/lang/Float/Binary16Conversion.java

+ * @bug 8289551
+ * @summary Verify conversion between float and the binary16 format
+ * @library ../Math
+ * @build FloatConsts


Is this line necessary?

test/jdk/java/lang/Float/Binary16Conversion.java

test/jdk/java/lang/Float/Binary16ConversionNaN.java

openjdk · 2022-07-22T22:59:02Z

@jddarcy 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:

8289551: Conversions between bit representations of half precision values and floats

Reviewed-by: psandoz, jrose

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 3 new commits pushed to the master branch:

a6faf5d: 8290485: [vector] REVERSE_BYTES for byte type should not emit any instructions
0ca5cb1: 8290834: Improve potentially confusing documentation on collection of profiling information
7c3cfd1: 8290557: tools/jpackage/share/AddLauncherTest.java#id1 failed with "ERROR: Failed: Check icon file"

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

rose00

Looks good.

rgiulietti · 2022-07-23T16:38:34Z

src/java.base/share/classes/java/lang/Float.java

@@ -975,6 +976,215 @@ public static int floatToIntBits(float value) {
    @IntrinsicCandidate
    public static native float intBitsToFloat(int bits);

+    /**
+     * {@return the {@code float} value closest to the numerical value


{@return seems to be out of place and missing a closing }

The closing brace is on line 982, "... {@code short}}"

(Using the in-line form of the @return tag allows an explicit @return tag to be skipped by reusing the initial sentence. The generated javadoc is as intended.)

src/java.base/share/classes/java/lang/Float.java

rgiulietti · 2022-07-23T17:38:55Z

src/java.base/share/classes/java/lang/Float.java

+    }
+
+    /**
+     * {@return the floating-point binary16 value, encoded in a {@code


{@0return seems to be out of place and missing a closing }

src/java.base/share/classes/java/lang/Float.java

rgiulietti · 2022-07-23T18:15:27Z

src/java.base/share/classes/java/lang/Float.java

+
+        // Shift left difference in the number of significand bits in
+        // the float and binary16 formats
+        final int SIGNIF_SHIFT = (FloatConsts.SIGNIFICAND_WIDTH - 11);


Wouldn't it make more sense to declare this local as private static final? It could then even be used in the next method.

rgiulietti · 2022-07-23T18:55:03Z

src/java.base/share/classes/java/lang/Float.java

+            // Dealing with finite values in exponent range of
+            // binary16 (when rounding is done, could still round up)
+            int exp = Math.getExponent(f);
+            assert -25 <= exp && exp <= 15;


I think that both the subnormal and the normal case can be unified if we pay closer attention to the positions of the lsb, round and sticky bits in subnormals.

// Clamp exp to the [-15, 15] range while retaining the // difference between the original value and -15 on clamping. // This is the excess shift value in addition to 13. int expdelta = Math.max(0, -15 - exp); exp += expdelta; assert -15 <= exp && exp <= 15; int f_signif_bits = doppel & 0x007f_ffff; // original significand // Significand bits as if using rounding to zero (truncation). short signif_bits = (short)(f_signif_bits >> (13 + expdelta)); // For round to nearest even, determining whether or // not to round up (in magnitude) is a function of the // least significant bit (LSB), the next bit position // (the round position), and the sticky bit (whether // there are any nonzero bits in the exact result to // the right of the round digit). An increment occurs // in three cases: // // LSB Round Sticky // 0 1 1 // 1 1 0 // 1 1 1 // See "Computer Arithmetic Algorithms," Koren, Table 4.9 int lsb = f_signif_bits & (1 << 13 + expdelta); int round = f_signif_bits & (1 << 12 + expdelta); int sticky = f_signif_bits & ((1 << 12 + expdelta) - 1); if (round != 0 && ((lsb | sticky) != 0 )) { signif_bits++; } // No bits set in significand beyond the *first* exponent // bit, not just the sigificand; quantity is added to the // exponent to implement a carry out from rounding the // significand. assert (0xf800 & signif_bits) == 0x0; return (short)(sign_bit | ( ((exp + 15) << 10) + signif_bits ) );

I didn't test this variant, will do tomorrow when also reviewing the tests themselves.

The correct variant below passes the tests.

// For binary16 subnormals, beside forcing exp to -15, // retain the difference expdelta = E_min - exp. // This is the excess shift value, in addition to 13, to be used // in the computations below. // Further the (hidden) msb with value 1 in f must be involved as well. int expdelta = 0; int msb = 0x0000_0000; if (exp < -14) { expdelta = -14 - exp; exp = -15; msb = 0x0080_0000; } int f_signif_bits = doppel & 0x007f_ffff | msb; // Significand bits as if using rounding to zero (truncation). short signif_bits = (short)(f_signif_bits >> (13 + expdelta)); // For round to nearest even, determining whether or // not to round up (in magnitude) is a function of the // least significant bit (LSB), the next bit position // (the round position), and the sticky bit (whether // there are any nonzero bits in the exact result to // the right of the round digit). An increment occurs // in three cases: // // LSB Round Sticky // 0 1 1 // 1 1 0 // 1 1 1 // See "Computer Arithmetic Algorithms," Koren, Table 4.9 int lsb = f_signif_bits & (1 << 13 + expdelta); int round = f_signif_bits & (1 << 12 + expdelta); int sticky = f_signif_bits & ((1 << 12 + expdelta) - 1); if (round != 0 && ((lsb | sticky) != 0 )) { signif_bits++; } // No bits set in significand beyond the *first* exponent // bit, not just the sigificand; quantity is added to the // exponent to implement a carry out from rounding the // significand. assert (0xf800 & signif_bits) == 0x0; return (short)(sign_bit | ( ((exp + 15) << 10) + signif_bits ) );

Test classes look good.

rgiulietti · 2022-07-26T10:45:15Z

LGTM

jddarcy · 2022-07-26T16:53:34Z

/integrate

openjdk · 2022-07-26T16:54:47Z

Going to push as commit 7318b22.
Since your change was applied there have been 8 commits pushed to the master branch:

2ae8e31: 8290669: Fix wording in sun.security.ec
8939095: 8289996: Fix array range check hoisting for some scaled loop iv
da9cc5c: 8290806: Only add eager reclaim task to G1 post evacuate tasks if there were candidates
330adc0: 8290969: DumpClassListCLDClosure incorrectly uses ResizeableResourceHashtable
28bbdc5: 8290972: ProblemList java/lang/ProcessBuilder/PipelineLeaksFD.java
a6faf5d: 8290485: [vector] REVERSE_BYTES for byte type should not emit any instructions
0ca5cb1: 8290834: Improve potentially confusing documentation on collection of profiling information
7c3cfd1: 8290557: tools/jpackage/share/AddLauncherTest.java#id1 failed with "ERROR: Failed: Check icon file"

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-26T16:55:04Z

@jddarcy Pushed as commit 7318b22.

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

JDK-8289551: Conversions between bit representations of half precisio…

72fb42c

…n values and floats

jddarcy marked this pull request as draft July 8, 2022 06:11

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 8, 2022

jddarcy added 2 commits July 7, 2022 23:41

Add cardinal value tests.

4654eb7

Appease jcheck.

5c635c5

rose00 reviewed Jul 8, 2022

View reviewed changes

src/java.base/share/classes/java/lang/Float.java Outdated Show resolved Hide resolved

jddarcy added 7 commits July 8, 2022 16:36

Merge branch 'master' into JDK-8289551

e3f6eeb

Merge branch 'master' into JDK-8289551

ebb738c

Implement review feedback from John Rose.

ce0b73e

Initial implementation of float -> binary16 round-to-nearest with tests.

9d72e6c

Fix implementation thresholds; refine tests.

652b12a

Refine spec and tests.

2484587

Further refine spec.

399248d

jddarcy marked this pull request as ready for review July 13, 2022 05:44

openjdk bot added the rfr Pull request is ready for review label Jul 13, 2022

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 13, 2022

rgiulietti reviewed Jul 13, 2022

View reviewed changes

jddarcy added 7 commits July 14, 2022 09:46

Merge branch 'master' into JDK-8289551

383887b

Partial implementation of review feedback; test refinement.

fc35153

Improve NaN significand fidelity; refine tests.

d8e6296

Correct carry-out bug; add full binade test.

74eff2c

Respond to review feedback.

b09fdfc

Preserve NaN sign bit.

420a9f3

Add NaN test.

a30daf2

jddarcy added 3 commits July 20, 2022 22:24

Respond to review feedback; improve tests.

6c3faac

Merge branch 'master' into JDK-8289551

4342c12

Method rename.

9b06018

rgiulietti reviewed Jul 21, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Jul 21, 2022

Implement review feedback.

d54f9c9

PaulSandoz approved these changes Jul 22, 2022

View reviewed changes

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

rose00 approved these changes Jul 23, 2022

View reviewed changes

rgiulietti reviewed Jul 23, 2022

View reviewed changes

jddarcy added 5 commits July 25, 2022 10:30

Merge branch 'master' into JDK-8289551

e9b79bc

Implement review feedback.

6afaafb

Implement additional review feedback.

c4ded35

Refactor equivalent() implementation.

39cb541

Adopt alternative implementaiton from review.

f012d1d

openjdk bot added the integrated Pull request has been integrated label Jul 26, 2022

openjdk bot closed this Jul 26, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 26, 2022

uschindler mentioned this pull request Oct 4, 2023

Make byte[] vector comparisons faster! (if possible) apache/lucene#12621

Closed

jddarcy deleted the JDK-8289551 branch October 27, 2024 18:30

JDK-8289551: Conversions between bit representations of half precision values and floats #9422

JDK-8289551: Conversions between bit representations of half precision values and floats #9422

Conversation

jddarcy commented Jul 8, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 8, 2022

openjdk bot commented Jul 8, 2022

jddarcy commented Jul 13, 2022

mlbridge bot commented Jul 13, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jddarcy commented Jul 18, 2022

Choose a reason for hiding this comment

jddarcy commented Jul 21, 2022

PaulSandoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 22, 2022 • edited Loading

rose00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgiulietti commented Jul 26, 2022

jddarcy commented Jul 26, 2022

openjdk bot commented Jul 26, 2022

openjdk bot commented Jul 26, 2022

jddarcy commented Jul 8, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 13, 2022 •

edited

Loading

openjdk bot commented Jul 22, 2022 •

edited

Loading