Improve Math.BigMul performance on x64 #117261
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR backports the Math.BigMul hardware intrinsic support on x64 without the mulx instruction extension. It adds new BigMul overloads in the X86Base APIs, hooks them into Math.BigMul, and extends the JIT to lower, schedule, and codegen these multi-register intrinsics.
- Introduces
BigMulmethods for 32-, 64-, and pointer-size integers inX86Baseand their platform-not-supported stubs. - Updates
Math.BigMulto prefer the newX86Base.X64.BigMulpath on non-MONO x64, falling back as before. - Enhances JIT (linear scan, lowering, import, list, codegen, tree layout) to recognize and generate
BigMulintrinsics.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/X86Base.cs | Added BigMul intrinsics for various operand widths |
| src/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/X86Base.PlatformNotSupported.cs | Added BigMul stubs throwing on unsupported platforms |
| src/System.Private.CoreLib/src/System/Math.cs | Routed Math.BigMul to use the new intrinsics on x64 |
| src/coreclr/jit/lsraxarch.cpp | Updated register allocator for BigMul multi-reg defs |
| src/coreclr/jit/lowerxarch.cpp | Enabled containment checks for BigMul |
| src/coreclr/jit/hwintrinsicxarch.cpp | Imported BigMul as a multi-register HW intrinsic |
| src/coreclr/jit/hwintrinsiclistxarch.h | Listed BigMul in the x86 and x64 HW intrinsic tables |
| src/coreclr/jit/hwintrinsiccodegenxarch.cpp | Emitting MUL/IMUL sequence for BigMul |
| src/coreclr/jit/hwintrinsic.h | Updated multi-reg return count for BigMul |
| src/coreclr/jit/gentree.cpp | Defined struct layout for BigMul return |
Comments suppressed due to low confidence (3)
src/libraries/System.Private.CoreLib/src/System/Math.cs:205
- Consider adding unit tests that validate the new
Math.BigMul(ulong, ulong, out ulong)path on x64 and ensure correct behavior whenX86Base.X64.IsSupportedis true/false.
#if !MONO // X64.BigMul is not yet implemented in MONO
|
@EgorBo, PTAL. |
|
Rerunning failed test. |
| /// <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(); } |
There was a problem hiding this comment.
Why do we need these if we don't use them and they're internal?
There was a problem hiding this comment.
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:
The nint and 32 bit version never gets called, should I remove them or keep them for the symmetry ?
I do no longer think it makes much sense to make this methods public >(#58263) since you can just call existing BigMul methods, and the nint version was solved by a simple *if * in #114731
There was a problem hiding this comment.
I guess it means that NI_X86Base_BigMul should be removed as well ?
There was a problem hiding this comment.
I'd remove, it's basically a dead/untested code. Btw, I've pushed some AI-generated tests 🙂
There was a problem hiding this comment.
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)
This #115966 but without the mulx support.
See the old PR for benchmarks results and generated code
The reason for opening a separate PR is
Feel free to close it if you prefer to work with the original PR