Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[release/3.0] Updating Math.Round and MathF.Round to be IEEE compliant so that the intrinsic and managed form are deterministic.#26017

Merged
wtgodbe merged 2 commits into
dotnet:release/3.0from
tannergooding:backport-fix-25857
Aug 7, 2019
Merged

[release/3.0] Updating Math.Round and MathF.Round to be IEEE compliant so that the intrinsic and managed form are deterministic.#26017
wtgodbe merged 2 commits into
dotnet:release/3.0from
tannergooding:backport-fix-25857

Conversation

@tannergooding
Copy link
Copy Markdown
Member

Description

The managed implementations of the Math.Round functions were not IEEE compliant and would return a different result between Debug and Release mode. In .NET Core 3 with the tiered JIT support they now return a different result for the first 29 calls of the function before the JIT recognizes them as "hot" and they are rejitted.

Customer Impact

Customers calling Math.Round or MathF.Round may get different behavior between two consecutive calls to the functions.

Regression?

Yes, as per the description.

Risk

Low. This is pulling in a port of a known good implementation from the BSD SoftFloat library. It was additionally validated that all 4 billion inputs for the single-precision input range are returning the correct values as compared to the intrinsic forms of these APIs (which compile down to the roundss and roundsd instructions).

…intrinsic and managed form are deterministic. (#25901)

* Updating Math.Round and MathF.Round to be IEEE compliant so that the intrinsic and managed form are deterministic.

* Fixing the Math.Round and MathF.Round handling for values greater than 0.5 and less than 1.0

* Applying formatting patch.
@tannergooding
Copy link
Copy Markdown
Member Author

This is a backport of #25901

@tannergooding
Copy link
Copy Markdown
Member Author

CC. @CarolEidt, @AndyAyersMS, @jkotas, @danmosemsft

I added one additional commit, which addresses @CarolEidt's request that we document that only the roundToNearestTiesToEven code path (see the request here: https://github.com/dotnet/coreclr/pull/25901/files/e248d2232ef0930583982053e19923c4edfd9d51#diff-3ad59bdf390b0bd6aa8fda8d4e7e3a6a)

@tannergooding tannergooding added this to the 3.0 milestone Aug 5, 2019
Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@danmoseley
Copy link
Copy Markdown
Member

Bug exposed by enabling tiering by default in 3.0

@danmoseley
Copy link
Copy Markdown
Member

Approved by tactics for 3.0, @wtgodbe or @Anipik will merge when branch opens for preview 9.

@stephentoub stephentoub changed the title Updating Math.Round and MathF.Round to be IEEE compliant so that the intrinsic and managed form are deterministic. [release/3.0] Updating Math.Round and MathF.Round to be IEEE compliant so that the intrinsic and managed form are deterministic. Aug 7, 2019
@wtgodbe
Copy link
Copy Markdown
Member

wtgodbe commented Aug 7, 2019

Branch is open, merging

@wtgodbe wtgodbe merged commit f879b52 into dotnet:release/3.0 Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants