-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Improve Math.BigMul performance on x64 #117261
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
d781b3a
90d2934
e110d32
69494cb
dae319b
a1ddb63
b41469b
c50082a
c18eb40
571d2c4
ef4bf17
ff84cf2
044f2eb
3d0909e
1d10451
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,23 @@ internal X64() { } | |
| /// </summary> | ||
| [Experimental(Experimentals.X86BaseDivRemDiagId, UrlFormat = Experimentals.SharedUrlFormat)] | ||
| public static (long Quotient, long Remainder) DivRem(ulong lower, long upper, long divisor) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> | ||
| /// <para>unsigned _umul128(unsigned __int64 Multiplier, unsigned __int64 Multiplicand, unsigned __int64 * HighProduct)</para> | ||
| /// <para> MUL reg/m64</para> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>Its functionality is exposed in the public <see cref="Math" /> class.</para> | ||
| /// </remarks> | ||
| internal static (ulong Lower, ulong Upper) BigMul(ulong left, ulong right) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> | ||
| /// <para> IMUL reg/m64</para> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>Its functionality is exposed in the public <see cref="Math" /> class.</para> | ||
| /// </remarks> | ||
| internal static (long Lower, long Upper) BigMul(long left, long right) { throw new PlatformNotSupportedException(); } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -109,6 +126,28 @@ internal X64() { } | |
| [Experimental(Experimentals.X86BaseDivRemDiagId, UrlFormat = Experimentals.SharedUrlFormat)] | ||
| public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> | ||
| /// <para> MUL reg/m32</para> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>Its functionality is exposed in the public <see cref="Math" /> class.</para> | ||
| /// </remarks> | ||
| internal static (uint Lower, uint Upper) BigMul(uint left, uint right) { throw new PlatformNotSupportedException(); } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need these if we don't use them and they're internal?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, they (int and nint overloads) can probably be removed since we do not have any such public api unless the corresponding API proposal is approved (#58263) They were there to follow the pattern of other hardware methods and because 32bit bigmul used them before implementing it in JIT instead. Comment on the other PR:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it means that NI_X86Base_BigMul should be removed as well ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd remove, it's basically a dead/untested code. Btw, I've pushed some AI-generated tests 🙂
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the 32bit and IntPtr (nint) overloads as well as NI_X86Base_BigMul . I did not look that much on the tests but thy seems fine (during developmen one error where register was swapped was detected by xxhash beeing used in tests and not by existing BigMul test. It seems like it should be covered by one of the new tests) |
||
|
|
||
| /// <summary> | ||
| /// <para> IMUL reg/m32</para> | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para>Its functionality is exposed in the public <see cref="Math" /> class.</para> | ||
| /// </remarks> | ||
| internal static (int Lower, int Upper) BigMul(int left, int right) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> MUL reg/m</summary> | ||
| internal static (nuint Lower, nuint Upper) BigMul(nuint left, nuint right) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> IMUL reg/m</summary> | ||
| internal static (nint Lower, nint Upper) BigMul(nint left, nint right) { throw new PlatformNotSupportedException(); } | ||
|
|
||
| /// <summary> | ||
| /// <para>void _mm_pause (void);</para> | ||
| /// <para> PAUSE</para> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.