perf: experiment with SVE2 Keccak on ARM64 - #12801
Conversation
|
Claude finished @kamilchodola's task in 6m 13s —— View job Review: SVE2 Keccak experimentI hand-verified the permutation algebra and reviewed the gating, codegen shape, and benchmark. The math is correct; the problems are in verification coverage, the failure mode of the gate, and a code shape that probably can't win the benchmark it was written for. Verdict: needs work before merge — 3 High, 3 Medium, 2 Low. Nothing is a live production risk today (the path is off by default), but the High items should land before anyone runs this on hardware. What I verified as correct ✅I checked the permutation lane by lane against the Keccak-f[1600] spec:
Findings
One design observationBeyond the individual findings: broadcasting one scalar lane across the vector and reading back only Separately:
No code changes made — this was a review request. I did not build or run tests: the SVE2 path cannot execute on this x64 runner, and the author already reports clean Release builds for both touched projects. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
|
@claude re-review |
|
Claude finished @kamilchodola's task in 7m 38s —— View job Re-review: SVE2 Keccak experiment (after
|
| # | Prior severity | Status |
|---|---|---|
| 1 | High — cctor throw poisons KeccakHash |
✅ Fixed. No throw path remains; Unsupported/VerificationFailed both fall back. Verified: on x64 IsSve2KeccakSupported() short-circuits at Sve2.IsSupported before the P/Invoke. |
| 2 | High — no correctness verification | ✅ Fixed. Enable-time self-test over zero / all-ones / non-byte-periodic states, plus a platform-gated Sve2_permutation_matches_scalar test and the fresh-process fallback regression. |
| 3 | High — Span<Vector<ulong>> through memory |
✅ Addressed. All 60 vectors are named locals now, [SkipLocalsInit] present, no vector stackalloc. The Vector<T>-in-array hazard is gone entirely. |
| 5 | Medium — missing [SkipLocalsInit] |
✅ Fixed. |
| 6 | Medium — benchmark can't A/B | ✅ Fixed. KeccakPermutationBenchmark calls both permutations directly, verifies equality in setup, InProcessNoEmitToolchain + AlwaysUseLocal gives a same-process comparison. The API-level ComputeHash benchmark was correctly dropped. |
| 4 | Medium — HWCAP2_SVESHA3 gate |
🟡 Acknowledged with rationale — not blocking. See the RAX1 comment: keeping the gate is the conservative direction, but then the code should use the instruction the gate guarantees. |
| 7, 8 | Low — magic 25, redundant (byte) casts, pragma scope |
⬜ Not addressed. |
Correctness — re-verified, exact ✅
The new version is a line-for-line transliteration of KeccakF1600Scalar, which makes it checkable mechanically rather than by algebra. I did both:
- All 48 XAR immediates are
64 − scalar ROL amount, in both half-rounds. Spot list:44→20,43→21,21→43,14→50,28→36,3→61,45→19,61→3,1→63,6→58,25→39,8→56,18→46,27→37,36→28,10→54,15→49,56→8,62→2,55→9,39→25,41→23,2→62— all correct, and the odd-rounde*block repeats them identically. - Operands and assignment targets match the scalar version lane for lane, including the deliberately reordered
bCi/bCe/bCasequence at the start of the odd half-round. Sve2.Xor(Xor(a,b,c),d,e)= the 5-way column XOR ✓.BitwiseClearXor(bCa, bCi, bCe)=bCa ^ (bCi & ~bCe)= scalarbCa ^ ((~bCe) & bCi)✓ for all 50 sites. ι applied toeba/abawithround/round + 1✓.- VL-agnostic: broadcast in, lane 0 out, no cross-lane ops — correct for any SVE vector length.
KeccakF1600→KeccakF1600Scalarrename has no dangling references anywhere in the repo ✓.
Findings
| # | Severity | Issue |
|---|---|---|
| 1 | Medium | GlobalSetup throws on every non-SVE2 host → the README's documented --filter "*Benchmark*" run of the precompiles assembly now produces a failed benchmark on x64. Verified against Nethermind.Benchmark.Runner/Program.cs:78-82. Use a BDN IFilter to decline selection instead of throwing. |
| 2 | Medium | Child-process pipe deadlock + no timeout in Experimental_sve2_opt_in_on_unsupported_host_does_not_poison_KeccakHash. Sequential ReadToEnd() on stdout then stderr with both redirected; a chatty test host fills the stderr buffer and both sides block, then bare WaitForExit() turns it into a job-timeout hang rather than a failure. |
| 3 | Medium | The status enum reports nothing and both catches swallow silently. Unsupported and VerificationFailed are indistinguishable from Disabled at runtime. If the opt-in silently falls back on c9gd, the EXPB run reads as "SVE2 gives no speedup" when SVE2 never executed — a wrong conclusion from a healthy node. Also flags the load-bearing RoundConstants-before-ExperimentalSve2Keccak declaration order, which nothing documents. |
| 4 | Medium | RAX1 is unused despite the SHA3 gate. RotateLeftOne + Xor is 4 vector ops × 5 lanes × 2 half-rounds × 24 = 960 ops/permutation that RAX1 does in 240 — roughly 19% of the permutation's instruction count, and the gate already guarantees the instruction is present. |
| 5 | Low | 48 redundant (byte) casts on the XAR immediates (implicit constant conversion covers literals); #pragma warning disable SYSLIB5003 at line 64 spans the DllImport too — only Sve2.IsSupported needs it. |
| 6 | Low | Literal 25 in VerifySve2Keccak and KeccakTests; STATE_SIZE / sizeof(ulong) already exists (.agents/rules/coding-style.md — constants over reused magic numbers). |
| 7 | Low | The fresh-process test spawns a full test host on every Nethermind.Core.Test run on x64, and passes on exit code alone — if the --filter ever stops matching, coverage vanishes silently while still green. |
| 8 | Low | [assembly: InternalsVisibleTo("Nethermind.Precompiles.Benchmark")] is new surface for a benchmark's sake. Acceptable, but it means Nethermind.Core internals are now reachable from a benchmark project by default — worth a moment's thought before it becomes a habit. |
Performance outlook — still unresolved, and this is the point of the PR
The named-locals rewrite removed the guaranteed memory round-trip, but it does not remove spilling: 60 live Vector<ulong> locals against 32 Z-registers means the allocator must spill, and SVE spill slots are VL-sized (str z/ldr z), so on a 256-bit-VL part each spill moves 2× the bytes the scalar version's 64-bit spills do. The scalar path is the same 60-local shape against 31 X-registers and spills too — so the comparison is "wide spills + ~19% fewer arithmetic ops (more if RAX1 lands)" versus "narrow spills + more ops."
On top of that, per KeccakF call the SVE path pays 25 broadcasts on entry and 25 lane-0 extracts on exit, and KeccakF runs twice for a typical 32-byte hash. And the fundamental ceiling stands: broadcasting one lane and reading back [0] does Vector<ulong>.Count× redundant work for 1× useful output.
None of that is a defect — it's why the disassembly step you've still got open is the real gate. I'd suggest landing RAX1 first so the measurement reflects the best version of this shape.
What I could not check
dotnet is not permitted on this runner, so I neither built nor ran tests. In particular I could not confirm that Sve2.Xor(3-arg), Sve2.XorRotateRight and Sve2.BitwiseClearXor have exactly these signatures in .NET 10 — I'm relying on your reported clean Release build for both touched projects. The SVE2 path also cannot execute here, so the two new platform-gated tests self-Ignore.
- Read the updated diff (
git diff origin/master...HEAD) - Re-verify the permutation after the locals rewrite — all 48 rotates + 50 BCAX sites + ι confirmed against the scalar path
- Check the new self-test, status enum, and fallback gate
- Review the new tests and permutation benchmark
- Post findings (4 inline comments + this summary)
No code changes made — this was a review request.
• branch perf/arm64-keccak-sve2-experiment
| public void Setup() | ||
| { | ||
| if (!KeccakHash.IsSve2KeccakSupported()) | ||
| throw new PlatformNotSupportedException("KeccakPermutationBenchmark requires SVE2 SHA3 support."); |
There was a problem hiding this comment.
Medium — throwing from [GlobalSetup] breaks the documented "run all precompile benchmarks" command on every non-SVE2 host.
Nethermind.Benchmark.Runner runs BenchmarkSwitcher.FromAssemblies(...).Run(benchmarkArgs, ...) over an assembly list that includes typeof(KeccakBenchmark).Assembly (Program.cs:78-82), and this project's README documents:
dotnet run -c Release --project Nethermind.Benchmark.Runner -- --filter "*Benchmark*"*Benchmark* matches KeccakPermutationBenchmark, so on x64 — i.e. every dev box and CI runner today — this class is selected and its GlobalSetup throws. BDN turns that into a failed benchmark entry plus error output in the middle of an otherwise-clean precompiles run. The class is unrunnable there by design; it should decline to be selected rather than fail.
Cleanest fix that keeps the class discoverable on the target: add an IFilter to InProcessConfig that returns false when KeccakHash.IsSve2KeccakSupported() is false, so BDN excludes the benchmarks instead of erroring. Keep the hard throw only for the state-mismatch check below — that one should fail loudly.
Smaller note on the same method: _scalarState and _sveState are permuted once in Setup and then keep being permuted in place across iterations, so the two benchmarks run over diverging state. Keccak-f timing is data-independent, so this doesn't bias the A/B — worth a one-line comment saying so, since it looks like a bug otherwise.
| string standardOutput = process.StandardOutput.ReadToEnd(); | ||
| string standardError = process.StandardError.ReadToEnd(); | ||
| process.WaitForExit(); |
There was a problem hiding this comment.
Medium — this is the classic Process pipe deadlock, and there's no timeout to break out of it.
Both StandardOutput and StandardError are redirected, and they're drained sequentially: the parent blocks in StandardOutput.ReadToEnd() until the child closes stdout. If the child writes more than the stderr pipe buffer (~64 KB on Linux) before finishing, the child blocks writing stderr, the parent blocks reading stdout, and neither ever proceeds. Then WaitForExit() with no timeout means the hang lasts until the GitHub Actions job timeout rather than failing the test.
A test host is exactly the kind of child that can produce bulk stderr — MSBuild/MTP diagnostics, a runtime warning, an AssemblyLoadContext complaint. It'll pass locally and hang CI on the one run where it matters.
using Process process = Process.Start(startInfo)
?? throw new InvalidOperationException("Could not start the Keccak child test process.");
Task<string> standardOutputTask = process.StandardOutput.ReadToEndAsync();
Task<string> standardErrorTask = process.StandardError.ReadToEndAsync();
if (!process.WaitForExit(ChildProcessTimeoutMs))
{
process.Kill(entireProcessTree: true);
Assert.Fail("The Keccak child test process did not exit in time.");
}
string standardOutput = standardOutputTask.GetAwaiter().GetResult();
string standardError = standardErrorTask.GetAwaiter().GetResult();Related (Low): the assertion is ExitCode == 0 only, so if the --filter ever stops matching Experimental_sve2_opt_in_child_computes_known_hash — a rename, an MTP option change — the regression silently stops covering anything while still passing. Assert on something proving the test actually executed (parse the TRX in resultsDirectory, or check the child output for one passed test).
| private static readonly ExperimentalSve2KeccakStatus ExperimentalSve2Keccak = GetExperimentalSve2KeccakStatus(); | ||
|
|
||
| private static ExperimentalSve2KeccakStatus GetExperimentalSve2KeccakStatus() | ||
| { | ||
| try | ||
| { | ||
| if (Environment.GetEnvironmentVariable("NETHERMIND_EXPERIMENTAL_SVE2_KECCAK") != "1") | ||
| return ExperimentalSve2KeccakStatus.Disabled; | ||
|
|
||
| if (!IsSve2KeccakSupported()) | ||
| return ExperimentalSve2KeccakStatus.Unsupported; | ||
|
|
||
| return VerifySve2Keccak() ? ExperimentalSve2KeccakStatus.Enabled : ExperimentalSve2KeccakStatus.VerificationFailed; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| return ExperimentalSve2KeccakStatus.VerificationFailed; | ||
| } | ||
| } | ||
|
|
||
| #pragma warning disable SYSLIB5003 | ||
| internal static bool IsSve2KeccakSupported() | ||
| { | ||
| try | ||
| { | ||
| return OperatingSystem.IsLinux() | ||
| && RuntimeInformation.ProcessArchitecture == Architecture.Arm64 | ||
| && Sve2.IsSupported | ||
| && (GetAuxiliaryValue(AT_HWCAP2) & HWCAP2_SVESHA3) != 0; | ||
| } | ||
| catch (Exception) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Medium — the four-state status enum is computed and then thrown away, and both catch blocks swallow the exception with no trace. A silent fallback will corrupt the experiment it exists to serve.
The static-initializer poisoning problem is properly fixed — thanks. But the only consumer is ExperimentalSve2Keccak == ExperimentalSve2KeccakStatus.Enabled at line 87, so Unsupported and VerificationFailed are indistinguishable from Disabled at runtime, and nothing reports them. Concretely: an operator sets NETHERMIND_EXPERIMENTAL_SVE2_KECCAK=1 on the c9gd box, the HWCAP2 bit isn't set (see the SVE2-vs-SVE2-SHA3 point below — that's the likely way to land here) or the self-test trips, and the node silently runs the scalar path. The EXPB numbers then get read as "SVE2 Keccak gives no speedup" when SVE2 Keccak never ran. That's a wrong conclusion drawn from a working node — the worst outcome for a benchmark experiment.
.agents/rules/robustness.md also asks for at minimum a log on a swallowed exception; catch (Exception) { return VerificationFailed; } discards the reason entirely, so a self-test failure and a missing getauxval look identical.
Minimal fix that stays inside Nethermind.Core (no logger available here): keep the exception in a field and expose the status, then have a startup step log it.
internal static ExperimentalSve2KeccakStatus Sve2KeccakStatus => ExperimentalSve2Keccak;
internal static Exception? Sve2KeccakFailure { get; private set; }Even a one-off Console.Error.WriteLine on Unsupported/VerificationFailed when the opt-in was explicitly requested would be enough for an experiment. If you'd rather not surface it at all, then the enum is dead differentiation and should collapse back to a bool.
Two smaller points on this block:
- Static-initializer ordering is a load-bearing invariant with nothing marking it.
VerifySve2KeccakcallsKeccakF1600Scalar, which readsRoundConstants. That works only becauseRoundConstants(line 24) is declared textually beforeExperimentalSve2Keccak(line 45) in the same file — field initializers run in declaration order. Move either field toKeccakHash.cs, or reorder them, and the self-test dereferences a null array, thecatchconverts it toVerificationFailed, and the feature silently never enables. Worth one comment on line 45 stating the dependency. - The
#pragma warning disable SYSLIB5003at line 64 spansIsSve2KeccakSupportedand thegetauxvalDllImport; only theSve2.IsSupportedreference needs it.
| da = Vector.Xor(bCu, RotateLeftOne(bCe)); | ||
| de = Vector.Xor(bCa, RotateLeftOne(bCi)); | ||
| di = Vector.Xor(bCe, RotateLeftOne(bCo)); | ||
| @do = Vector.Xor(bCi, RotateLeftOne(bCu)); | ||
| du = Vector.Xor(bCo, RotateLeftOne(bCa)); |
There was a problem hiding this comment.
Medium (performance) — you're gating on HWCAP2_SVESHA3 and then not using the one instruction it guarantees. RAX1 collapses these five lines from 20 vector ops to 5.
RAX1 is defined as Zd = Zn EOR ROL(Zm, 1) — which is exactly da = bCu ^ ROL(bCe, 1). Right now each of the five θ D[x] values costs ShiftLeft + ShiftRightLogical + BitwiseOr + Xor = 4 vector ops, so 20 per half-round, 40 per loop body, 960 per permutation. With RAX1 that's 5 / 10 / 240.
Rough per-half-round vector op count for this implementation is ~80 (10 for the five 5-way EOR3 column XORs, 20 for D[x], 25 XAR, 25 BCAX), so this is ~19% of the whole permutation's instruction count — material for an experiment that has to beat a fully-unrolled register-resident scalar path.
It also makes the gating self-consistent. Since you're keeping the HWCAP2_SVESHA3 requirement (fine by me — it's the conservative direction and you've given your rationale), RAX1 is guaranteed present whenever this code runs, so there is no portability argument for open-coding it. As written, the gate rejects hardware over an instruction the code never issues.
One factual correction for the record, since it affects who can run the experiment: in the Arm ARM, SVE EOR3, BCAX and XAR are baseline FEAT_SVE2; FEAT_SVE2_SHA3 adds only RAX1. GCC's sve2-sha3 flag enables the extension, but the feature table listing those mnemonics together is about what the flag permits, not about what each instruction requires. Practical consequence: on an SVE2 part without the SHA3 crypto extension the path silently falls back with no log (see my other comment) — so if the c9gd numbers come back flat, check the status before concluding anything. Using RAX1 makes this moot.
1 participant
Changes
EOR3,XAR, andBCAX) with named vector locals and no vector-backed stack arrays.NETHERMIND_EXPERIMENTAL_SVE2_KECCAK=1, .NET SVE2 support, LinuxHWCAP2_SVESHA3, and a successful scalar-equivalence self-test.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
dotnet test Nethermind.Core.Test/Nethermind.Core.Test.csproj -c Release --no-restore --filter FullyQualifiedName~Keccak— 1,066 passed; two expected skips on unsupported x64.dotnet build Nethermind.Precompiles.Benchmark/Nethermind.Precompiles.Benchmark.csproj -c Release --no-restore— passed with zero warnings and errors.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
This remains a controlled benchmark experiment. Target acceptance requires the full Keccak suite with the opt-in enabled, the direct scalar/SVE benchmark, and disassembly confirming
EOR3,XAR, andBCAXwithout excessive spill traffic.