Fix incorrect rounding in Math.Round and MathF.Round with digits#130574
Conversation
Math.Round(value, digits, mode) previously scaled the input by 10^digits,
rounded, then unscaled. The scaling step is inexact, so values just below a
decimal midpoint (e.g. 655.924999999999954525... for the literal 655.925) were
turned into an exact midpoint and rounded the wrong way, and large magnitudes
lost their fractional bits entirely before rounding.
Round the exact value of the input instead, using the internal Number.BigInteger
machinery, and convert the correctly rounded decimal result back to the nearest
representable value via NumberToFloat. This matches the value already produced by
the F{digits} format and is correct for all finite inputs.
The threshold below which a fractional portion is possible is now the exact
integer boundary (2^52 for double, 2^23 for float); at or above it every
representable value is already an integer.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR changes the implementation of Math.Round(double, digits, mode) and MathF.Round(float, digits, mode) to compute results based on the exact IEEE-754 input value (rather than rounding after an inexact value * 10^digits scaling step), and adds a fast path intended to match the reference result while avoiding arbitrary-precision work in common cases.
Changes:
- Replace the
Round(value * 10^digits) / 10^digitsapproach withNumber.TryRoundToDecimalDigitsFastand aNumber.RoundToDecimalDigitsfallback to ensure correctly rounded results for the exact input. - Tighten
MidpointRoundingvalidation for thedigitsoverloads and add targeted tests that assert exact bitwise results for corrected cases. - Add a new CoreLib implementation file (
System/Number.Rounding.cs) and wire it into the shared CoreLib project items.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/MathF.cs | Adds exact-value rounding test vectors and invalid MidpointRounding validation tests for MathF.Round(..., digits, mode). |
| src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/Math.cs | Adds exact-value rounding test vectors and invalid MidpointRounding validation tests for Math.Round(..., digits, mode). |
| src/libraries/System.Private.CoreLib/src/System/Number.Rounding.cs | Introduces reference rounding via exact decomposition + BigInteger and a fast path (FMA / integer) intended to match the reference results. |
| src/libraries/System.Private.CoreLib/src/System/MathF.cs | Updates MathF.Round(float, digits, mode) to validate mode, apply the integer boundary check, and call the new Number helpers. |
| src/libraries/System.Private.CoreLib/src/System/Math.cs | Updates Math.Round(double, digits, mode) similarly to use the new rounding helpers and integer boundary check. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Adds the new System\\Number.Rounding.cs file to the shared CoreLib compilation inputs. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 4
The exact BigInteger-based routine is only needed when scaling the input by 10^digits cannot be done exactly. Compute |value| * 10^digits as an exact hi + lo double-double via a fused-multiply-add; 10^digits is exactly representable across the supported digits range, so lo recovers the single rounding folded into hi exactly. From that exact scaled value we determine the correctly rounded integer part directly (using an exact two-sum residual to compare against the midpoint) and materialize the result with a single correctly rounded division, so long as the rounded integer is exactly representable. Fall back to RoundToDecimalDigits otherwise (namely once the scaled value reaches the integer boundary). This produces bit-identical results to the exact routine (validated against a BigInteger reference over 20M+ inputs across all digits and modes) while avoiding the arbitrary precision arithmetic for the common in-range cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
db3d4de to
2f20899
Compare
…load Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The vectorized scale-round-unscale path produced results that diverged from the now-corrected scalar T.Round at the decimal midpoints. Defer to the scalar implementation for every element so the two agree on all hardware; a correctly rounded vectorized path is left as a future improvement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Assert.Equal(double, double, precision) rounds both operands via Math.Round, which is now exact, so two values that differ by a single ulp can straddle a 14th-decimal boundary and round differently. Compare against an absolute variance instead, which is equivalent in strictness without the rounding-boundary fragility. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The 0-15 (double) / 0-6 (float) cap was an artifact of the old scale-by-10^digits approach. The exact routine is correct for any digit count, so accept any non-negative digits and only throw for negative values. Counts the fast path can't handle route straight to the exact RoundToDecimalDigits, which gains a no-op gate that returns the value unchanged once it is an exact multiple of 10^-digits -- this also bounds the arbitrary-precision work so the BigInteger and digit buffer stay within their fixed capacities. Consolidate the now-identical MathF throw helper and resource string into the shared non-negative variant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
I am supportive of this change in .NET 11, favoring correctness going forward over bugward compatibility of incorrect math. We generally have the position of making fixes to math operations for improved correctness. |
The scalar T.Round(x, digits, mode) no longer caps digits, so the float/double-specific 6/15 limits here diverged from the documented contract that this overload computes T.Round element-wise. Since the digit path already falls back to the scalar operator for every element, drop the type-specific caps in favor of a single up-front non-negative check that applies to all T, matching the mode validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The fast-path gate was carried over from the old public cap (15 double / 6 float), but the FMA and integer paths stay correctly rounded as long as 10^digits is exact. That holds through 19 digits for double -- bounded by the integer fallback scaling by (ulong)10^digits -- and 10 for float, bounded by 10^digits being exactly representable as a float. Raise the gate to those true maxima so digits 16-19 (double) and 7-10 (float) take the fast path instead of the arbitrary-precision routine; larger counts still fall back to it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Set DOTNET_EnableHWIntrinsic through the environment indexer rather than Add, which throws if the variable is already present in the parent environment. Skip vectors that can never reach the integer fast path (digits == 0, magnitudes at or above the integer boundary, or digit counts beyond the fast-path range) so the remote run stays focused on the fallback and avoids redundant arbitrary-precision work. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jeffhandley
left a comment
There was a problem hiding this comment.
Good with me, but I'd prefer to get a re-review from @eiriktsarpalis before we merge as I don't have enough context for the new commits today.
|
/ba-g unrelated failure already fixed on main |
Fixes #1643
Math.Round(double, digits, mode)andMathF.Round(float, digits, mode)computedRound(value * 10^digits, mode) / 10^digits. Becausevalue * 10^digitsis generally not exactly representable, values slightly below (or above) a decimal midpoint could be scaled into an exact midpoint (or across a rounding boundary), producing an incorrectly rounded result. Large magnitudes could also lose their fractional bits entirely during the scaling step. Roughly 5% of random inputs across the supporteddigitsrange differed from the correctly rounded result.(
655.925is stored as655.924999999999954525…, which is below the655.925midpoint, so the correct result is655.92.)Commit 1 — correctness. The result is now computed from the exact value of the input and is the nearest representable value to the correctly rounded decimal result, matching what
value.ToString("F{digits}")already produced. The reference path uses arbitrary precision arithmetic over the exactmantissa * 2^exponentdecomposition. Mode validation was also tightened.Commit 2 — fast path. A fast path that avoids the arbitrary precision arithmetic while producing the exact same correctly rounded result, falling back to the reference path when it cannot guarantee exactness.
|value| * 10^digitsis formed as an exacthi + lodouble-double viaFusedMultiplyAdd; the correctly rounded integer part is then determined directly and materialized with a single correctly rounded division while the scaled value stays below the point where every value is already an integer. This path is shared generically by bothfloatanddouble.FusedMultiplyAdddegrades to slow software emulation, the fast path instead uses exact integer arithmetic (ulong/UInt128fordouble,ulongforfloat).The
Fma.IsSupported || AdvSimd.Arm64.IsSupportedguard is referenced directly so it folds to a JIT constant at its single use site.This is a behavioral break; a breaking-change doc will be filed against dotnet/docs. Tests were added covering the corrected results, subnormal/large-shift inputs, and invalid
MidpointRoundingvalues; all Math/MathF tests pass on both the FMA and integer-fallback paths (the latter validated withDOTNET_EnableAVX2=0).Note
This PR description was drafted by GitHub Copilot on @tannergooding's behalf.