valhalla icon indicating copy to clipboard operation
valhalla copied to clipboard

8334432: Refine Float16.fma

Open jddarcy opened this issue 1 year ago • 12 comments

Adding comments and test cases for Float16.fma.


Progress

  • [x] Change must not contain extraneous whitespace

Issue

Reviewers

Contributors

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1143

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1143.diff

Webrev

Link to Webrev Comment

jddarcy avatar Jun 21 '24 04:06 jddarcy

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

bridgekeeper[bot] avatar Jun 21 '24 04:06 bridgekeeper[bot]

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

8334432: Refine Float16.fma

Co-authored-by: Raffaello Giulietti <[email protected]>
Reviewed-by: jbhateja, rgiulietti

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 lworld+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) 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).

openjdk[bot] avatar Jun 21 '24 04:06 openjdk[bot]

I've asked my colleague @rgiulietti to help with the error analysis of the current implementation.

The commented-out code in the fma for an inexact product-sum does get executed for the expected test cases. If special-case code for an inexact product-sum turns out need to be needed, this extra code will be deleted before a push.

jddarcy avatar Jun 21 '24 04:06 jddarcy

The analysis done here seems correct, but there is an implicit suspension moment in the last case.

That leaves possibly non-exact product-sum with a combination of product in the subnormal range of Float16 and the c term to be added in being not-small. However, if this product-sum is non-exact, the smaller term from the product, with at most 22 exponent bit positions set, and the the 11 bits from c being summed in, must be separated by at least 53 - (22 + 11) = 20 bit positions otherwise the product-sum would fit in a double. I believe this implies at least one of the double-rounding scenarios cannot occur, in particular a half-way result in the smaller precision, Float16 in this case, rounding differently because sticky bit information from the higher precision was rounded away.

Here's a further analysis of this case.

Double rounding is usually harmless. It is harmful only in two situations:

  • The first rounding from the exact value to the extended precision (here double) happens to be directed toward 0 to a value exactly midway between two adjacent working precision (here float16) values, followed by a second rounding from there which again happens to be directed toward 0 to one of these values (the one with lesser magnitude). A single rounding from the exact value to the working precision, in contrast, rounds to the value with larger magnitude.
  • Symmetrically, the first rounding to the extended precision happens to be directed away from 0 to a value exactly midway between two adjacent working precision values, followed by a second rounding from there which again happens to be directed away from 0 to one of these values (the one with larger magnitude). However, a single rounding from the exact value to the working precision rounds to the value with lesser magnitude.

In any other case double rounding is innocuous, returning the same value as a single rounding to the working precision. We only need to ensure that the first rounding to double does not produce the midpoint of two adjacent float16 values.

  • If a·b and c have the same sign, the sum a·b + c has a significand with a large gap of 20 or more 0s between the bits of the signifcand of c to the left (at most 11 bits) and those of the product a·b to the right (at most 22 bits). The rounding bit for the final working precision of float16 is the leftmost 0 in the gap.

    • If rounding to double is directed toward 0, all the 0s in the gap are preserved, thus the float16 rounding bit is unaffected and remains 0. This means that the double value is not the midpoint of two adjacent float16 values, so double rounding is harmless.
    • If rounding to double is directed away form 0, the rightmost 0 in the gap might be replaced by a 1, but the others are unaffected, including the float16 rounding bit. Again, this shows that the double value is not the midpoint of two adjacent float16 values, and double rounding is innocuous.
  • If a·b and c have opposite signs, the sum a·b + c the long gap of 0s above is replaced by a long gap of 1s. The float16 rounding bit is the leftmost 1 in the gap, or the second leftmost 1 iff c is a power of 2. In both cases, the rounding bit is followed by at least another 1.

    • If rounding to double is directed toward 0, the float16 rounding bit and its follower are preserved and both 1, so the double value is not the midpoint of two adjacent float16 values, and double rounding is harmless.
    • If rounding to double is directed away from 0, the float16 rounding bit and its follower are either preserved (both 1), or both switch to 0. Either way, the double value is again not the midpoint of two adjacent float16 values, and double rounding is harmless.

rgiulietti avatar Jun 21 '24 14:06 rgiulietti

/contributor @rgiulietti

jddarcy avatar Jun 21 '24 18:06 jddarcy

@jddarcy Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <[email protected]>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

openjdk[bot] avatar Jun 21 '24 18:06 openjdk[bot]

/contributor add @rgiulietti

jddarcy avatar Jun 21 '24 18:06 jddarcy

@jddarcy Contributor Raffaello Giulietti <[email protected]> successfully added.

openjdk[bot] avatar Jun 21 '24 18:06 openjdk[bot]

The analysis done here seems correct, but there is an implicit suspension moment in the last case.

