Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EgorBo wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −31
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7cbbe8
Remove unsafe code from BigIntegerCalculator.ShiftRot
EgorBo May 6, 2026
48d686b
Merge branch 'main' into reduce-unsafe-bigint-shift
EgorBo May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 33 additions & 31 deletions src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.ShiftRot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,50 +153,52 @@ public static void LeftShiftSelf(Span<nuint> bits, int shift, out nuint carry)
{
carry = bits[^1] >> back;

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

@MihaZupan MihaZupan May 7, 2026

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.


// Each vector load needs one extra element below it to source the carry bits
// that shift in from the lower limbs, hence the +1 minimum.
while (Vector512.IsHardwareAccelerated && offset >= Vector512<nuint>.Count + 1)
// that shift in from the lower limbs, hence data.Length > Count (not >=).
while (Vector512.IsHardwareAccelerated && data.Length > Vector512<nuint>.Count)
{
Vector512<nuint> current = Vector512.LoadUnsafe(ref start, (nuint)(offset - Vector512<nuint>.Count)) << shift;
Vector512<nuint> carries = Vector512.LoadUnsafe(ref start, (nuint)(offset - (Vector512<nuint>.Count + 1))) >> back;
int tailStart = data.Length - Vector512<nuint>.Count;
Vector512<nuint> current = Vector512.Create((ReadOnlySpan<nuint>)data.Slice(tailStart)) << shift;
Vector512<nuint> carries = Vector512.Create((ReadOnlySpan<nuint>)data.Slice(tailStart - 1, Vector512<nuint>.Count)) >> back;
Comment thread
EgorBo marked this conversation as resolved.

Comment thread
EgorBo marked this conversation as resolved.
Vector512<nuint> newValue = current | carries;

Vector512.StoreUnsafe(newValue, ref start, (nuint)(offset - Vector512<nuint>.Count));
offset -= Vector512<nuint>.Count;
newValue.CopyTo(data.Slice(tailStart));
data = data.Slice(0, tailStart);
}

while (Vector256.IsHardwareAccelerated && offset >= Vector256<nuint>.Count + 1)
while (Vector256.IsHardwareAccelerated && data.Length > Vector256<nuint>.Count)
{
Vector256<nuint> current = Vector256.LoadUnsafe(ref start, (nuint)(offset - Vector256<nuint>.Count)) << shift;
Vector256<nuint> carries = Vector256.LoadUnsafe(ref start, (nuint)(offset - (Vector256<nuint>.Count + 1))) >> back;
int tailStart = data.Length - Vector256<nuint>.Count;
Vector256<nuint> current = Vector256.Create((ReadOnlySpan<nuint>)data.Slice(tailStart)) << shift;
Vector256<nuint> carries = Vector256.Create((ReadOnlySpan<nuint>)data.Slice(tailStart - 1, Vector256<nuint>.Count)) >> back;

Vector256<nuint> newValue = current | carries;

Vector256.StoreUnsafe(newValue, ref start, (nuint)(offset - Vector256<nuint>.Count));
offset -= Vector256<nuint>.Count;
newValue.CopyTo(data.Slice(tailStart));
data = data.Slice(0, tailStart);
}

while (Vector128.IsHardwareAccelerated && offset >= Vector128<nuint>.Count + 1)
while (Vector128.IsHardwareAccelerated && data.Length > Vector128<nuint>.Count)
{
Vector128<nuint> current = Vector128.LoadUnsafe(ref start, (nuint)(offset - Vector128<nuint>.Count)) << shift;
Vector128<nuint> carries = Vector128.LoadUnsafe(ref start, (nuint)(offset - (Vector128<nuint>.Count + 1))) >> back;
int tailStart = data.Length - Vector128<nuint>.Count;
Vector128<nuint> current = Vector128.Create((ReadOnlySpan<nuint>)data.Slice(tailStart)) << shift;
Vector128<nuint> carries = Vector128.Create((ReadOnlySpan<nuint>)data.Slice(tailStart - 1, Vector128<nuint>.Count)) >> back;

Vector128<nuint> newValue = current | carries;

Vector128.StoreUnsafe(newValue, ref start, (nuint)(offset - Vector128<nuint>.Count));
offset -= Vector128<nuint>.Count;
newValue.CopyTo(data.Slice(tailStart));
data = data.Slice(0, tailStart);
}

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

@MihaZupan MihaZupan May 7, 2026

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?

{
nuint value = carry2 | bits[i] << shift;
carry2 = bits[i] >> back;
bits[i] = value;
nuint value = carry2 | data[i] << shift;
carry2 = data[i] >> back;
data[i] = value;
}
}
else
Expand Down Expand Up @@ -227,32 +229,32 @@ public static void RightShiftSelf(Span<nuint> bits, int shift, out nuint carry)

Span<nuint> remaining = bits;

while (Vector512.IsHardwareAccelerated && remaining.Length >= Vector512<nuint>.Count + 1)
while (Vector512.IsHardwareAccelerated && remaining.Length > Vector512<nuint>.Count)
{
Vector512<nuint> current = Vector512.Create(remaining) >> shift;
Vector512<nuint> carries = Vector512.Create(remaining.Slice(1)) << back;
Vector512<nuint> current = Vector512.Create((ReadOnlySpan<nuint>)remaining) >> shift;
Vector512<nuint> carries = Vector512.Create((ReadOnlySpan<nuint>)remaining.Slice(1)) << back;

Comment thread
EgorBo marked this conversation as resolved.
Vector512<nuint> newValue = current | carries;

newValue.CopyTo(remaining);
remaining = remaining.Slice(Vector512<nuint>.Count);
}

while (Vector256.IsHardwareAccelerated && remaining.Length >= Vector256<nuint>.Count + 1)
while (Vector256.IsHardwareAccelerated && remaining.Length > Vector256<nuint>.Count)
{
Vector256<nuint> current = Vector256.Create(remaining) >> shift;
Vector256<nuint> carries = Vector256.Create(remaining.Slice(1)) << back;
Vector256<nuint> current = Vector256.Create((ReadOnlySpan<nuint>)remaining) >> shift;
Vector256<nuint> carries = Vector256.Create((ReadOnlySpan<nuint>)remaining.Slice(1)) << back;

Vector256<nuint> newValue = current | carries;

newValue.CopyTo(remaining);
remaining = remaining.Slice(Vector256<nuint>.Count);
}

while (Vector128.IsHardwareAccelerated && remaining.Length >= Vector128<nuint>.Count + 1)
while (Vector128.IsHardwareAccelerated && remaining.Length > Vector128<nuint>.Count)
{
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

@MihaZupan MihaZupan May 7, 2026

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?


Vector128<nuint> newValue = current | carries;

Expand Down
Loading