Fix BigInteger.LeadingZeroCount, PopCount, TrailingZeroCount, RotateLeft, and RotateRight for platform-independent 32-bit word semantics#126259
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
|
@tannergooding, is the .NET 10 behavior that uses the upper 32-bit word what we want? |
There was a problem hiding this comment.
Pull request overview
This PR restores platform-independent, 32-bit word–based semantics for BigInteger.LeadingZeroCount after the internal _bits storage migrated from uint[] to nuint[], and adds/updates tests to lock in the intended behavior.
Changes:
- Update
BigInteger.LeadingZeroCountto always compute LZC using 32-bit word semantics (including on 64-bit platforms). - Fix existing generic-math test expectations to be platform-independent (32/31 for Zero/One).
- Add a dedicated
LeadingZeroCountTestssuite and include it in the Numerics test project.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs |
Adjusts LeadingZeroCount to use 32-bit word semantics for both _sign and _bits representations. |
src/libraries/System.Runtime.Numerics/tests/BigIntegerTests.GenericMath.cs |
Updates assertions to expect platform-independent results for LeadingZeroCount(Zero/One). |
src/libraries/System.Runtime.Numerics/tests/BigInteger/LeadingZeroCountTests.cs |
Adds coverage for boundary and platform-independence scenarios (small/large, positive/negative). |
src/libraries/System.Runtime.Numerics/tests/System.Runtime.Numerics.Tests.csproj |
Includes the newly added LeadingZeroCountTests.cs in the test build. |
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.Numerics/tests/BigInteger/LeadingZeroCountTests.cs:67
- Same as above: this parse call would be clearer and more consistent with the surrounding test suite if it used
using System.Globalization;+NumberStyles.HexNumberinstead of theGlobalization.qualifier.
// Parse the magnitude as a positive hex value (leading zero keeps high bit clear),
// then negate it so the result is negative and stored in _bits.
BigInteger magnitude = BigInteger.Parse(hexMagnitude, Globalization.NumberStyles.HexNumber);
BigInteger value = -magnitude;
This comment has been minimized.
This comment has been minimized.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ac91600f-e8a6-44ef-adcb-9f02df235fb0 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…into GenericMath Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/82e43e2a-a6a2-4c40-ad57-ffaa52c2cc18 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…rd semantics - PopCount _sign path: nint.PopCount -> int.PopCount (fixes platform-dependent results for small negative values like -1 returning 64 on 64-bit vs 32 on 32-bit) - PopCount _bits negative path: replace inline two's complement with formula PopCount(2^W - m) = W - PopCount(m) - TZC(m) + 1 using 32-bit word width W (fixes ~nuint filling upper 32 bits with 1s on 64-bit) - TrailingZeroCount _sign path: nint.TrailingZeroCount -> int.TrailingZeroCount (fixes TZC(0) returning 64 on 64-bit vs 32 on 32-bit) - LeadingZeroCount: replace magic constant 32 with BitsPerUInt32 - Add comprehensive tests for all three methods covering _sign path, _bits path, large positive/negative values, and platform-independence invariants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the Environment.Is64BitProcess branch with the simpler BitOperations.LeadingZeroCount(value._bits[^1]) & 31 expression. The & 31 maps 64-bit LZC to 32-bit word semantics: when the upper half is zero, LZC is 32 + uint_lzc, and (32 + x) & 31 == x. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
b0ca7bb to
d9c38b9
Compare
This comment has been minimized.
This comment has been minimized.
|
@copilot also address RotateLeft/RotateRight and any other similarly affected methods on BigInteger. Ensure comprehensive tests are added/updated in a manner consistent with the other APIs in this PR. |
…pdate tests Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/0c384115-0987-41ef-b9d1-34ac2bb67e7d Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Fixed I also verified that Note: The pre-existing |
@copilot, can you elaborate? If it wasn't platform-dependent before #125799, then I need you to fix it in this PR. |
Remove the "32" suffix from RotateLeft32, RotateRight, SwapUpperAndLower32, LeftShiftSelf32, and RightShiftSelf32 since the nuint-based versions no longer exist. Add clearer documentation explaining that the rotation operates on a Span<uint> view of the nuint[] buffer, with shift operations delegating to the SIMD-accelerated nuint overloads while carry extraction and digit swapping operate at 32-bit word granularity. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f2568909-19c7-4a74-9b63-a849eee3dce8 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
…r, 32-bit uses nuint path, 64-bit uses uint view with inline rotation Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/fc887d25-0f15-446b-b265-1ff699c595ee Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
…ke nuint RotateLeft/Right overloads private Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/fc887d25-0f15-446b-b265-1ff699c595ee Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
…wer feedback On 64-bit with even word count: standard nuint rotation, absorbing odd 32-bit digit shifts into the nuint small shift (0..63). On 64-bit with odd word count (half-used last limb): nuint SIMD shift with carry fixup, uint-level digit swap only where the swap boundary may fall mid-nuint. Two's complement conversion uses NumericsHelpers.DangerousMakeTwosComplement at nuint level with half-limb clearing. Removed LeftShiftSelf(Span<uint>) and RightShiftSelf(Span<uint>) since the bit-shift now operates at nuint level for all cases. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e72bd9b4-df13-46c5-8180-72d9cc2022f2 Co-authored-by: tannergooding <10487869+tannergooding@users.noreply.github.com>
|
This is still not as small of a diff as it should be, but I'm tired of fighting with copilot on the simpler fix for Rotate. I'll follow up to remove the unnecessary code duplication in a separate PR. |
|
/ba-g unrelated mono ios timeout |
After the
_bitsmigration fromuint[]tonuint[](#125799), severalIBinaryInteger<BigInteger>methods returned platform-dependent results. This PR restores consistent 32-bit word semantics for all five affected methods, including both the_signpath and the_bitspath.Description
Changes
LeadingZeroCount:_signpath:nint.LeadingZeroCount→uint.LeadingZeroCount((uint)value._sign)_bitspath:BitOperations.LeadingZeroCount(value._bits[^1]) & 31to map to 32-bit word semanticsPopCount:_signpath:nint.PopCount(value._sign)→int.PopCount(value._sign)PopCount(-1)returning 64 on 64-bit vs 32 on 32-bit_bitsnegative path: Uses standardnuinttwo's complement with MSL (most-significant limb) upper-32-bit correction. On 64-bit, the MSL's upper 32 bits may contain sign-extension 1s that don't correspond to real 32-bit words, so the correction subtractsBitOperations.PopCount(bits[^1] >> BitsPerUInt32) * (BitsPerLimb - BitsPerUInt32)to account for those phantom bits. UsesIndexOfAnyExcept((nuint)0)for efficient first non-zero limb detection.TrailingZeroCount:_signpath:nint.TrailingZeroCount(value._sign)→int.TrailingZeroCount(value._sign)TrailingZeroCount(BigInteger.Zero)returning 64 on 64-bit vs 32 on 32-bitRotateLeft:_signpath:BitOperations.RotateLeft((nuint)value._sign, rotateAmount)→uint.RotateLeft((uint)value._sign, rotateAmount)withnew BigInteger((int)rs)/new BigInteger(rs)result constructionRotateLeft(BigInteger.One, 32)returning2^32on 64-bit vs1on 32-bit_bitspath: The rotation operates directly onnuintlimbs using the original SIMD-accelerated algorithm. On 64-bit, a last-index fixup handles the edge case where the upper 32 bits of the final limb are zero (positive values) or the upper 33 bits are all ones (negative values after two's complement), indicating those bits are "extra" compared touint[]storage. When the effective 32-bit word count is odd,SwapUpperAndLowerusesMemoryMarshal.Cast<nuint, uint>for the digit swap at the mid-nuint boundary, while all shift operations use the existing SIMD-acceleratednuintoverloads directly. On 32-bit,nuint==uintso the algorithm is unchanged.RotateRight:_signpath:BitOperations.RotateRight((nuint)value._sign, rotateAmount)→uint.RotateRight((uint)value._sign, rotateAmount)withnew BigInteger((int)rs)/new BigInteger(rs)result constructionRotateRight(BigInteger.One, 1)returning2^63on 64-bit vs2^31on 32-bit_bitspath: SameRotate()approach as RotateLeft (shared helper)Performance: The rotation
_bitspath operates directly onnuintlimbs, preserving full 64-bit throughput with SIMD-accelerated shifts. Only theSwapUpperAndLowerdigit-swap step uses aSpan<uint>view (viaMemoryMarshal.Cast) when the swap boundary falls mid-nuint on 64-bit with an odd word count. TheRotate()helper works directly in the final resultnuint[]buffer allocated viaRentedBuffer, avoiding any temporary array allocation.Endianness safety: On big-endian 64-bit platforms, the
SwapUpperAndLowerhelper swaps the two 32-bit halves within eachnuintlimb before and after performing uint-level digit swapping, ensuring correct word order regardless of endianness. On 32-bit platforms,nuintanduintare the same size so no special handling is needed.BigIntegerCalculatorhelpers:RotateLeft/RotateRight/SwapUpperAndLower/LeftShiftSelf/RightShiftSelfoverloads are used as the primary rotation API, with full Vector128/256/512 SIMD acceleration for shiftsSwapUpperAndLowerusesMemoryMarshal.Cast<nuint, uint>for digit swapping when the swap boundary falls mid-nuint on 64-bit with an odd word countSwapUpperAndLowerusesRentedBufferfor temporary storage, matching the pattern used elsewhere in BigIntegerTests: Comprehensive platform-independent test cases added to existing
BigIntegerTests.GenericMath.csfor all five methods, covering_signpath values,_bitspath boundary values (32-bit word boundaries), and negative multiword values including mixed-bit cases exercising the two's complement PopCount formula. All pre-existingRotate.cstests updated from platform-dependentnint.Size * 8to fixed 32-bit semantics.⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.