Add mulx support to Math.BigMul#128787
Conversation
…ce register (#2) - it produce slighly better code for some scenarios
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@JulieLeeMSFT, @jakobbotsch @EgorBo is there any more action required from my part here? The test results seemed unrelated and based on asm diffs the performance regression should be fixed with additional performance improvements expected for decimal and more. |
|
@Daniel-Svensson, sorry for the delay in review. We are at the end of product cycle, so a bit busy. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "9cfe7116b1db33419ef046cedcb2318027920c66",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "8fe2929f5e365d262ca514854330c979fe6ab28b",
"last_reviewed_commit": "9cfe7116b1db33419ef046cedcb2318027920c66",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "8fe2929f5e365d262ca514854330c979fe6ab28b",
"last_recorded_worker_run_id": "29680698145",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "9cfe7116b1db33419ef046cedcb2318027920c66",
"review_id": 4730526312
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: This PR is a follow-up to #117261 that addresses the performance regression tracked in #128594. The unsigned 64-bit Math.BigMul path currently lowers to the legacy mul instruction, which has an implicit EAX/EDX register constraint. On BMI2-capable CPUs the mulx instruction can compute the same unsigned 128-bit product without clobbering flags and, crucially, without forcing the operands and results into fixed registers, giving the register allocator far more freedom. The reported SuperPMI asm-diffs show consistent code-size and PerfScore improvements across nearly all collections, confirming the motivation.
Approach: The change is limited to LSRA (lsraxarch.cpp) and codegen (hwintrinsiccodegenxarch.cpp) for NI_X86Base_X64_BigMul. Both add a new branch, gated on baseType == TYP_ULONG (codegen keys off ins == INS_mulEAX, which is equivalent since INS_mulEAX is only selected for the unsigned long base type) and compOpportunisticallyDependsOn(InstructionSet_AVX2). In LSRA the new branch marks the node non-RMW, forces one source into EDX (preferring op2 when neither is contained, per the SuperPMI data cited in the comment), and defines both results into freely-allocatable (APX-aware) registers instead of the fixed EAX/EDX. Codegen mirrors this by moving the first operand into RDX and emitting mulx hi, lo, rmOp via inst_RV_RV_TT(..., isRMW=false). The signed path and the non-BMI2 unsigned path are preserved unchanged in the else branches. The pattern closely mirrors the pre-existing NI_AVX2_MultiplyNoFlags (mulx) handling, which is a good consistency signal. The remaining diff is whitespace-only comment rewrapping in two unrelated functions.
Summary: This is a focused, well-reasoned CQ improvement that reuses the established mulx codegen pattern already present for MultiplyNoFlags. The register-allocation model (non-RMW, EDX implicit source, free result registers) matches mulx semantics, the feature gate is correct, and the fallback paths are untouched so non-BMI2 and signed cases are unaffected. Containment handling for BigMul was already wired to the shared mulx/BigMul case in lowering, so the new memory-operand path is covered. I found no correctness concerns in the changed lines; the only note is a harmless dead store flagged inline. Verdict: LGTM. Note that I did not build or run tests in this worker — assess CI (JIT SuperPMI / coreclr test legs) for functional confirmation.
Detailed Findings
No blocking issues. One minor, non-blocking observation is left as an inline comment on the isRMW = false; dead store in lsraxarch.cpp.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 105.2 AIC · ⌖ 10.6 AIC · ⊞ 10K
| ForceLowGprForApxIfNeeded(op1, RBM_NONE, canHWIntrinsicUseApxRegs); | ||
| if (baseType == TYP_ULONG && m_compiler->compOpportunisticallyDependsOn(InstructionSet_AVX2)) | ||
| { | ||
| isRMW = false; |
There was a problem hiding this comment.
Minor: this isRMW = false; assignment is a dead store. In this branch the code sets buildUses = false; and break;s without any further read of isRMW (the RMW-dependent uses only apply to buildUses-driven paths later in the function). It's harmless and arguably documents intent, but if you want to avoid the dead store you could drop it, or convert it to a comment explaining that MULX is a non-destructive 3-operand form (unlike the implicit-EAX mul).
Follow up to #117261, with added support for mulx to adress performance regression #128594.
@EgorBo I've not yet run the benchmarks, is it easy for you to use your bot against a PR ?
Otherwise you will have to wait for a couple of days for benchmarks results
Corrently both lokal spmi run and
devops build shows an expected improvement to generated code.
Devops spmi results shows nice improvements especially for Linux x64 with >5000 bytes improvement with memory accesses in several locations.
linux x64 asm-diffs (2 juli 2026, PR build)
windows
old local resuls (motivation to why op2 is used for rdx instead of op1)
local SPMI results (latest)
FullOpts (-1,488 bytes)
local SPMI where op1 was fixed
FullOpts (-707 bytes)