-
Notifications
You must be signed in to change notification settings - Fork 11
Improve Mulmod perf #80
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.
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
Conversation
There was a problem hiding this 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 pull request refactors the UInt256 implementation to improve the performance of modular arithmetic operations, particularly Mulmod. The changes include reorganizing code into separate partial class files for better maintainability and potentially optimizing the underlying implementations.
Key changes:
- Reorganized
UInt256implementation by splitting into multiple partial class files (core logic, operators, constructors, and conversions) - Modified the scalar multiplication implementation (
MultiplyScalar) to use a more efficient algorithm - Added exception handling for division by zero in modular arithmetic operations
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
UInt256.cs |
Removed large sections of code (moved to new partial files); updated multiplication and carry logic; added constant Len; disabled AVX-512 multiplication path |
UInt256.Operators.cs |
New file containing all operator overloads and type conversion operators previously in main file |
UInt256.Ctors.cs |
New file containing constructors and factory methods previously in main file |
UInt256.Conversions.cs |
New file containing conversion and parsing methods previously in main file |
UInt256Tests.cs |
Updated tests to check for DivideByZeroException and ArgumentException in modular arithmetic operations with zero modulus |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Fallback to old code for |
|
Added specialized path
|
|
|
||
| qhat--; | ||
| } | ||
| public int CompareTo(object? obj) => obj is not UInt256 int256 ? throw new InvalidOperationException() : CompareTo(int256); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be compared to other number types?
| // y != 0 | ||
| // x > y | ||
|
|
||
| if (x.IsUint64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is somewhat redundant with x.IsZero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you what you mean? Will have returned already if x.IsZero; so different check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So both checks, are testing some same thing to some extent.
Like we could check IsUint64, IsZero and IsOne only once and save few instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 3 test 0 on last 3 fields and differ only on test on 1st field
| if (y.IsZero) ThrowDivideByZeroException(); | ||
| if (x.IsZero || y.IsOne) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y.IsZero and y.IsOne are somewhat redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x.IsZero or y.IsOne; different variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same variables on different lines
| if (m.IsZero) ThrowDivideByZeroException(); | ||
| if (m.IsOne) | ||
| { | ||
| // Any value mod 1 is mathematically 0. | ||
| res = default; | ||
| return; | ||
| } | ||
|
|
||
| // Compute 257-bit sum S = x + y as 5 limbs (s0..s3, s4=carry) | ||
| bool overflow = AddOverflow(in x, in y, out UInt256 sum); | ||
| ulong s4 = !overflow ? 0UL : 1UL; | ||
|
|
||
| if (m.IsUint64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m.IsZero, m.IsOne and m.IsUint64 are somewhat redundant? They check same fields for most.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using BitLen is slower
uint modBits = (uint)m.BitLen;
uint xBits = (uint)x.BitLen;
uint yBits = (uint)y.BitLen;| else if (m.u3 != 0) | ||
| { | ||
| Remainder257By256Bits(in sum, in m, out res); | ||
| } | ||
| else if (m.u2 != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again comparing m fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't really find a better way that measurably shows up
| if (m.IsZero) ThrowDivideByZeroException(); | ||
| if (m.IsOne || x.IsZero || y.IsZero) | ||
| { | ||
| res = default; | ||
| return; | ||
| } | ||
|
|
||
| // Trivial no-mul cases first. | ||
| if (y.IsOne) { Mod(in x, in m, out res); return; } | ||
| if (x.IsOne) { Mod(in y, in m, out res); return; } | ||
|
|
||
| // Modulus-size dispatch first - keeps all the tiny-mod magic. | ||
| if (m.IsUint64) | ||
| { | ||
| MulModBy64Bits(in x, in y, m.u0, out res); | ||
| return; | ||
| } | ||
|
|
||
| if ((m.u2 | m.u3) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same redundant compares?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is > 128bit check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will measure leading zeros as single check 🤔
|
Anything more for? |
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
Yes but wasn't great for the amount of additional code added. Are opportunities there, will revisit |
Uh oh!
There was an error while loading. Please reload this page.