Arm keccak - #12843
Conversation
- SDK 11.0.100-preview.7 in global.json, TFM net11.0 - Microsoft.* packages to 11.0.0-preview.7; drop redundant Microsoft.Extensions.Logging.Abstractions references - Dockerfiles, nix flake, and CI workflow updated to match
- Add KeccakF1600ArmSha3 using ARMv8.2-SHA3 intrinsics (EOR3/RAX1/ XAR/BCAX via Sha3.*) and prefer it in KeccakF dispatch - Restructure the AVX-512 permutation into a shared Round helper with shared lane-index vectors and pre-broadcast round constants - Rename the portable path to KeccakF1600Scalar (internal) and add an Arm-vs-scalar equivalence test plus a permutation benchmark
core-repository-dispatch-app
Bot
added
agentic 🤖
dependencies
|
Claude finished @benaadams's task in 8m 10s —— View job Review: Arm Keccak + AVX-512 tuning
I hand-verified both intrinsic paths against the scalar reference. Both are mathematically correct — details below. The findings are about test coverage, a dropped bounds pre-check, and missing safety documentation, not about the maths. I could not build locally: What I verified by handARM SHA-3 path — faithful to
AVX-512 Pi rewrite — all five re-rooted chains match π (
I also checked that lanes 5–7 stay zero for the whole permutation (θ parity → 0, ρ rotates by 0, π selects indices 5/6/7 from either operand, χ's FindingsHigh1. The rewritten AVX-512 path has no test, and the new ARM test almost certainly never runs. (inline)
2. Same gap on the ARM side. The unit-test workflows run on x64; the only arm64 runner in Both collapse into one fix: a single Medium3. 4. The safety comments justifying the deliberate over-read/over-write were deleted, and the new ARM file adds none. (inline) 5. Register pressure in the ARM path. (inline) The 6. Two unrelated changes in one PR. The .NET 11 preview 7 bump is 25 of the 31 changed files and pins the whole repo — SDK, Docker base images, nix flake, PGO workflow — to a preview runtime. AGENTS.md asks for minimal, focused diffs; the Keccak work would be far easier to review and revert on its own. I assume the Low7. 8. 9. The new test's 64-case loop should be 10. The don't-care index lanes in the new Pi chains (e.g. 11. 12. 13. No XML docs on the new/visibility-changed members ( The maths is sound and the ARM path is a worthwhile addition. Items 1–3 are what I'd want closed before merge: a differential test for both intrinsic paths, and the restored pre-check on the |
There was a problem hiding this comment.
Pull request overview
This PR adds an ARM64 SHA-3–intrinsics Keccak-f[1600] permutation implementation (with runtime dispatch) and updates the repository/tooling baseline to .NET net11.0 / SDK 11.0.100-preview.7, including Docker and Nix packaging changes.
Changes:
- Add
Sha3.IsSupported-gated ARM64 Keccak permutation path and keep scalar fallback. - Refactor the AVX-512 Keccak permutation implementation and add a targeted benchmark plus an ARM permutation correctness test.
- Bump target framework/toolchain and container/Nix dependencies from .NET 10 to .NET 11 preview 7.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/SendBlobs/README.md | Updates build instructions to .NET 11 SDK. |
| tools/SendBlobs/Dockerfile | Moves SendBlobs build/runtime images to .NET 11 preview 7. |
| tools/RpcTests/RpcTests.Monitor/RpcTests.Monitor.csproj | Bumps monitor tool to net11.0. |
| tools/RpcTests/RpcTests.Monitor/Dockerfile | Updates runtime image to .NET 11 preview 7. |
| tools/Kute/Dockerfile | Moves Kute build/runtime images to .NET 11 preview 7. |
| tools/JitAsm/Program.cs | Updates example path comment from net10.0 to net11.0. |
| tools/EngineApiProxy/Dockerfile | Updates build/runtime images to .NET 11 preview 7. |
| src/Nethermind/Nethermind.Test.Runner/Dockerfile | Updates build/runtime images to .NET 11 preview 7. |
| src/Nethermind/Nethermind.Runner/packages.lock.json | Regenerates lock file for net11.0 and .NET 11 preview dependencies. |
| src/Nethermind/Nethermind.Runner/Dockerfile | Updates runner base image to .NET 11 preview 7. |
| src/Nethermind/Nethermind.Logging.Microsoft/Nethermind.Logging.Microsoft.csproj | Adjusts references (notably removes explicit logging abstractions ref). |
| src/Nethermind/Nethermind.Kademlia/Nethermind.Kademlia.csproj | Adjusts package references (removes explicit logging abstractions ref). |
| src/Nethermind/Nethermind.Core/Crypto/KeccakHash.std.cs | Adds ARM SHA-3 dispatch, renames scalar permutation, refactors AVX-512 rounds/constants. |
| src/Nethermind/Nethermind.Core/Crypto/KeccakHash.arm64.cs | Adds ARM64 SHA-3 intrinsics Keccak-f[1600] implementation. |
| src/Nethermind/Nethermind.Core.Test/KeccakTests.cs | Adds an ARM SHA-3 permutation-vs-scalar correctness test. |
| src/Nethermind/Nethermind.Benchmark/Core/KeccakPermutationBenchmarks.cs | Adds a benchmark comparing scalar vs ARM SHA-3 permutation. |
| scripts/rpc-bench/run-ethcallchaos.sh | Updates bench SDK image to .NET 11 preview 7. |
| scripts/build/Dockerfile | Updates build SDK image to .NET 11 preview 7 (digest pinned). |
| nix/package.nix | Switches Nix build to a custom .NET 11 preview 7 package. |
| nix/nuget-deps.json | Updates/extends NuGet dependency set for .NET 11 preview 7. |
| nix/dotnet-11-preview7.nix | Adds custom Nix derivation for .NET 11 preview 7 SDK/runtime. |
| global.json | Moves repo SDK to 11.0.100-preview.7.26381.103 and enables prerelease. |
| flake.nix | Updates devshell to use the custom .NET 11 preview 7 SDK. |
| Dockerfile.pgo | Updates PGO build/runtime images to .NET 11 preview 7 (digests pinned). |
| Dockerfile.diag | Updates diag build/runtime images to .NET 11 preview 7. |
| Dockerfile.chiseled | Updates chiseled build/runtime images to .NET 11 preview 7 (digests pinned). |
| Dockerfile | Updates main build/runtime images to .NET 11 preview 7 (digests pinned). |
| Directory.Packages.props | Bumps Microsoft package versions to .NET 11 preview 7 set. |
| Directory.Build.props | Updates default TargetFramework to net11.0. |
| AGENTS.md | Updates repo baseline statement from net10.0 to net11.0. |
| .github/workflows/collect-pgo-profile.yml | Updates runtime repo pin to align with .NET 11 preview 7 baseline. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <ItemGroup> | ||
| <PackageReference Include="Collections.Pooled" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" /> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="all" /> | ||
| </ItemGroup> |
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\Nethermind.Logging\Nethermind.Logging.csproj" /> | ||
| </ItemGroup> |
| asu = Sha3.BitwiseClearXor(bCu, bCe, bCa); | ||
| } | ||
|
|
||
| fixed (ulong* destination = &state) |
| } | ||
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public static void KeccakF1600Avx512F(Span<ulong> state) | ||
| { | ||
| { | ||
| // Redundant statement that removes all the in loop bounds checks | ||
| _ = state[24]; | ||
| } | ||
|
|
||
| // Can straight load and over-read for start elements | ||
| ref ulong s = ref MemoryMarshal.GetReference(state); | ||
| Vector512<ulong> mask = Vector512.Create(ulong.MaxValue, ulong.MaxValue, ulong.MaxValue, ulong.MaxValue, ulong.MaxValue, 0UL, 0UL, 0UL); | ||
| Vector512<ulong> c0 = Unsafe.As<ulong, Vector512<ulong>>(ref MemoryMarshal.GetReference(state)); | ||
| // Clear the over-read values from first vectors | ||
| c0 = Vector512.BitwiseAnd(mask, c0); | ||
| Vector512<ulong> c1 = Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref MemoryMarshal.GetReference(state), 5)); | ||
| c1 = Vector512.BitwiseAnd(mask, c1); | ||
| Vector512<ulong> c2 = Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref MemoryMarshal.GetReference(state), 10)); | ||
| c2 = Vector512.BitwiseAnd(mask, c2); | ||
| Vector512<ulong> c3 = Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref MemoryMarshal.GetReference(state), 15)); | ||
| c3 = Vector512.BitwiseAnd(mask, c3); | ||
|
|
||
| // Can't over-read for the last elements (8 items in vector 5 to be remaining) | ||
| // so read a Vector256 and ulong then combine | ||
| Vector256<ulong> c4a = Unsafe.As<ulong, Vector256<ulong>>(ref Unsafe.Add(ref MemoryMarshal.GetReference(state), 20)); | ||
| Vector256<ulong> c4b = Vector256.Create(state[24], 0UL, 0UL, 0UL); | ||
| Vector512<ulong> c4 = Vector512.Create(c4a, c4b); | ||
|
|
||
| Vector512<ulong> permute1 = Vector512.Create(1UL, 2UL, 3UL, 4UL, 0UL, 5UL, 6UL, 7UL); | ||
| Vector512<ulong> permute2 = Vector512.Create(2UL, 3UL, 4UL, 0UL, 1UL, 5UL, 6UL, 7UL); | ||
| ulong[] roundConstants = RoundConstants; | ||
|
|
||
| // Use constant for loop so Jit expects to loop; unroll once | ||
|
|
||
| Vector512<ulong> c0 = Vector512.BitwiseAnd(mask, Unsafe.As<ulong, Vector512<ulong>>(ref s)); | ||
| Vector512<ulong> c1 = Vector512.BitwiseAnd(mask, Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref s, 5))); | ||
| Vector512<ulong> c2 = Vector512.BitwiseAnd(mask, Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref s, 10))); | ||
| Vector512<ulong> c3 = Vector512.BitwiseAnd(mask, Unsafe.As<ulong, Vector512<ulong>>(ref Unsafe.Add(ref s, 15))); | ||
| Vector512<ulong> c4 = Vector512.Create( | ||
| Unsafe.As<ulong, Vector256<ulong>>(ref Unsafe.Add(ref s, 20)), | ||
| Vector256.CreateScalar(state[24])); |
There was a problem hiding this comment.
Medium — the length pre-check was dropped, and the safety rationale went with it.
master opened this method with
{
// Redundant statement that removes all the in loop bounds checks
_ = state[24];
}That statement did double duty: besides the bounds-check hoist, it was the first thing executed, so a short span threw IndexOutOfRangeException before any unchecked access. Now the first four Unsafe.As<ulong, Vector512<ulong>> reads (offsets 0/5/10/15 → bytes 0..184) and the Vector256 read at offset 20 (bytes 160..192) all run before state[24] performs the only bounds check. KeccakF1600Avx512F is public, so an out-of-repo caller passing a shorter span now gets an ~192-byte OOB read instead of an exception.
Two things worth restoring:
- A guard — either
_ = state[24];up front again, orDebug.Assert(state.Length == 25)to matchKeccakF1600Scalar, or an explicit throw. - The comments explaining why the over-read/over-write is safe (
// Can straight load and over-read for start elements,// Can't over-read for the last elements…,// Can over-write for first elements,// Can't over-write for last elements…). Per robustness.md, deliberately-unsafe memory access needs a comment justifying the invariant — a reviewer can't otherwise tell that reading 64 bytes at lane 15 of a 25-lane span is intentional and in-bounds.
| Vector512<ulong> c0Pi = Avx512F.PermuteVar8x64x2(c0, Vector512.Create(0UL, 8 + 1, 2, 3, 4, 5, 6, 7), c1); | ||
| c0Pi = Avx512F.PermuteVar8x64x2(c0Pi, Vector512.Create(0UL, 1, 8 + 2, 3, 4, 5, 6, 7), c2); | ||
| c0Pi = Avx512F.PermuteVar8x64x2(c0Pi, Vector512.Create(0UL, 1, 2, 8 + 3, 4, 5, 6, 7), c3); | ||
| c0Pi = Avx512F.PermuteVar8x64x2(c0Pi, Vector512.Create(0UL, 1, 2, 3, 8 + 4, 5, 6, 7), c4); | ||
|
|
||
| Vector512<ulong> c1Pi = Avx512F.PermuteVar8x64x2(c1, Vector512.Create(0UL, 4UL, 8 + 0, 3, 4, 5, 6, 7), c2); | ||
| c1Pi = Avx512F.PermuteVar8x64x2(c1Pi, Vector512.Create(0UL, 1, 2, 8 + 1, 4, 5, 6, 7), c3); | ||
| c1Pi = Avx512F.PermuteVar8x64x2(c1Pi, Vector512.Create(0UL, 1, 2, 3, 8 + 2, 5, 6, 7), c4); | ||
| c1Pi = Avx512F.PermuteVar8x64x2(c1Pi, Vector512.Create(8UL + 3, 1, 2, 3, 4, 5, 6, 7), c0); | ||
|
|
||
| Vector512<ulong> c2Pi = Avx512F.PermuteVar8x64x2(c2, Vector512.Create(0UL, 1, 3UL, 8 + 4, 4, 5, 6, 7), c3); | ||
| c2Pi = Avx512F.PermuteVar8x64x2(c2Pi, Vector512.Create(0UL, 1, 2, 3, 8 + 0, 5, 6, 7), c4); | ||
| c2Pi = Avx512F.PermuteVar8x64x2(c2Pi, Vector512.Create(8UL + 1, 1, 2, 3, 4, 5, 6, 7), c0); | ||
| c2Pi = Avx512F.PermuteVar8x64x2(c2Pi, Vector512.Create(0UL, 8 + 2, 2, 3, 4, 5, 6, 7), c1); | ||
|
|
||
| Vector512<ulong> c3Pi = Avx512F.PermuteVar8x64x2(c3, Vector512.Create(0UL, 1, 2, 2UL, 8 + 3, 5, 6, 7), c4); | ||
| c3Pi = Avx512F.PermuteVar8x64x2(c3Pi, Vector512.Create(8UL + 4, 1, 2, 3, 4, 5, 6, 7), c0); | ||
| c3Pi = Avx512F.PermuteVar8x64x2(c3Pi, Vector512.Create(0UL, 8 + 0, 2, 3, 4, 5, 6, 7), c1); | ||
| c3Pi = Avx512F.PermuteVar8x64x2(c3Pi, Vector512.Create(0UL, 1, 8 + 1, 3, 4, 5, 6, 7), c2); | ||
|
|
||
| Vector512<ulong> c4Pi = Avx512F.PermuteVar8x64x2(c4, Vector512.Create(8 + 2, 1, 2, 3, 1UL, 5, 6, 7), c0); | ||
| c4Pi = Avx512F.PermuteVar8x64x2(c4Pi, Vector512.Create(0UL, 8 + 3, 2, 3, 4, 5, 6, 7), c1); | ||
| c4Pi = Avx512F.PermuteVar8x64x2(c4Pi, Vector512.Create(0UL, 1, 8 + 4, 3, 4, 5, 6, 7), c2); | ||
| c4Pi = Avx512F.PermuteVar8x64x2(c4Pi, Vector512.Create(0UL, 1, 2, 8 + 0, 4, 5, 6, 7), c3); |
There was a problem hiding this comment.
Low — the reordered Pi chains are correct, but the "don't care" lanes are invisible to a reader.
I hand-verified all five chains against π: A′[x,y] = A[(x+3y) mod 5, x], i.e. c_yPi[x] = c_x[(x+3y) mod 5]:
| lane0 | lane1 | lane2 | lane3 | lane4 | |
|---|---|---|---|---|---|
c0Pi |
c0[0] | c1[1] | c2[2] | c3[3] | c4[4] |
c1Pi |
c0[3] | c1[4] | c2[0] | c3[1] | c4[2] |
c2Pi |
c0[1] | c1[2] | c2[3] | c3[4] | c4[0] |
c3Pi |
c0[4] | c1[0] | c2[1] | c3[2] | c4[3] |
c4Pi |
c0[2] | c1[3] | c2[4] | c3[0] | c4[1] |
All five match, and starting each chain from c_y instead of c0 correctly shortens the dependency on c0. 👍
What isn't obvious: several index lanes in the first permute of each chain are dead — e.g. Create(0UL, 4UL, 8 + 0, 3, 4, ...) for c1Pi only contributes lanes 1 and 2; lanes 0/3/4 are overwritten by the three following PermuteVar8x64x2 calls. Same for the 2UL at lane 3 in c3Pi and 1UL at lane 4 in c4Pi (that one is live). Mixing bare int and UL-suffixed literals in the same Create makes it harder still to spot which lane is real.
In consensus-critical code a one-liner per chain — "first operand supplies lanes N and M; remaining lanes are overwritten below" — would save the next reviewer the table above.
| [SkipLocalsInit] | ||
| internal static unsafe void KeccakF1600ArmSha3(Span<ulong> st) | ||
| { | ||
| Debug.Assert(st.Length == STATE_SIZE / sizeof(ulong)); | ||
|
|
||
| Vector128<ulong> aba, abe, abi, abo, abu; | ||
| Vector128<ulong> aga, age, agi, ago, agu; | ||
| Vector128<ulong> aka, ake, aki, ako, aku; | ||
| Vector128<ulong> ama, ame, ami, amo, amu; | ||
| Vector128<ulong> asa, ase, asi, aso, asu; | ||
| Vector128<ulong> bCa, bCe, bCi, bCo, bCu; | ||
| Vector128<ulong> da, de, di, @do, du; | ||
| Vector128<ulong> eba, ebe, ebi, ebo, ebu; | ||
| Vector128<ulong> ega, ege, egi, ego, egu; | ||
| Vector128<ulong> eka, eke, eki, eko, eku; | ||
| Vector128<ulong> ema, eme, emi, emo, emu; | ||
| Vector128<ulong> esa, ese, esi, eso, esu; | ||
|
|
||
| ref ulong state = ref MemoryMarshal.GetReference(st); | ||
| ref ulong roundConstants = ref MemoryMarshal.GetArrayDataReference(RoundConstants); | ||
|
|
||
| asu = Vector64.LoadUnsafe(ref state, 24).ToVector128Unsafe(); |
There was a problem hiding this comment.
Medium — two undocumented invariants in an unsafe method.
I verified the whole permutation against KeccakF1600Scalar and it is faithful: Sha3.Xor = EOR3 (2 chained = 5-way parity ✓), BitwiseRotateLeftBy1AndXor(a, b) = a ^ ROL(b,1) ✓ for all five d*, every XorRotateRight(x, d, n) is ROR(x^d, 64 - rho) matching the scalar RotateLeft(..., rho) for all 50 rotations ✓, and BitwiseClearXor(a, b, c) = a ^ (b & ~c) correctly encodes a ^ ((~c) & b) ✓. Nice.
But per robustness.md ("unsafe blocks must have a comment justifying the safety invariant") two things need to be written down:
ToVector128Unsafe()leaves the upper 64 bits uninitialized, so every one of these 60 vectors carries garbage in lane 1 for all 24 rounds. It's harmless only because the epilogue stores.GetLower()exclusively — includingeba/aba, where the round constant is XOR'd in via anotherToVector128Unsafe(). That is the single most important thing to know when reading this file and it isn't stated anywhere.- The
fixed (ulong* destination = &state)block (line 199) plus the 25 uncheckedVector64.LoadUnsafeoffsets rely onst.Length == 25, which is only enforced byDebug.Assert— fine for aninternalhelper, but say so.
A <remarks> on the method covering both, plus the SHA-3 / FIPS 202 reference, would cover this.
Also minor: dispatch in KeccakF gates on Sha3.IsSupported alone while the epilogue uses AdvSimd.Arm64.StorePair. SHA3 implies Advanced SIMD on ARM64 so this is safe in practice, but it's worth a word — and symmetrically, the 25 individual Vector64.LoadUnsafe calls in the prologue could use AdvSimd.Arm64.LoadPairVector64 to halve the load count the way the epilogue already halves the stores.
| [Test] | ||
| public void Arm_sha3_permutation_matches_scalar() | ||
| { | ||
| if (!Sha3.IsSupported) | ||
| { | ||
| Assert.Ignore("Arm SHA-3 intrinsics are not supported on this machine."); | ||
| } | ||
|
|
||
| const int stateLength = 25; | ||
| ulong[] input = new ulong[stateLength]; | ||
|
|
||
| for (int testCase = 0; testCase < 64; testCase++) | ||
| { | ||
| for (int lane = 0; lane < stateLength; lane++) | ||
| { | ||
| input[lane] = testCase switch | ||
| { | ||
| 0 => 0, | ||
| 1 => ulong.MaxValue, | ||
| _ => unchecked((ulong)(testCase * stateLength + lane + 1) * 0x9e3779b97f4a7c15UL) | ||
| }; | ||
| } | ||
|
|
||
| ulong[] expected = (ulong[])input.Clone(); | ||
| ulong[] actual = (ulong[])input.Clone(); | ||
|
|
||
| KeccakHash.KeccakF1600Scalar(expected); | ||
| KeccakHash.KeccakF1600ArmSha3(actual); | ||
|
|
||
| Assert.That(actual, Is.EqualTo(expected), $"Permutation mismatch for test case {testCase}."); | ||
| } | ||
| } |
There was a problem hiding this comment.
High — the AVX-512 path was rewritten in this PR and has no equivalent test; this ARM test likely never runs in CI.
Two coverage gaps, both on a consensus-critical hash:
-
No AVX-512 differential test.
KeccakF1600Avx512F's Pi step was restructured (all five chains re-rooted,c1Pi/c2Pi/c3Pi/c4Pirewritten) and the round body extracted into a sharedRound. Nothing here compares it againstKeccakF1600Scalar. The only coverage is incidental — the existingSanity_check/hash tests happen to route throughKeccakF, and only if the runner CPU has AVX-512. GitHub's Linux runners are a mix of Ice Lake (has it) and EPYC Rome/Milan (doesn't), so this is luck-of-the-draw rather than a guarantee. -
This ARM test almost certainly reports
Ignoredeverywhere. The unit-test workflows run on x64; the only arm64 runner in.github/workflowsisubuntu-arm64-8-coreinsync-pr-gate.yml, which doesn't run this suite. And even there, Ampere Altra / Neoverse N1 implements AES/SHA1/SHA2 but not the optional ARMv8.2 SHA3 extension — soSha3.IsSupportedis false and the assertions never execute. The new code path is nonetheless the default dispatch for Graviton3/4, Neoverse V1/V2 and Apple M-series, where a wrong Keccak means an instant chain split.
Since both intrinsic entry points are now reachable from tests, one parameterized test covers both and satisfies the DRY guidance in AGENTS.md (also gives per-case failure reporting instead of one 64-iteration loop):
[TestCaseSource(nameof(PermutationImplementations))]
public void Simd_permutation_matches_scalar(string name, bool supported, Action<ulong[]> permute)
{
if (!supported) Assert.Ignore($"{name} intrinsics are not supported on this machine.");
// ... existing 64-case body, calling permute(actual)
}
private static IEnumerable<object[]> PermutationImplementations() =>
[
["Arm SHA-3", Sha3.IsSupported, (Action<ulong[]>)(s => KeccakHash.KeccakF1600ArmSha3(s))],
["AVX-512", Avx512F.IsSupported, (Action<ulong[]>)(s => KeccakHash.KeccakF1600Avx512F(s))],
];For the ARM half, please also record in the PR description that you ran this on a real SHA3-capable CPU (M-series / Graviton3), since CI can't.
| // Pre-broadcast round constants (kills the per-round vmovq) | ||
| private static readonly Vector512<ulong>[] RoundConstantVec = | ||
| Array.ConvertAll(RoundConstants, rc => Vector512.CreateScalar(rc)); |
There was a problem hiding this comment.
Low — worth confirming this is actually a win, and it costs memory on ARM.
The comment claims this "kills the per-round vmovq", but the previous form was Vector512.Create(roundConstant, 0UL, …) where roundConstant came from Unsafe.Add(ref GetArrayDataReference(roundConstants), round) — one scalar load plus a vmovq. The replacement is a Vector512<ulong>[] element read: static-field load (hoistable) + an array bounds check that survives because round is loop-variant + a 64-byte vmovdqu64. That's plausibly more work per round, not less, and the 64-byte load can't fold into the vpxorq memory operand unless the JIT can prove 64-byte alignment of the array payload.
Also: this array is 24 × 64 B ≈ 1.5 KB allocated on the type's static constructor on every platform, including ARM64 where KeccakF1600Avx512F is never reached. Same for LaneShift1/LaneShift2/ThetaRot4 — as static readonly fields these depend on the JIT's static-readonly-struct folding to be as good as the inline Vector512.Create(const…) they replaced (which folds to a straight rodata load).
If the benchmark numbers back this up, great — please put them in the PR description, since KeccakPermutationBenchmarks doesn't measure the AVX-512 path at all.
| } | ||
|
|
||
| [Benchmark(Baseline = true)] | ||
| public ulong Scalar() | ||
| { | ||
| KeccakHash.KeccakF1600Scalar(_scalarState); | ||
| return _scalarState[0]; | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public ulong ArmSha3() | ||
| { | ||
| KeccakHash.KeccakF1600ArmSha3(_armSha3State); | ||
| return _armSha3State[0]; | ||
| } |
There was a problem hiding this comment.
Low — the benchmark doesn't cover the path the PR title says it tunes.
The commit is perf(core): add Arm SHA-3 Keccak-f[1600] permutation, **tune AVX-512 path**, but this class only benchmarks Scalar vs ArmSha3. On x64 the filter removes ArmSha3 and this degenerates to a single un-compared baseline. Adding an Avx512 case gated on Avx512F.IsSupported (same SimpleFilter shape) would let the AVX-512 restructuring be justified with numbers rather than asserted.
Separately, on the ARM structure itself: this ping-pong keeps the 25 a* vectors live while producing the 25 e* vectors, so peak liveness is ~50 Vector128 values against 32 NEON registers — and because only the lower 64 bits of each are used, every spill slot burns 16 bytes to carry 8 useful ones. XKCP/OpenSSL's SHA3-extension KeccakF1600 instead updates in place, fitting the state in v0–v24 with v25–v31 as temps. If the measured speedup over scalar is well under the ~2× these instructions usually deliver, spill traffic is the first thing I'd look at.
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
|
Benchmarked this on the ARM rig (2026-08-18, same-session A/B vs The Keccak change looks genuinely good:
The .NET 11 preview runtime it requires does not:
So the algorithm wins per call, but the runtime bump currently costs more than the Keccak saves, and it doubles the served tail. Since the Runs: EXPB 32164680943, multicall 32171943767/32171951062, corpus 32171926197/32171935889. (For contrast: the SVE2 Keccak experiment #12801 was closed — its permutation profiled 2.1× slower than scalar.) |
Changes
KeccakF1600ArmSha3, a Keccak-f[1600] permutation built on the ARMv8.2-SHA3 intrinsics (EOR3/RAX1/XAR/BCAXviaSha3.*), and prefer it inKeccakFdispatch when supportedRoundhelper: shared lane-index vectors for Theta/Chi and pre-broadcast round constants (removes the per-round scalar broadcast)KeccakF1600Scalar(internal) so tests and benchmarks can target it directlyKeccakPermutationBenchmarksBenchmarkDotNet suite11.0.100-preview.7inglobal.json,net11.0TFM,Microsoft.*11.0 preview packages, Dockerfiles/nix/CI) - required for theSystem.Runtime.Intrinsics.Arm.Sha3APITypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Arm_sha3_permutation_matches_scalarchecks the Arm path against the scalar reference over 64 states (all-zero, all-ones, pseudo-random lanes); it skips where SHA-3 intrinsics are unavailable, so it needs an ARMv8.2-SHA3 machine (e.g. Graviton3+, Apple M-series) to exercise the new codeKeccakPermutationBenchmarksre-verifies Arm/scalar equivalence inGlobalSetupbefore measuringKeccakTestshash vectors cover the AVX-512 restructuring: 1050 passed on x64 with AVX-512Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Building from source now requires the .NET 11 preview 7 SDK (see
global.json).