Fix conversions from subnormal System.Half values #46536

tannergooding · 2021-01-04T17:57:13Z

This resolves #46515.

ghost · 2021-01-04T17:57:19Z

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #46515.

Author:	tannergooding
Assignees:	-
Labels:	`area-System.Numerics`
Milestone:	-

tannergooding · 2021-01-04T17:57:47Z

src/libraries/System.Private.CoreLib/src/System/Half.cs

@@ -683,10 +683,10 @@ private static double CreateDoubleNaN(bool sign, ulong significand)
        }

        private static float CreateSingle(bool sign, byte exp, uint sig)
-            => BitConverter.Int32BitsToSingle((int)(((sign ? 1U : 0U) << float.SignShift) | ((uint)exp << float.ExponentShift) | sig));
+            => BitConverter.Int32BitsToSingle((int)(((sign ? 1U : 0U) << float.SignShift) + ((uint)exp << float.ExponentShift) + sig));


The issue was that this was doing a bitwise or when it should have been using addition.

The original implementation was doing the correct thing: https://github.com/ucb-bar/berkeley-softfloat-3/blob/master/source/include/internals.h#L107

pgovind

Ah yup, my bad. LGTM

jeffhandley

We should submit this for 5.0 servicing approval. In the servicing template on the backport PR, please include notes for which boundary conditions we were previously testing and which ones we missed.

We will need to backport the test updates too (first), which can be done without approval.

ecorm · 2021-01-04T20:18:00Z

I've just uncovered another corner case where .NET's Half does not agree with http://half.sourceforge.net/ . When converting from Single(MinSubnormal/2) to Half (where MinSubnormal/2 is 0x33000000), the Sourceforge version makes it underflow to zero, whereas .NET makes it round up to Half.Epsilon. I'm not sure which is correct. I'm currently checking if @tannergooding 's fix makes both implementations agree.

ecorm · 2021-01-04T20:25:15Z

Ah yup, my bad. LGTM

I made the same mistake when I adapted http://half.sourceforge.net/ for my own use. I thought the + was an attempt to optimize a bitwise OR, but it turns out it really needs to perform addition for the rounding logic to work.

ecorm · 2021-01-04T20:29:08Z

I'm currently checking if @tannergooding 's fix makes both implementations agree.

Nope, they still disagree for the (Half)(BitConverter.Int32BitsToSingle(0x33000000)) case. Perhaps this should be a separate issue.

tannergooding · 2021-01-04T20:51:13Z

I've found the issue and a local fix, will push it up with a commit momentarily

…lues

tannergooding · 2021-01-04T21:11:05Z

src/libraries/System.Private.CoreLib/src/System/Half.cs

