Skip the zero leading quotient digit and reuse correction products in MULMOD reduction - #120
Conversation
benaadams
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
A reducer hot-path currently treats X86Base.X64.IsSupported as “hardware widening multiply available”, which can cause unnecessary software widening multiplies on x64 CPUs without BMI2 (and contradicts the intended reuse-vs-recompute decision).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes UInt256.MultiplyMod for multi-limb moduli by reducing unnecessary work in the Knuth D reduction loop and by tuning inlining/frame layout for different targets (host JIT vs zkevm guest). The changes target both throughput (fewer reduction iterations / less redundant multiply-subtract work) and call-frame overhead on trivial exit paths.
Changes:
- Split
MultiplyModinto a trivial-entry + wide-body helper and add ARM64 routing to keep the wide reduction in the entry frame when appropriate. - Optimize the 192/256-bit Knuth D reducers by skipping a provably-zero leading quotient digit, preserving exact remainders from reciprocal division, and reusing correction products where beneficial.
- Add targeted correctness tests for leading-digit boundary cases and add/extend benchmarks and mod-kernel coverage for power-of-two moduli.
File summaries
| File | Description |
|---|---|
| src/Nethermind.Int256/UInt256.zkevm.cs | Adds zkevm-specific inlining policy constants for the wide MULMOD body and 128-bit reducer. |
| src/Nethermind.Int256/UInt256.std.cs | Adds host-JIT inlining policy constants to keep wide spills off trivial paths and expose the 128-bit reducer. |
| src/Nethermind.Int256/UInt256.DivideMod.cs | Implements the core MULMOD/reduction optimizations (leading digit skip, exact-remainder reuse, product reuse) and refactors wide-body framing/inlining. |
| src/Nethermind.Int256.Tests/MultiplyModLeadingDigitTests.cs | Adds regression tests for leading quotient digit boundary cases and constructed q*d+r scenarios across reducers. |
| src/Nethermind.Int256.Tests/ModKernelTests.cs | Expands the power-of-two modulus sweep to cover every exponent. |
| src/Nethermind.Int256.Benchmark/MulModOptimizationBenchmarks.cs | Adds a dedicated benchmark harness to measure MULMOD performance across modulus “shape” scenarios. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
No correctness defects found. The three algorithmic claims hold, and the changed branches are genuinely covered by the new tests.
What I verified
1. Leading-digit skip. In the shift == 0 branch the ninth limb really is zero (w8 = 0, and limbs above uLen-1 are zero by construction of uLen), so the top window is (0 : u[uLen-1]) / ndTop and u[uLen-1] < ndTop forces qhat = 0 — the whole iteration is a no-op. Restricting the check to the no-shift branch is necessary, and the PR does. uLen >= 5 always, so m cannot go negative (uLen = 5 -> m = 0 is the tight case in the 256-bit reducer).
2. UDivRem2By1's remainder is exact. It is Möller–Granlund udiv_qrnnd_preinv with both final corrections, so rhat == (u8:u9) - qhat*ndTop holds identically to X86Base.X64.DivRem. Its preconditions (normalised divisor, u8 < ndTop) are guaranteed by the u8 >= ndTop -> goto FullSubMul guard immediately above. Dropping the now-unreachable qhat = EstimateQhat(...) fallthrough is right.
3. ph:pl reuse. The correction loop maintains ph:pl == qhat * nd_{n-2} across every qhat--, and both early exits (rhat overflow -> FullSubMul, saturated digit -> FullSubMul) leave before the reuse site, so a stale product cannot be consumed. v1 = u10 is safe too: u10 is read at the top of the iteration and only un[j] is written before it. Likewise the power-of-two mask — mask + 1 divides 2^64, so masking after the wrapping multiply is equivalent.
Testing
| check | result |
|---|---|
| Full suite, default ISA | 588,053 pass |
Full suite, DOTNET_EnableHWIntrinsic=0 |
588,052 pass, 1 skipped |
Full suite, DOTNET_EnableAVX2=0 (Bmi2=false, X86Base.X64=true) |
588,053 pass |
Independent BigInteger differential harness: 60k direct reducer cases per width + 400k MultiplyMod, std and zkevm variants, four ISA configs |
all match |
| Branch instrumentation driven by the PR's own new tests | reaches skip-taken and skip-not-taken, saturated digit, rhat-overflow -> FullSubMul, and add-back, in both the 192- and 256-bit reducers and in KnuthStep |
Mutation: < d3 -> <= d3 in the 256-bit skip |
caught by Leading_quotient_digit_boundaries_match_BigInteger (plus 78 existing tests) |
So the new tests are real regression tests, not smoke. I did not attempt to reproduce the timings.
Worth changing
-
Constructed_quotients_and_remainders_match_BigIntegeris missing[TestCase(128)]. The description says "4,096 constructedq*d + rcases per reducer", but the 128-bit reducer only gets the boundary test. Adding128works as written —qgets 384 bits, the product stays within 512, andhi != 0. -
Both new tests take a single parameter, so
[Values]fits better than stacked[TestCase]:public void Leading_quotient_digit_boundaries_match_BigInteger([Values(128, 192, 256)] int bits). -
The 128-bit reducer states the same optimisation twice —
uLen--underX86Base.X64.IsSupported,m--under!X86Base.X64.IsSupported— and the x64 line carries no comment at all, while its 192/256 twins say "skip the provably zero leading quotient digit". The// Keeping uLen unchanged...comment explains why the form differs but not what either line does. A "what" comment on the first, cross-referencing the second, would save the next reader a 28-line scroll. -
Bmi2.X64.IsSupported || ArmBase.Arm64.IsSupportedwith// Avoid repeating the software widening product. On x64 without BMI2,Multiply64falls through toMath.BigMul, which is a JIT intrinsic lowering tomul— hardware, not software. The predicate really means "the widening multiply is a single cheap instruction that does not needmulx". Either reword the comment or hoist the condition into a named local with that meaning; as written it will mislead. -
Benchmark class.
MultiplyModMaxTargetedalready coversmax/full(FullWidthMiss) /narrowwith an enum-typed[Params]and[NoIntrinsicsJob]. The new class overlaps it, usesstringparams where both sibling benchmarks (ModShapesBench.Shape,MultiplyModMaxCase) use enums, and hides"narrow"behind the_ =>default arm so nothing in the switch names it. It also callsUInt256.MultiplyModdirectly in the loop, whereasModShapesBenchdeliberately routes through a[MethodImpl(MethodImplOptions.NoInlining)]wrapper — "one real call per operation, as production callers see it" — which matters here specifically, since the entry-frame split is only observable across a real call boundary. ExtendingMultiplyModMaxCasewith the shift/192/128/pow2 shapes would be the DRY move; failing that, at least add the job and warmup/iteration attributes so the reported matrix is reproducible from the class alone.
Minor
// qhat overshot against the full divisor ... (rare)inKnuthStep: I measured 304k/1.16M ~= 26% on random inputs. Pre-existing wording, and not a regression — the old non-x64 route had the same add-back rate — but this PR makes it the universal path, so it is a fair moment to drop "(rare)".- CI has intrinsics-on and intrinsics-off jobs, but none with
X86Base.X64on and BMI2 off, which is the exact configuration the last commit (hi2 = ph; lo2 = plon x64) targets. Both halves are covered separately; aDOTNET_EnableAVX2=0job would cover the combination. I ran it locally and it is green.
Nice to see this land as a net removal of code (EstimateQhatEst, the duplicated Multiply64) rather than an addition.
Client A/B against Nethermind master (2026-09-07)Tested this PR as a staging package ( Verdict: safe, zero regressions, response parity 497/497 on both ISAs. The only measurable gain is on the arm64
|
| box | cell | avg | CPU-ms/req | p99 | paired per-record median (95% CI) | closed-loop throughput |
|---|---|---|---|---|---|---|
| amd64 | 100 rps | +0.2% | +0.4% | +0.1% | -0.7% (-1.4 .. -0.1) | +0.6% |
| amd64 | 300 rps¹ | -1.1% | -0.3% | -1.2% | ||
| arm64 | 100 rps | -0.1% | +0.1% | -0.5% | -1.8% (-1.9 .. -1.6) | +1.1% |
| arm64 | 300 rps | -2.5% | -1.5% | -2.5% |
¹ amd64 at 300 rps sits at ~90% of the box's serialization ceiling (≈2% HTTP failures in both arms), so that cell measures queueing as much as compute. The two non-EVM selector classes are flat on both boxes, so the arm64 gain is in EVM arithmetic. PR failure rate 0.00% at 100 rps on both boxes.
Runs: amd64 34126784881, arm64 34126788243.
Block processing, EXPB fusaka (999 mainnet blocks, paired per-block, with an identical-code A/A control image)
| box | arm | n | paired mean vs master | paired median | blocks worse |
|---|---|---|---|---|---|
| amd64 | PR | 4² | -0.035 ms/block | -0.05 | 484/999 |
| amd64 | A/A control | 6 | -0.090 ms/block | -0.15 | 426/999 |
| arm64 | PR | 6 | -0.018 ms/block | 0.00 | 432/999 |
| arm64 | A/A control | 6 | +0.064 ms/block | 0.00 | 475/999 |
The PR delta is inside the A/A floor on both boxes (split-half floors 0.13-0.29 ms on amd64, up to 0.08 ms on arm64): neutral. All 34 client jobs: no exceptions, no invalid blocks, clean shutdown.
² Both amd64 "PR run 3" jobs died 9 s in because the workflow's per-job yq download from GitHub releases returned a 504 page twice in a row; infrastructure, not this PR.
Runs: amd64 34126791812 / 34126795596, arm64 34126798850 / 34126802317 (forward and mirrored image order).
Results
MultiplyMod(x, y, m)by modulus shape, againstmain. Lower PR/main is better: 0.86 means 14% less time.x64 is a Ryzen 9 9950X (Zen 5, AVX-512) under Tier-1 with dynamic PGO; ARM64 is the
ubuntu-24.04-armrunner (Neoverse N2) through
benchmark.yml, andMulModOptimizationBenchmarksin this PR is what producesthat column. The zk column runs the same ten workloads as a bflat riscv64 guest under
ziskemuand countswhole-guest instruction steps, startup included.
xandyare random 256-bit values, so the product fillsall eight limbs; the shape names the modulus.
full,192and128have the high bit of their top limb setand need no normalising shift; the
shiftvariants have a top limb below 2^32 and need one.64is aone-limb odd modulus,
narrowis a one-limbxagainst a random odd 256-bit modulus,maxis 2^256 - 1 andpow64is 2^k.Geomean over the ten shapes, by configuration. FullOpts (
DOTNET_TieredCompilation=0) compiles everythingonce without profile data and is where the frame changes below show most; PGO already recovers part of that
for
main, so the Tier-1 rows are the conservative figure:DOTNET_EnableHWIntrinsic=0, Tier-1 PGODOTNET_EnableHWIntrinsic=0, FullOptsThe guest in detail, since steps are the proving cost there. Memory cost is Zisk's weighted memory-access
count, not RAM:
On the wrong side of the noise band: x64
narrowunder PGO is 1% slower with AVX-512 and 4% slower with AVX2only (20.96 -> 21.79 ns), while FullOpts and every other target improve it;
64with intrinsics off under PGOis 2% slower (43.04 -> 44.00 ns); the guest's
64takes 0.19% more steps, and64andpow64each cost 168more memory units. The ARM figures are single ShortRun jobs on separate runners, so its rows inside 3%
(
192shift,shift,64,max) should be read as parity.What is in it
MultiplyModwith a modulus of two or more limbs is a 256x256 -> 512-bit product followed by Knuth Dagainst the modulus, one quotient digit per iteration. Each iteration divides the top of the window by the
modulus's top limb, corrects the estimate, then multiplies the estimate back through the whole modulus and
subtracts. Three things in that loop were being paid for and never used.
The leading quotient digit was almost always zero, and every kernel computed it anyway. When the
modulus needs no normalising shift, the product's eight limbs get a zero ninth limb on top and the first
window is
(0 : u7) / nd_top, which is 0 or 1, and 0 wheneveru7 < nd_top. For uniformly random operandsthat is about 95% of calls (the product of two uniform draws skews low); for operands already below the
modulus, as in field arithmetic,
x*y < m^2makes it every call bar a modulus whose top limb is all ones.One compare before the loop drops the whole iteration: its divide, its correction and its multiply-subtract.
The 256-, 192- and 128-bit reducers all get it, in the no-shift branch only - after a normalising shift the
ninth limb holds real bits, and a check that covered that case cost the
shiftshapes more than it saved. Inthe reciprocal 128-bit path the check shortens the digit count after normalisation rather than
uLenbeforeit, which keeps the loop free of spills.
The reciprocal path threw its remainder away. On x64,
DivRemreturnsrhat == (u8:u9) - qhat*nd_topexactly, so the kernels already skipped the top limb's product and subtracted one limb less. Every other
target - ARM64,
DOTNET_EnableHWIntrinsic=0, the zk guest - went throughEstimateQhat, which runs the same2-by-1 reciprocal divide, discards its remainder, and falls into the full multiply-subtract.
UDivRem2By1isexact too, so those targets now take the x64 path in the 192- and 256-bit reducers and in the 128-bit
KnuthStep. Only the saturated digit (u8 >= nd_top) and a correction that overflowsrhat, where theidentity does not fit a limb, still go the long way. This is most of the ARM, scalar and guest movement.
The correction already held the next limb's product. The Knuth correction loop maintains
ph:pl == qhat * nd_{n-2}so it can test the estimate; the multiply-subtract then computedMultiply64(nd_{n-2}, qhat)again. The 192-bit reducer now reusesph:plon every target, and reads thelimb it already loaded rather than reloading it through the ref. The 256-bit reducer reuses it only where the
widening multiply is software: with
mulxor ARM64mul/umulhthe product is cheaper than keeping twomore values live across the four-limb subtraction, and reusing it there measured no repeatable gain.
Power-of-two moduli masked both operands before multiplying. The low
kbits of a product depend only onthe low
kbits of its operands, so one mask after the multiply is enough.Frames.
Multiply256To512BitLargewasNoInliningand now inlines into its callers, and the 128-bitreducer inlines into
MulModBy65To128Bitson host JITs. The wide body moves out ofMultiplyMod's entry sothe zero, one and narrow-modulus exits stop paying its prologue: the public entry goes from 864 bytes, two
saved registers and a 104-byte frame to 193 bytes, a 40-byte frame and tail jumps into
MultiplyModWideorMod, while the wide helper grows to 1,955 bytes with the product inlined. Under FullOpts the trivial exitsgain 0.3-1.1 ns (
zero1.67 -> 1.36 ns,one7.11 -> 6.41 ns over 10M-call samples); under PGO they areflat.
MultiplyOverflowreaches the same product throughMultiply256To512Bitand picks up the inlined bodytoo; it was not measured here. Two targets want the opposite policy, and say so in one constant each:
ARM run at the previous commit had
fullat 89.89 ns,192at 91.72 andshiftat 107.16, against 83.01, 87.82 and 102.57 with the bodyinline, so
ArmBase.Arm64.IsSupportedcalls the core directly.UInt256.zkevm.cs) forces the wide body inline and leaves the 128-bit reducer to thecompiler: a separate call raised guest steps and memory traffic, and forcing the reducer inline lowered
memory cost but raised steps. With those two constants the final guest matches the monolithic build's
steps and memory cost exactly on all ten shapes.
Not taken
ph:plalive through the four-limbsubtraction lengthened live ranges with no repeatable gain against a hardware widening multiply, so those
targets recompute and the software targets reuse. The 192-bit reducer has one limb less in flight and reuses
everywhere.
2^256 - 1fold were slower than the existing SIMD add in everyx64 configuration; the fold stays as it was.
shiftshapes than it removed. The check stays in the no-shift branch.
Validation
test jobs pass - Windows/Linux/macOS, native ARM64, intrinsics on and off, debug and release, and the
zkevmvariant.at the commit before the ARM routing change; that last commit only alters which call ARM64 takes, and its
x64 listings are identical bar relocation addresses.
MultiplyModLeadingDigitTestsdrive the three reducers directly: leading limb one below, equal to andone above the divisor's top limb, 5- to 8-limb dividends, shifts of 0/1/31/63 with zero and all-ones tails,
output aliased onto the divisor; and 4,096 constructed
q*d + rcases per reducer withrin{0, d/2, d-1} across all 64 normalising shifts. The power-of-two
Modsweep now covers every exponentinstead of every third.
MultiplyModresults agree withBigInteger, and 150,000 aliased calls agree with theunaliased result, in each of the AVX-512, AVX2-only and no-intrinsics configurations.
carries no floating-point, compressed or atomic instructions.
git diff --checkclean.Measurement notes: x64 is .NET 10.0.11 with
DOTNET_TieredCompilation=1andDOTNET_TC_CallCountingDelayMs=0(PGO) or
DOTNET_TieredCompilation=0(FullOpts),DOTNET_EnableAVX512=0for the AVX2 column, processes runserially and pinned to one core, 1,024 operand triples per shape, nine samples of 1,024,000 calls after a
1,024,000-call warm-up, baseline and candidate in ABBA order, the mean of the two process medians reported.
The ARM baseline is
mainplus the benchmark classand the candidate is the final SHA, both
BenchmarkDotNet ShortRun. Guest counts are
ziskemu1.2.0-alpha on bflat riscv64 builds, 1,024 MULMODs pershape, startup included.