Optimize UInt256 logical shifts with a portable funnel shift - #109
Merged
Conversation
kamilchodola
force-pushed
the
perf/pr-shifts-x86only-392d749
branch
from
August 28, 2026 14:15
bb4a902 to
6741dea
Compare
kamilchodola
force-pushed
the
perf/pr-shifts-x86only-392d749
branch
from
August 28, 2026 14:18
6741dea to
8baac47
Compare
kamilchodola
marked this pull request as draft
September 1, 2026 13:42
Supersedes the X86Base-gated implementation from the previous commit on this branch. That version kept origin/main's body as a second implementation behind an architecture gate, even though its own fast path contained no intrinsics. This replaces both with one portable body. Unifies the funnel: the carry term is written (lo >> 1) >> (63 - bitShift), which equals lo >> (64 - bitShift) but yields 0 when bitShift is 0. Whole-word counts therefore need no separate path, so the (n % 64) == 0 switch, the Lsh64/Lsh128/Lsh192 and Rsh64/Rsh128/Rsh192 helpers and NativeLsh/NativeRsh all go away. Only the word offset is still branched on, and as a compare chain rather than a switch: a 4-way switch here compiles to a jump table reached by an indirect jmp, which predicts perfectly when a benchmark fixes the shift count per case but not on a mixed count stream, and Dynamic PGO cannot reorder it. The result is written in one store of the width callers read it back at. Most of this type loads a UInt256 as a single Vector256 (AddAvx2, LessThanAvx2, ToBigEndian), and a 32-byte load cannot be store-forwarded from four 8-byte stores - it waits on L1. SetLimbs stores through Unsafe.As rather than returning a UInt256, because struct promotion splits the latter back into limb stores, and falls back to limb stores where Vector256 is not hardware accelerated: there the callers read limbs too, and the software Vector256.Create is out-of-line calls (Rsh went 317 -> 1290 bytes before that guard was added). Measured against main at 3a03caf on Zen 5 (9950X, .NET 10), Tier-1 + PGO, on a Solidity-idiomatic shift mix. Paired rounds in one process with rotating order, median of per-round ratios, then median over 9 independent processes; the A/A control sits at 1.000-1.002 in every cell. Lower is faster: Rsh Lsh caller reads 4x8B, throughput 0.843 [.800-.911] 0.810 [.767-.838] caller reads 1x32B, throughput 0.440 [.426-.443] 0.394 [.387-.937] caller reads 4x8B, dep chain 0.978 [.960-.997] 0.920 [.888-.928] caller reads 1x32B, dep chain 0.812 [.809-1.251] 0.824 [.793-.826] Brackets are min..max over the 9 processes. The 32-byte-consumer cells are sensitive to code layout, which shifts store-to-load forwarding: one process in nine put Lsh throughput at 0.937 and Rsh dep chain at 1.251. The median is the estimate; the tail is real and a single-process measurement of these two cells should not be trusted. Tier-1 code size, x64: Lsh 338 -> 367 B, Rsh 337 -> 373 B. Under FullOpts main emits two out-of-line Lsh192 calls and this emits none. With DOTNET_EnableHWIntrinsic=0: Lsh 377 -> 335 B, Rsh 376 -> 317 B. Negative counts keep the pre-existing release-build behaviour even though the old path violated its own Debug.Assert(n < 64) to produce it, so it is fallout rather than a contract; the new tests pin it, unskipped on every target, so it can be retired deliberately rather than by accident.
benaadams
marked this pull request as ready for review
September 1, 2026 18:58
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Pull request overview
This PR rewrites UInt256 logical left/right shifts to use a single portable “funnel shift” implementation, eliminating the prior word-shift helper methods and any architecture-gated duplicate implementations. It also adds a dedicated unit test suite that validates boundary, aliasing, negative-count legacy behavior, and randomized correctness against a BigInteger oracle.
Changes:
- Reimplemented
UInt256.LshandUInt256.Rshusing a unified funnel-shift approach with only a word-offset branch and a single full-width store viaSetLimbs. - Removed unused shift helpers (
NativeLsh/NativeRsh,Lsh64/128/192,Rsh64/128/192) and verified there are no remaining references. - Added
UInt256ShiftTeststo exercise shift semantics (including aliasing and negative-count legacy behavior) against aBigIntegeroracle.
File summaries
| File | Description |
|---|---|
| src/Nethermind.Int256/UInt256.cs | Replaces Lsh/Rsh with a portable funnel shift and introduces SetLimbs to store results in a single full-width write when Vector256 is hardware accelerated. |
| src/Nethermind.Int256.Tests/UInt256ShiftTests.cs | Adds comprehensive shift correctness tests (boundaries, aliasing, full range up to 256, negative legacy behavior, randomized oracle checks). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
benaadams
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UInt256.Lsh/Rshare rewritten as a single portable funnel shift. There is no architecture gate and no second implementation.(lo >> 1) >> (63 - bitShift). That equalslo >> (64 - bitShift)but yields 0 whenbitShiftis 0, so whole-word counts need no separate path. The(n % 64) == 0switch, theLsh64/Lsh128/Lsh192andRsh64/Rsh128/Rsh192helpers andNativeLsh/NativeRshare all removed.switch. A 4-wayswitchhere compiles to a jump table reached by an indirectjmp; that predicts perfectly when a benchmark fixes the shift count per case but not on a mixed count stream, and Dynamic PGO cannot reorder it.Head history:
8baac47(the originalX86Base.X64-gated implementation) is preserved,mainis merged in, and71be7f6replaces the implementation. The net diff againstmainis the funnel rewrite only.Why the gated implementation was replaced
The previous approach kept
origin/main's body as a second implementation behindX86Base.X64.IsSupported, but its own fast path contained no intrinsics, so the gate bought nothing except a duplicate body to maintain.More importantly, its
wordShift switcharms buildnew UInt256(...), which at the time wrote the result as four 8-byte stores. A 32-byte load cannot be store-forwarded from four 8-byte stores; it waits on L1, roughly ten cycles. Most of this type loads aUInt256as a singleVector256(AddAvx2,LessThanAvx2,ToBigEndian), as does the EVM stack. Measured against392d749, that path was 1.53-1.58x slower than base for a caller reading the result as 32 bytes, while looking 0.81-0.91x (faster) for a caller reading.u0/.u3. A benchmark that consumes the result narrowly cannot see it.That is consistent with this PR's own earlier finding of +1.3% on the
eth_callreplay alongside a large microbenchmark win.mainhas since changed the four-argument constructor to write plain limbs (#103/#104), so this specific gap has narrowed: against3a03cafthe old implementation measures 0.86-0.96 across the same four shapes, with no regression. The store-width effect itself did not go away, it becamemain's behaviour, and 25 sites inUInt256.csstill read aUInt256as aVector256.SetLimbstherefore stores throughUnsafe.Asrather than returning aUInt256(struct promotion splits the latter back into limb stores), and falls back to limb stores whereVector256is not hardware accelerated, because there the callers read limbs too and the softwareVector256.Createis out-of-line calls.Measurements
Headline: this saves between roughly half a cycle and roughly twelve cycles per shift call, depending on how the caller reads the result. The large, reliable win is that callers which load the result as one 32-byte value stop paying a store-to-load forwarding stall. Everything else is a modest tightening worth one to four cycles.
Zen 5 (9950X), .NET 10, Tier-1 + Dynamic PGO, baseline
mainat3a03caf, on a Solidity-idiomatic shift mix (selector/address/byte extraction, power-of-two scaling, plus out-of-range counts).Cycle figures assume 4.3 GHz. Nothing regresses in any shape.
What the shapes mean. "Reads 1x32B" is a caller that loads the shift result as a single
Vector256-AddAvx2,SubtractAvx2,LessThanAvx2,ToBigEndian, and the EVM stack. "Reads 4x8B" is a caller that touches individual limbs. "Throughput" is independent shifts back to back; "dependency chain" feeds each result into the next shift, so it measures latency. Real code sits between the two, and the 32-byte row is the common one for this type.Where the 11-12 cycles come from. A 32-byte load cannot be store-forwarded from four 8-byte stores; it waits on L1.
mainwrites the result as limb stores, so aVector256-reading caller stalls. Writing one 32-byte store removes the stall, which is why that row is the outlier and why the two consumer widths are reported separately.Method and caveats
Both variants live in one process; each round measures baseline and candidate with rotating order; per-round ratios are reduced to a median, and that is repeated across 9 independent processes with the median taken again. An A/A control (baseline in the candidate slot) sits at 0.999-1.000 in every cell, min 0.994, max 1.005.
Code size
JitAsm, Tier-1 + PGO, x64:
main@3a03cafLshRshLsh,DOTNET_EnableHWIntrinsic=0Rsh,DOTNET_EnableHWIntrinsic=0Neither body emits a jump table or an indirect jump. Under FullOpts
mainemits two out-of-lineLsh192calls and this emits none. For reference, the superseded8baac47body was 510/512 B at Tier-1.Correctness
DOTNET_EnableHWIntrinsic=0(the skip is the pre-existing hardware-accelerated hash test, unrelated to shifts).DOTNET_EnableAVX2=0andDOTNET_EnableAVX512F=0.ubuntu-24.04-arm, inHWIntrinsicsandNoHWIntrinsics, debug and release, plus the zkEVM variant. ARM64 exercises the limb-store branch ofSetLimbs, whereVector256.IsHardwareAcceleratedis false.BigIntegeroracle: every count in[-300, 300]plusint.MinValue,int.MaxValueand+/-2^20, over 72 values, forLsh,Rshand both aliased forms.The test file no longer gates negative non-word counts behind
X86Base.X64.IsSupported; with one implementation there is nothing left to skip, and it now covers every count in[-260, -1]on every target. Negative counts are pinned rather than designed: the pre-1.6.1 path produced those results by violating its ownDebug.Assert(n < 64), so the behaviour is release-build fallout, not a contract. Pinning it means it can be retired deliberately instead of by accident, and definingnas unsigned would be a reasonable follow-up.Outstanding
eth_callreplay needs re-running against71be7f6. The result recorded below (+1.3% median, "held for rework") measured8baac47, which is no longer the implementation, and the baseline it used has also moved.vpermdwith a sign-masked index, 133 B) and an AVX512VLvpermt2qvariant. The AVX2 form wins the throughput shapes 0.60-0.75x but loses the dependency-chain shapes, and averaged over 24 points the portable scalar funnel matched the AVX512 version, so no ISA gate was added. Three ways of sharing the funnel across the word-offset arms (goto casefall-through, and two switch-selection forms) are 30-40% smaller and all slower, because the duplication is specialisation:wordShift == 3needs oneshland sharing forces all four funnels on every call.Superseded: original evidence for the
X86Base.X64-gated implementation (8baac47)Retained for history. All figures below describe
8baac47against base392d749; both the implementation and the baseline have since changed, so these numbers no longer describe this PR.Original summary
X86Base.X64.IsSupported.Hosted microbenchmark evidence
bench-run/shifts-candidate-x86only-cc8a702, sourcecc8a702, workflow head3cc977ca393ba5abfc27d79fc5bea818cb7e8a30, run 33175200432bench-run/shifts-base-x86only-c020944, production source392d749, benchmark-harness headb3adc40d3885e43127a51f129db852e054a02d61, run 33175617930CorpusWeightedused the captured 497-corpus rates: 88.54% non-word Lsh counts and 68.17% non-word Rsh counts.AMD64 HW per-workload deltas:
Each of those workloads fixes the shift count, which is what made the jump table and the narrow result consumer look free.
Original JIT/code-size check
With tiering disabled and
COMPlus_JitDisasm: x64 HWLsh825 bytes,Rsh903 bytes; x64 no-HWLsh/Rsh461/495 bytes. Measured under FullOpts rather than Tier-1, and compared against a prior candidate rather than againstorigin/main.Isolated target-corpus update (2026-08-29)
Staged as
1.6.1-alpha.24bagainst exact Nethermind master in the seeded AMD 497-recordeth_callworkflow: run 33247079804.Conclusion at the time: held for rework, not recommended for shipping unchanged despite the x64 microbenchmark win. That regression is what the store-width analysis above explains.