Implement the sqrt, remainder, fma, scaleB, ilogB, bit increment/decrement, and quantize operations for the IEEE 754 decimal types#130956
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…imal types Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rement/BitDecrement operations IEEE 754-2019 5.1 requires general-computational operations to produce canonical results, so a returned NaN operand must be re-canonicalized. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…types Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
There was a problem hiding this comment.
Pull request overview
Adds additional IEEE 754 / IFloatingPointIeee754<TSelf>-related operations for System.Numerics.Decimal32, Decimal64, and Decimal128, and expands the existing Intel readtest.in outerloop harness + targeted unit tests to validate bit-exact behavior.
Changes:
- Exposes new public APIs on
Decimal32/Decimal64/Decimal128:Sqrt,Ieee754Remainder,FusedMultiplyAdd,ScaleB,ILogB,BitIncrement,BitDecrement,Quantize,Quantum,SameQuantum. - Implements the core algorithms in
Number.DecimalIeee754(including FMA, IEEE remainder, sqrt, scaleB/ilogB, quantize/quantum/sameQuantum, nextUp/nextDown helpers). - Adds/extends test coverage (inline bit-pattern tests + Intel vector gating and data enumeration).
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DecimalIeee754IntelTestData.cs | Adds Intel vector operation groups + enumerators for the new operations; adds ScaleB exponent-token parsing helper. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Decimal64Tests.cs | Adds unit tests and Intel-vector-backed theories for the newly exposed Decimal64 operations. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Decimal32Tests.cs | Adds unit tests and Intel-vector-backed theories for the newly exposed Decimal32 operations. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Decimal128Tests.cs | Adds unit tests and Intel-vector-backed theories for the newly exposed Decimal128 operations. |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Adds the new public surface area for Decimal32/Decimal64/Decimal128 in the reference assembly. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs | Wires new public static methods to Number.*DecimalIeee754 implementations. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs | Wires new public static methods to Number.*DecimalIeee754 implementations. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs | Wires new public static methods to Number.*DecimalIeee754 implementations. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs | Adds implementations for FMA, IEEE remainder, sqrt, scaleB/ilogB, quantize/quantum/sameQuantum, and nextUp/nextDown support utilities. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 1
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. It was produced by GitHub Copilot (Claude Opus 4.8) and independently cross-checked with two additional models (gpt-5.3-codex, gemini-3.1-pro-preview). Please treat its findings as assistive input for human review.
Holistic Review
Motivation: Justified. Continues the approved, tracked buildout of the IEEE 754-2019 decimal types (Decimal32/64/128) under the api-approved issue #81376, stacking on merged #130890. It adds the next batch of IFloatingPointIeee754<TSelf> operations plus the decimal-specific Quantize/Quantum/SameQuantum.
Approach: Sound and consistent with the codebase. Each operation is a careful port of the Intel BID reference (BSD-3, with the required attribution header), reuses the file's established Unpack/WideMultiply/NumberToDecimalIeee754BitsFromWide/DecimalIeee754FiniteNumberBinaryEncoding infrastructure, preserves preferred exponents, and canonicalizes NaN/results. Clarity is favored over Intel's reciprocal-multiply tricks, with those explicitly flagged as future perf work.
Summary: ✅ LGTM. New public surface is fully API-approved with exact signature/parameter-name matches and no extra surface; the algorithms handle special values and round-to-nearest-ties-to-even correctly; tests are comprehensive. No blocking issues were found, and two independent cross-check models reached the same conclusion. Final rounding-correctness assurance rests on the bit-exact Intel reference-vector runs in the outerloop harness, which a maintainer should confirm green before merge.
Detailed Findings
✅ API approval — verified against issue #81376
Issue #81376 carries api-approved. Its approved shape defines IDecimalFloatingPointIeee754<TSelf> with Quantize(TSelf x, TSelf y), Quantum(TSelf x), SameQuantum(TSelf x, TSelf y), plus the IFloatingPointIeee754 members. The ref/System.Runtime.cs additions match exactly — names, return types, and parameter names (x/y, left/right/addend, n). No unapproved surface is added; transcendentals are intentionally deferred (documented). The types don't yet declare the interface (incremental buildout), which is the correct way to ship a partial implementation.
✅ Numerical correctness — no high-confidence bugs (corroborated by 2 models)
- FusedMultiplyAdd: The
retain = 2·Precision + 2window guarantees the dominant operand is fully retained, sostickycan only come from the smaller operand — making the subtraction borrow (resultLow -= 1) rigorously correct. Exact cancellation (comparison == 0) provably impliessticky == false→+0. Zero-result sign (bothNegative),productZero/addendZeropadding, NaN order (y→z→x), andInf·0/Inf + (−Inf)invalids are all correct. Two-limb bounds hold. - Ieee754Remainder: Chunked modular reduction with correct ties-to-even; only the final step's quotient parity matters (earlier partials are scaled by positive powers of ten, hence even). Early
remainder == 0break is harmless since2·remainder == divisoris then impossible. Theelse(eb>ea) threshold andPower10(gap-1)·Power10(1)peel keep lookups in range; noremainder+remainderoverflow. - Sqrt / WideSqrt: 2-bits/iteration integer sqrt consumes the full double-width value with a clean even-
limbBitslow/high boundary; guard-digit scaling stays within two limbs;e >> 1handles odd/negative exponents natively. - ScaleB / BitIncrement / BitDecrement / Quantize / Quantum / SameQuantum:
(long)exponent math, gradual-underflow ties-to-even, precision-boundary carry/borrow,MAXFP→Inf, subnormal→signed-zero, andnextDown = −nextUp(−x)all verified.Power10(drop-1)is guarded (drop ≥ 1).
✅ The return z thread — already resolved
The earlier "non-canonical leak" concern on the product-zero path is correctly refuted: when addendZero is false, UnpackDecimalIeee754 guarantees the coefficient ≤ MaxSignificand (canonical), and z already carries the preferred exponent (min(prodExp, addExp) == addExp). Independently confirmed — no further action needed.
✅ Tests — strong, with full type parity
Always-on hand-computed [Theory]/[InlineData] cover edge cases (zeros, subnormals, carries, NaN payload canonicalization, invalids), layered under ConditionalTheory Intel reference-vector suites gated by IsAvailable for bit-exact validation. Row-skip rationale is documented per operation. Full parity across Decimal32/64/128; conventions followed.
💡 Non-blocking observations
- Performance:
WideSqrtis bit-by-bit and rounding uses direct division rather than Intel's reciprocal-multiply. Comments explicitly mark these as future optimizations; there is no regression to existing paths, which is acceptable for new correctness-first APIs. - CI: The one red leg (
Build windows-x86 Release Libraries_NET481) appears unrelated — a net481 build break inMicrosoft.Extensions.Caching.Abstractionstests (UnsafeAccessorAttributenot found), which this PR does not touch.
Implements the next batch of
IFloatingPointIeee754<TSelf>operations forDecimal32,Decimal64, andDecimal128, continuing #81376 on top of #130890.New public surface (per type):
SqrtIeee754RemainderFusedMultiplyAddScaleB/ILogBBitIncrement/BitDecrementQuantize/Quantum/SameQuantum(IEEE 754 §5.3.2, §5.7.3)All operations produce canonical BID results, preserve the preferred exponent where the spec defines one, and canonicalize NaN operands/results.
Non-trivial method bodies are based on the Intel(R) Decimal Floating-Point Math Library reference implementation (BSD 3-Clause) and carry the required attribution header.
Each operation is validated bit-exact against Intel's own
readtest.invectors through the existing outerloop Intel test harness (DecimalIeee754IntelTestData), with coverage added for the new operations.The transcendental operations (
Exp/Log/Sin/Cos/Pow, …) are intentionally deferred. Intel's reference does no argument reduction for large-argument trig and diverges heavily there, so those need a separate correctly-/faithfully-rounded strategy.The commits are kept separate per operation so this can be split into smaller PRs if preferred.
Note
This PR description was drafted by GitHub Copilot.