Skip to content

Remove unsafe code from System.Numerics.BigIntegerCalculator.ShiftRot#127847

Open
EgorBo wants to merge 2 commits intodotnet:mainfrom
EgorBo:reduce-unsafe-bigint-shift
Open

Remove unsafe code from System.Numerics.BigIntegerCalculator.ShiftRot#127847
EgorBo wants to merge 2 commits intodotnet:mainfrom
EgorBo:reduce-unsafe-bigint-shift

Conversation

@EgorBo
Copy link
Copy Markdown
Member

@EgorBo EgorBo commented May 6, 2026

Note

This PR is AI-generated.

+5b asm diffs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 6, 2026 00:05
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 6, 2026

@MihuBot

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors System.Runtime.Numerics' internal BigIntegerCalculator shift/rotate implementation to remove direct LoadUnsafe/StoreUnsafe usage from the SIMD paths, which sit underneath BigInteger shift and rotate operations.

Changes:

  • Reworks LeftShiftSelf to use sliced spans plus Vector128/256/512.Create and CopyTo instead of ref+offset-based vector loads/stores.
  • Applies the same span-based vector load/store pattern to RightShiftSelf.
  • Changes loop bookkeeping from integer offsets to shrinking Span<nuint> windows while keeping the existing carry/scalar fallback structure.

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 6, 2026

Note

AI-generated benchmark.

@EgorBot -arm -amd

using System;
using System.Numerics;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

public class Bench
{
    private BigInteger _value;

    [Params(128, 1024, 8192)]
    public int Bits { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var rng = new Random(42);
        byte[] bytes = new byte[Bits / 8];
        rng.NextBytes(bytes);
        bytes[^1] &= 0x7F; // ensure positive
        _value = new BigInteger(bytes);
    }

    [Benchmark]
    public BigInteger LeftShift() => _value << 7;

    [Benchmark]
    public BigInteger RightShift() => _value >> 7;
}

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 7, 2026

@EgorBot -arm -amd

using System;
using System.Numerics;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

public class Bench
{
    private BigInteger _value;

    [Params(128, 1024, 8192)]
    public int Bits { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var rng = new Random(42);
        byte[] bytes = new byte[Bits / 8];
        rng.NextBytes(bytes);
        bytes[^1] &= 0x7F; // ensure positive
        _value = new BigInteger(bytes);
    }

    [Benchmark]
    public BigInteger LeftShift() => _value << 7;

    [Benchmark]
    public BigInteger RightShift() => _value >> 7;
}

@EgorBo EgorBo marked this pull request as ready for review May 7, 2026 12:28
Copilot AI review requested due to automatic review settings May 7, 2026 12:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


ref nuint start = ref MemoryMarshal.GetReference(bits);
int offset = bits.Length;
Span<nuint> data = bits;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using a different local needed to get the good codegen (vs. just using bits)?
I guess it's the same as for the method below with remaining.

Vector128<nuint> current = Vector128.Create(remaining) >> shift;
Vector128<nuint> carries = Vector128.Create(remaining.Slice(1)) << back;
Vector128<nuint> current = Vector128.Create((ReadOnlySpan<nuint>)remaining) >> shift;
Vector128<nuint> carries = Vector128.Create((ReadOnlySpan<nuint>)remaining.Slice(1)) << back;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all these extra casts needed?


nuint carry2 = 0;
for (int i = 0; i < offset; i++)
for (int i = 0; i < data.Length; i++)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we similarly delete the else branch in this method too as was done for RightShiftSelf?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants