Updating BigInteger to implement IBinaryInteger and ISignedNumber #68964

tannergooding · 2022-05-06T14:45:26Z

No description provided.

dotnet-issue-labeler · 2022-05-06T14:45:34Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-05-06T14:45:37Z

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Numerics`, `new-api-needs-documentation`
Milestone:	-

stephentoub · 2022-05-06T14:51:37Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+                for (int i = 0; i < value._bits.Length; i++)
+                {
+                    uint part = value._bits[i];
+                    result += uint.PopCount(part);


Would it be worth reinterpreting _bits as ulongs and doing a PopCount on each of those (plus any additional uint that remains)?

It would probably be better to use nuint (so 32-bit perf stays good), but there is a more general rewrite of BigInteger planned at some point and I think it would be better to do that then rather than complicate the current logic more.

The general goal of the rewrite is to make BigInteger use nuint[] (rather than uint[]) and make it two's complement, rather than one's complement representation

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

GrabYourPitchforks · 2022-05-06T23:36:45Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+            int byteCount = (value._bits is null) ? sizeof(int) : (value._bits.Length * 4);
+
+            // Normalize the rotate amount to drop full rotations
+            rotateAmount %= (int)(byteCount * 8L);


Is this guaranteed to handle overflow (negative byteCount) correctly? Some very limited testing says yes, but I wondered if you knew if this was a solid guarantee.

I believe so, although its possible there is some edge case.

More concretely we have APIs that allow getting the underlying bytes into an array or span, and so long term I want to limit _bits.Length to be no more than Array.MaxLength bytes. This ensures you can always fit the underlying data into a span and 2^2_147_483_591 should be more than big enoguh for anyone.

The shifting and rotate APIs are then somewhat "oddities" in that they currently take int but theoretically we allow up to Array.MaxLength * 8 bits, which is currently up to 17,179,868,728 bits (requiring up to 34 bits).

If we limit the actual value to no more than int.MaxLength bits so these APIs continue to make sense then we allow BigInteger up to 268_435_455 bytes or approximately 256 MB. This allows an astronomically large number of approx: 7.15 * 10^80_807_123.

I'm not necessarily "hardset" on either of these limits, but I think they are practical and realistic for .NET. Elsewise we need "chunking" APIs and various APIs may throw or not function as expected in all scenarios.

Added handling to ensure that _bits.Length <= (Array.Length / sizeof(uint)) and to throw an OverflowException otherwise.

Changed the modulus logic to then be rotateAmount = (int)(rotateAmount % (byteCount * 8L))

GrabYourPitchforks · 2022-05-06T23:40:03Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+        }
+
+        /// <inheritdoc cref="IBinaryInteger{TSelf}.LeadingZeroCount(TSelf)" />
+        public static BigInteger LeadingZeroCount(BigInteger value)


If you change BigInteger to use nint[] instead of int[] in a future PR (per your comment at https://github.com/dotnet/runtime/pull/68964/files#r866933097), you'll need to normalize the result of this method across platforms. Otherwise the results could differ on x86 vs x64.

Yes. However, I believe that's fine/acceptable.

Various types, including user defined ones, may be variable sized. As such, LeadingZeroCount is logically a value between 0 and GetByteCount() * 8L (put another way it is (GetByteCount() * 8L) - GetShortestBitLength().

This is functionally similar to how nint, nuint, CLong, and CULong work as well and gives a well-defined contract even in the face of this. The value is then still usable within these limits/expectations.

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

GrabYourPitchforks · 2022-05-06T23:47:42Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+                {
+                    // When the value is positive, we simply need to copy all bits as little endian
+
+                    ref byte address = ref MemoryMarshal.GetReference(destination);


No need for GetReference in the common little-endian case. If the length check on line 3308 above succeeds, you know that MemoryMarshal.Cast<uint, byte>(bits).CopyTo(destination) will also succeed. The only time you'd need to bswap is in the big-endian case.

Once your two's-complement implementation comes online, you'll be able to reuse the same memcpy code there.

I'm going to intentionally keep this simple for now and handle it as part of the rewrite.

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

GrabYourPitchforks · 2022-05-06T23:53:55Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+        }
+
+        /// <inheritdoc cref="INumber{TSelf}.MaxMagnitude(TSelf, TSelf)" />
+        public static BigInteger MaxMagnitude(BigInteger x, BigInteger y)


Perf: This method could end up allocating as part of the temporary -x and -y calculations, even though the results are discarded. Is this acceptable?

BigInteger, in general, allocates all over the place today. I'm not overly concerned about this but I definitely could simplify it a bit more.

dakersnar

Looks good! Only skimmed the test, but they look robust.

dakersnar · 2022-05-10T22:22:13Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

@@ -603,6 +621,8 @@ private BigInteger(Span<uint> value)

        public static BigInteger MinusOne { get { return s_bnMinusOneInt; } }

+        internal static int MaxLength => Array.MaxLength / sizeof(uint);


Is this restriction designed so that we can hold every BigInteger in an array of bytes?

Yes and based on the feedback from API review we are going to restrict this further such that MaxLength => Array.MaxLength / (sizeof(uint) * 8)

That is, it is expected that any BigInteger be no more than approx 2 billion bits.

As per the summary I gave above (#68964 (comment))

This is up to 268_435_455 bytes or approximately 256 MB. This still allows an astronomically large number of approx: 7.15 * 10^80_807_123 at which point

Most conceivable code will fit well into 80_807_100 (80 million) digits provided and anything beyond is increasingly niche and starts having very complex considerations into how it impacts the GC, memory usage in general, etc.

dakersnar · 2022-05-10T22:24:45Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

@@ -1766,6 +1786,31 @@ private static BigInteger Subtract(ReadOnlySpan<uint> leftBits, int leftSign, Re
            return new BigInteger(value);
        }

+        public static implicit operator BigInteger(nint value)


Is this operator keyword referring to the cast operation?

Yes. Most operators are of the form static TResult operator ... where the ... is the operator name (+, -, *, /, %, etc; including the new checked variants checked +, etc).

Implicit and explicit conversion operators take a different shape of the form static implicit operator TResult and static explicit operator TResult (with the checked keyword still following the operator keyword, so static explicit operator checked TResult).

dakersnar · 2022-05-10T22:34:45Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

+        //
+
+        /// <inheritdoc cref="IAdditionOperators{TSelf, TOther, TResult}.op_Addition(TSelf, TOther)" />
+        static BigInteger IAdditionOperators<BigInteger, BigInteger, BigInteger>.operator checked +(BigInteger left, BigInteger right) => left + right;


Is this needed now that checked + has a DIM in IAdditionOperators with this behavior?

It won't be once the DIM support is available for use by the language. As of right now, its not in or usable at the moment so we still need to define this ourselves.

uweigand · 2022-05-16T09:26:36Z

Hi @tannergooding, the new test is failing on big-endian s390x:
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-main-2b43123da88d4ef296/System.Runtime.Numerics.Tests/1/console.a4a405ef.log?helixlogtype=result

Seems like there could be again endian issues? In particular, it seems odd that TryWriteLittleEndian apparently byte-swaps positive numbers, but not negative numbers ...

tannergooding · 2022-05-16T12:49:00Z

This is fixed in #69391.

tannergooding added 2 commits May 5, 2022 15:37

Updating BigInteger to implement the generic math interfaces

7baf6bf

Adding generic math tests for BigInteger

590b307

ghost assigned tannergooding May 6, 2022

dotnet-issue-labeler bot added area-System.Numerics new-api-needs-documentation labels May 6, 2022

tannergooding requested a review from dakersnar May 6, 2022 14:47

stephentoub reviewed May 6, 2022

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs Show resolved Hide resolved

GrabYourPitchforks reviewed May 6, 2022

View reviewed changes

GrabYourPitchforks mentioned this pull request May 7, 2022

Perf: JIT does not optimize pattern "~x + 1" #69003

Closed

tannergooding added 2 commits May 8, 2022 08:03

Addressing PR feedback

7454978

Merge remote-tracking branch 'dotnet/main' into generic-math-biginteger

6fc476b

dakersnar approved these changes May 10, 2022

View reviewed changes

GrabYourPitchforks approved these changes May 11, 2022

View reviewed changes

tannergooding merged commit b4f46ac into dotnet:main May 11, 2022

KalleOlaviNiemitalo mentioned this pull request Jun 7, 2022

Implement INumber<T> for System.Numerics.BigInteger and System.Numerics.Complex #70404

Closed

ghost locked as resolved and limited conversation to collaborators Jun 15, 2022

tannergooding deleted the generic-math-biginteger branch November 11, 2022 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating BigInteger to implement IBinaryInteger and ISignedNumber #68964

Updating BigInteger to implement IBinaryInteger and ISignedNumber #68964

tannergooding commented May 6, 2022

dotnet-issue-labeler bot commented May 6, 2022

ghost commented May 6, 2022

stephentoub May 6, 2022

tannergooding May 6, 2022

GrabYourPitchforks May 6, 2022

tannergooding May 7, 2022

tannergooding May 8, 2022

GrabYourPitchforks May 6, 2022

tannergooding May 7, 2022

GrabYourPitchforks May 6, 2022

tannergooding May 7, 2022

GrabYourPitchforks May 6, 2022

tannergooding May 7, 2022

dakersnar left a comment

dakersnar May 10, 2022

tannergooding May 11, 2022

tannergooding May 11, 2022

dakersnar May 10, 2022

tannergooding May 11, 2022

dakersnar May 10, 2022

tannergooding May 11, 2022

uweigand commented May 16, 2022

tannergooding commented May 16, 2022

		@@ -603,6 +621,8 @@ private BigInteger(Span<uint> value)

		public static BigInteger MinusOne { get { return s_bnMinusOneInt; } }

		internal static int MaxLength => Array.MaxLength / sizeof(uint);

Updating BigInteger to implement IBinaryInteger and ISignedNumber #68964

Updating BigInteger to implement IBinaryInteger and ISignedNumber #68964

Conversation

tannergooding commented May 6, 2022

dotnet-issue-labeler bot commented May 6, 2022

ghost commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakersnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uweigand commented May 16, 2022

tannergooding commented May 16, 2022