@@ -635,6 +635,7 @@ private static ushort RoundPackToHalf(bool sign, short exp, ushort sig)
                {
                    sig = (ushort)ShiftRightJam(sig, -exp);
                    exp = 0;
+                    roundBits = sig & 0xF;


The other issue was that this was missing adjusting the roundBits based on the updated sig.

The original implementation was doing the correct thing: https://github.com/ucb-bar/berkeley-softfloat-3/blob/b64af41c3276f97f0e181920400ee056b9c88037/source/s_roundPackToF16.c#L77

I imagine this was accidentally dropped when eliding the unnecessary exception handling logic.

ecorm · 2021-01-04T21:20:28Z

I'm not sure which is correct.

I think the Sourceforge implementation was correct for the MinSubnormal/2 case. Ties round to even, so UInt16BitsToHalf(0) being even would be the correct choice, whereas UInt16BitsToHalf(0x0001) would be odd.

Sorry if this all sounds elementary, but I've never needed to deal with the gory inner workings of IEEE floats before. 😄

ecorm · 2021-01-04T21:25:04Z

Feel free to borrow (and verify) some of my test vectors from here while you're at it: https://gist.github.com/ecorm/3b5963749752f2cc2f96fb493d29fcba

No attribution necessary (or wanted).

tannergooding · 2021-01-06T18:16:21Z

/backport to release/5.0

github-actions · 2021-01-06T18:17:21Z

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/466823164

github-actions · 2021-01-06T18:19:08Z

@tannergooding backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix conversions from subnormal System.Half values
error: sha1 information is lacking or useless (src/libraries/System.Private.CoreLib/src/System/Half.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix conversions from subnormal System.Half values
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* Fix conversions from subnormal System.Half values * Fix conversions from subnormal System.Single/Double to System.Half values * Fixing the HalfTests parameters

…to .NET 5.0 (#46641) * Half conversion test cases for subnormal corner cases (#46523) * Half conversion test cases for subnormal corner cases * Single/Double->Half rounding corner test cases * Single/Double->Half conversion subnormal rounding corner test cases * Fix conversions from subnormal System.Half values (#46536) * Fix conversions from subnormal System.Half values * Fix conversions from subnormal System.Single/Double to System.Half values * Fixing the HalfTests parameters Co-authored-by: Emile Cormier <ecorm@users.noreply.github.com>

Fix conversions from subnormal System.Half values

00dc015

Dotnet-GitSync-Bot added the area-System.Numerics label Jan 4, 2021

tannergooding commented Jan 4, 2021

View reviewed changes

pgovind approved these changes Jan 4, 2021

View reviewed changes

pgovind mentioned this pull request Jan 4, 2021

Half to Double/Float conversion is broken for subnormal values #46515

Closed

jeffhandley approved these changes Jan 4, 2021

View reviewed changes

Fix conversions from subnormal System.Single/Double to System.Half va…

1b99129

…lues

tannergooding commented Jan 4, 2021

View reviewed changes

Fixing the HalfTests parameters

a5984ea

tannergooding force-pushed the fix-46515 branch from 6ee4b0c to a5984ea Compare January 6, 2021 02:34

tannergooding merged commit 167c598 into dotnet:master Jan 6, 2021

tannergooding mentioned this pull request Jan 6, 2021

[Release/5.0] Port Fix conversions from subnormal System.Half values to .NET 5.0 #46641

Merged

ghost locked as resolved and limited conversation to collaborators Feb 5, 2021

tannergooding deleted the fix-46515 branch November 11, 2022 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix conversions from subnormal System.Half values #46536

Fix conversions from subnormal System.Half values #46536

tannergooding commented Jan 4, 2021

ghost commented Jan 4, 2021

tannergooding Jan 4, 2021

tannergooding Jan 4, 2021

pgovind left a comment

jeffhandley left a comment

ecorm commented Jan 4, 2021

ecorm commented Jan 4, 2021

ecorm commented Jan 4, 2021 •

edited

Loading

tannergooding commented Jan 4, 2021

tannergooding Jan 4, 2021

tannergooding Jan 4, 2021

ecorm commented Jan 4, 2021 •

edited

Loading

ecorm commented Jan 4, 2021 •

edited

Loading

tannergooding commented Jan 6, 2021

github-actions bot commented Jan 6, 2021

github-actions bot commented Jan 6, 2021

Fix conversions from subnormal System.Half values #46536

Fix conversions from subnormal System.Half values #46536

Conversation

tannergooding commented Jan 4, 2021

ghost commented Jan 4, 2021

tannergooding Jan 4, 2021

Choose a reason for hiding this comment

tannergooding Jan 4, 2021

Choose a reason for hiding this comment

pgovind left a comment

Choose a reason for hiding this comment

jeffhandley left a comment

Choose a reason for hiding this comment

ecorm commented Jan 4, 2021

ecorm commented Jan 4, 2021

ecorm commented Jan 4, 2021 • edited Loading

tannergooding commented Jan 4, 2021

tannergooding Jan 4, 2021

Choose a reason for hiding this comment

tannergooding Jan 4, 2021

Choose a reason for hiding this comment

ecorm commented Jan 4, 2021 • edited Loading

ecorm commented Jan 4, 2021 • edited Loading

tannergooding commented Jan 6, 2021

github-actions bot commented Jan 6, 2021

github-actions bot commented Jan 6, 2021

ecorm commented Jan 4, 2021 •

edited

Loading

ecorm commented Jan 4, 2021 •

edited

Loading

ecorm commented Jan 4, 2021 •

edited

Loading