Fix BigInteger.Rotate{Left,Right}

[kzrnm]

Fix #112854

• Fixed the logic of RotateRight.
• Fixed a bug that caused incorrect two’s complement computation for negative numbers with the most significant bit set in RotateRight and RotateLeft.

I reused the code from #112632.

Benchmark

Code

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using System.Numerics;

[MemoryDiagnoser(false)]
[HideColumns("Job", "Error", "StdDev", "Median", "RatioSD")]
public class RotateTest
{
    const int shiftSize = (1 << 23) + 13;
    BigInteger positive, negative;

    [GlobalSetup]
    public void Setup()
    {
        var bytes = new byte[1 << 25];
        new Random(227).NextBytes(bytes);
        positive = new BigInteger(bytes, isUnsigned: true);
        negative = -positive;
    }

    [Benchmark] public BigInteger PositiveRotateLeft() => BigInteger.RotateLeft(positive, shiftSize);
    [Benchmark] public BigInteger NegativeRotateLeft() => BigInteger.RotateLeft(negative, shiftSize);

    [Benchmark] public BigInteger PositiveRotateRight() => BigInteger.RotateRight(positive, shiftSize);
    [Benchmark] public BigInteger NegativeRotateRight() => BigInteger.RotateRight(negative, shiftSize);
}

BenchmarkDotNet v0.13.12, Windows 11 (10.0.26100.3194)
13th Gen Intel Core i5-13500, 1 CPU, 20 logical and 14 physical cores
.NET SDK 10.0.100-alpha.1.25077.2
  [Host]     : .NET 10.0.0 (10.0.25.7313), X64 RyuJIT AVX2
  Job-JFZQTT : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-RFISUH : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX2

Method	Toolchain	Mean	Ratio	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
PositiveRotateLeft	\main\corerun.exe	19.15 ms	1.00	156.2500	156.2500	156.2500	33 MB	1.00
PositiveRotateLeft	\pr\corerun.exe	14.10 ms	0.61	218.7500	218.7500	218.7500	32.69 MB	0.99
NegativeRotateLeft	\main\corerun.exe	19.35 ms	1.00	156.2500	156.2500	156.2500	33 MB	1.00
NegativeRotateLeft	\pr\corerun.exe	18.59 ms	0.96	218.7500	218.7500	218.7500	33.25 MB	1.01
PositiveRotateRight	\main\corerun.exe	14.93 ms	1.00	156.2500	156.2500	156.2500	32.5 MB	1.00
PositiveRotateRight	\pr\corerun.exe	14.07 ms	0.94	234.3750	234.3750	234.3750	32.73 MB	1.01
NegativeRotateRight	\main\corerun.exe	16.92 ms	1.00	156.2500	156.2500	156.2500	33 MB	1.00
NegativeRotateRight	\pr\corerun.exe	16.43 ms	0.97	218.7500	218.7500	218.7500	33.25 MB	1.01

[tannergooding]

Can this initially be done as a smaller fix that doesn't rewrite the whole algorithm?

We likely need to backport this fix and a less complex change is more desirable for that, as it reduces risk.

A separate PR that also rewrites it for improved performance just for .NET 10 is fine, just ideally separate from the bug fix.

[kzrnm]

#112878

[skyoxZ]

I don't think it should be backported.

1. What is the correct definition on RotateLeft/RotateRight of BigInteger? BigInteger doesn't have a fixed bits length and doesn't use 2's complement code to store data. I'd rather document the behavior as undefined.
2. The issue is said to have existed since .NET 7. Who using an old version .NET would want this new behavior?

[kzrnm]

This is functionally "by design".
There were two options possible, one of which always treated the bit sequence as the shortest possible two's complement representation and thus -1 is 1 while +1 is 01. The other of which is to treat it in terms of the underlying storage unit. The latter was chosen as it was overall more consistent with existing behavior, especially when viewed across all possible types.

Originally posted by @tannergooding in #91169

[tannergooding]

@skyoxZ, the backport isn't about changing the behavior; it's about fixing an edge case where the behavior is broken because RotateRight is effectively propagating the carry bit left instead of right in certain edge cases.

[skyoxZ]

As a user I can't predict the result of BigInteger.RotateLeft so I don't care if it's a bug. I just expect the same input always leads to the same result and believe .NET team is unlikely to change it. A backport will break my expectation.

[kzrnm]

The possible approaches for handling bitwise operations are as follows:

1. Do not implement this operations. Throw an exception if called via an interface.
2. Provide an explicit implementation based on a specific policy.
3. Implement it as a public method based on a specific policy (chosen).
4. Return an arbitrary, meaningless value (which is effectively the case for RotateRight).

Personally, I believe option 1 is the most desirable in an ideal scenario. An example of this approach is Java's BigInteger, which does not implement unsigned right shifts, stating that they "make little sense". @skyoxZ's argument seems to be based on the same idea.

However, changing to option 1 would be far too breaking and likely unfeasible.

@skyoxZ's stance (for the current version) aligns with option 4, advocating that breaking changes should be avoided. Breaking changes guideline - Bucket 2: Reasonable grey area
No one knows the specification, no one uses the method, and therefore no one would be affected even if its implementation has a bug.

On the other hand, even if no one knows the specification, it is reasonable to expect a public method to have a sensible implementation.

I will leave the final decision to .NET team.

[tannergooding]

Personally, I believe option 1 is the most desirable in an ideal scenario

1 isn't done because it breaks user expectations of how operating with binary integers in generic contexts works. It also prevents doing common and sensible optimizations, like treating division by 2 as shift operation or being able to permute the underlying bits.

The behavior chosen for 3 is then "sensible" for singular rotations and it behaves in a well defined way. Which is that functionally ROL(x, amount) will produce the same result as would happen for a standard integer x stored with a precision of n-bits. The biggest nuance is that ROR(ROL(x, amount), amount) may not return x since the stored precision might change and there's no way to track how many implied leading zero/sign bits were originally there.

The behavior is specifically that we determine the number of bits required for the "shortest" two's complement representation rounded up to the nearest 32-bits. We then rotate the number of bits specified. This produces a new BigInteger value and is therefore logically similar to had you manually done the two bit shifts as a single combined operation without increasing the backing storage. Because we produce a new BigInteger, that is then trimmed for storage and computation efficiency to remove unnecessary leading zero/sign bits, which is what can lead to not being able to recover the original value.

[tannergooding]

Because its meant to behave in a well-defined way, you can fully predict the answer for a given integer x. Because there is then a bug, it is applicable to apply a bug fix for the broken behavior especially as it is more likely to cause problems that it is returning an incorrect result.

[tannergooding]

@kzrnm were you not interested in taking the improvement here still, or was it no longer as meaningful with the other fix in?

[kzrnm]

@tannergooding The original PR #112632 aimed to optimize all bitwise operations, so I included shift operations as well and submitted a new pull request #113005.