That leaves possibly non-exact product-sum with a combination of product in the subnormal range of Float16 and the c term to be added in being not-small. However, if this product-sum is non-exact, the smaller term from the product, with at most 22 exponent bit positions set, and the the 11 bits from c being summed in, must be separated by at least 53 - (22 + 11) = 20 bit positions otherwise the product-sum would fit in a double. I believe this implies at least one of the double-rounding scenarios cannot occur, in particular a half-way result in the smaller precision, Float16 in this case, rounding differently because sticky bit information from the higher precision was rounded away.

Here's a further analysis of this case.

Double rounding is usually harmless. It is harmful only in two situations:

* The first rounding from the exact value to the extended precision (here `double`) happens to be directed _toward_ 0 to a value exactly midway between two adjacent working precision (here `float16`) values, followed by a second rounding from there which again happens to be directed _toward_ 0 to one of these values (the one with lesser magnitude).
  A single rounding from the exact value to the working precision, in contrast, rounds to the value with larger magnitude.

* Symmetrically, the first rounding to the extended precision happens to be directed _away_ from 0 to a value exactly midway between two adjacent working precision values, followed by a second rounding from there which again happens to be directed _away_ from 0 to one of these values (the one with larger magnitude).
  However, a single rounding from the exact value to the working precision rounds to the value with lesser magnitude.

In any other case double rounding is innocuous, returning the same value as a single rounding to the working precision. We only need to ensure that the first rounding to double does not produce the midpoint of two adjacent float16 values.

* If a·b and c have the same sign, the sum a·b + c has a significand with a large gap of 20 or more 0s between the bits of the signifcand of c to the left (at most 11 bits) and those of the product a·b to the right (at most 22 bits).
  The rounding bit for the final working precision of `float16` is the leftmost 0 in the gap.
  
  * If rounding to `double` is directed toward 0, all the 0s in the gap are preserved, thus the `float16` rounding bit is unaffected and remains 0. This means that the `double` value is _not_ the midpoint  of two adjacent `float16` values, so double rounding is harmless.
  * If rounding to `double` is directed away form 0, the rightmost 0 in the gap might be replaced by a 1, but the others are unaffected, including the `float16` rounding bit. Again, this shows that the `double` value is _not_ the midpoint  of two adjacent `float16` values, and double rounding is innocuous.

* If a·b and c have opposite signs, the sum a·b + c the long gap of 0s above is replaced by a long gap of 1s. The `float16` rounding bit is the leftmost 1 in the gap, or the second leftmost 1 iff c is a power of 2. In both cases, the rounding bit is followed by at least another 1.
  
  * If rounding to `double` is directed toward 0, the `float16` rounding bit and its follower are preserved and both 1, so the `double` value is _not_ the midpoint  of two adjacent `float16` values, and double rounding is harmless.
  * If rounding to `double` is directed away from 0, the `float16` rounding bit and its follower are either preserved (both 1), or both switch to 0. Either way, the `double` value is again _not_ the midpoint  of two adjacent `float16` values, and double rounding is harmless.

Thanks; that agrees with the sketch of those cases I was working on. I'll update the explanatory comment and cleanup the implementation accordingly.

jddarcy avatar Jun 21 '24 20:06 jddarcy

Getting back to this PR, I pushed an update to the explanation about why Float16 fma is double arithmetic is sufficient and an initial update of the tests.

More test augmentation as previously discussed on the way.

jddarcy avatar Jul 31 '24 21:07 jddarcy

Hi @jddarcy , Some queries / comments.

Hi @jatin-bhateja ,

Please re-review the current state of the PR and if it looks good, I'm integrate it for sponsorship. Thanks.

jddarcy avatar Aug 01 '24 20:08 jddarcy

Thanks for the review comments @rgiulietti ; the most recent push should address the points you raised.

jddarcy avatar Aug 12 '24 15:08 jddarcy

@jatin-bhateja , if you re-review the current state of the PR, I'll integrate the changes so they can be sponsored. Thanks.

jddarcy avatar Aug 12 '24 15:08 jddarcy

/integrate

jddarcy avatar Aug 13 '24 16:08 jddarcy

@jddarcy Your change (at version 2d357510c6f14f2284ccb126f711877788e16402) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Aug 13 '24 16:08 openjdk[bot]

/sponsor

jatin-bhateja avatar Aug 14 '24 07:08 jatin-bhateja

Going to push as commit be93540f9bed7b18e0e788f9635782554415d920.

openjdk[bot] avatar Aug 14 '24 07:08 openjdk[bot]

@jatin-bhateja @jddarcy Pushed as commit be93540f9bed7b18e0e788f9635782554415d920.

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

openjdk[bot] avatar Aug 14 '24 07:08 openjdk[bot]