keccakf_arm64: drop unused 200-byte frame, mark NOSPLIT - #12
keccakf_arm64: drop unused 200-byte frame, mark NOSPLIT#12AskAlexSharov wants to merge 4 commits into
Conversation
The TEXT directive declared a 200-byte stack frame that the function never touches — the whole state lives in V registers. The frame forces a stack-bound check and frame setup/teardown on every permutation call (once per 136-byte block). Declare NOSPLIT $0-16 instead. ~7% faster on single-block hashes on an Apple M-series (BenchmarkFasterKeccak/32B: 137.3ns -> 128.0ns, p=0.000, n=8); larger sizes within noise.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the arm64 Keccak-f[1600] permutation assembly by removing an unused stack frame and marking the function as NOSPLIT, reducing per-call overhead in the hot permutation path.
Changes:
- Change
TEXT ·keccakF1600Sha3from a$200-16frame toNOSPLIT, $0-16on arm64. - Eliminate the implicit stack growth check and prologue/epilogue associated with a non-zero frame.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7dffb4d to
f792e6b
Compare
lint.yml tells auditors to diff this file against Go's crypto/internal/fips140/sha3/sha3_arm64.s. That file declares $200-8, so the frame change here shows up as an unexplained difference and is easy to revert on the next upstream re-sync. Record why it is there instead.
domiwei
left a comment
domiwei
left a comment
There was a problem hiding this comment.
Approved. The zero-frame ABI0 leaf is safe: the generated symbol has no stack-pointer writes, calls, or morestack path, and the argument layout remains correct. One non-blocking wording comment: NOSPLIT makes the current no-split intent explicit, but it does not mechanically require the frame to remain zero; a future edit can add a frame while retaining the flag. Please consider adjusting the comment so it does not imply that NOSPLIT guards against future spills or frames.
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
The $0 frame is correct for the current body — the state never leaves V0–V24, and arm64 CI genuinely exercises this path. Requesting changes for one runtime regression and a few accuracy/durability points.
Preemption regression (blocking). The dropped stack-growth check was the only cooperative preemption point in the absorb/squeeze block loops, and asm bodies are never async-preemptible, so one large Sum256/Write call now stalls stop-the-world for its full duration. Measured max GC pause (/sched/pauses/stopping/gc): from ~164 µs on master to ~42 ms at 32 MiB input and ~168 ms at 128 MiB, with every P in the process blocked meanwhile. Latent at erigon's ≤24 KiB inputs, but Sum256/Write are public and size-unbounded; stdlib's fips140 sha256 chunks its asm calls at 64 KiB for exactly this hazard. Please chunk the Go-side block loops (~64 KiB), or keep a ≥128-byte frame.
Claims that don't hold (each checked empirically):
- "NOSPLIT records that requirement" — it doesn't:
NOSPLIT, $64-16with real frame use passes build, vet, and tests silently. Worse, a ≥128-byte frame added under a kept NOSPLIT silently loses the stack check it would otherwise get. Reword as unenforced intent, or drop the sentence (same wording in the file comment and the PR description). - "Keep the $0 when re-syncing" needs its condition: only while the synced body still never touches the stack. A named-local spill under
$0assembles into a below-SP store with no diagnostic — green on darwin/linux, memory corruption where below-SP isn't safe (ios/windows arm64). - Verification section: asmdecl validates argument size and FP offsets only, not the frame size —
NOSPLIT, $16-16passesgo vetclean on this exact file. Please correct it, so the next re-syncer doesn't trust vet for the one field this PR changes.
Divergence durability. The next upstream re-sync is a hand merge (upstream renamed the function to keccakF1600NEON), and the comment sits in the exact hunk such a merge rewrites — nothing would catch $200 silently returning. Suggest a one-line CI grep pinning NOSPLIT, $0-16 (ci.yml's amd64 drift check exists for exactly this failure class) and a clause in lint.yml's diff -w audit note naming the intended TEXT-line delta. Filing the one-liner upstream would let the delta converge instead of being hand-carried forever. While reworking the comment: the prologue mechanics and the $200-8 literal will rot; the two load-bearing facts are "body never touches the stack" and "upstream declares a dead frame — don't restore it".
Minor. Commit f792e6b still says "~7% faster", which this description retracts — squash-merge with the corrected text or reword the commit. Optional: PCALIGN $16 before rounds: restores the loop-head 16-byte alignment the prologue removal shifts (loop head moves from entry+0x110 to entry+0xf4; cost is 3 one-time NOOPs) and is a plausible mechanism for the +0.74% (p=0.001) 128B datum. FYI: the frameless leaf drops the immediate caller from Linux perf FP-only callchains on arm64 (Go pprof unaffected).
|
Reproduced every point. The preemption regression is real, and chasing it turned up that the same bug already ships on amd64 — so it is now split into #32, and this PR looks like it should be dropped. Preemption regression — confirmedMax GC stopping pause while hashing, M4 Max,
Same magnitude as your numbers. The same bug already ships on amd64
Fixed in #32 by removing I also built the chunking fix you asked for — absorb and squeeze returning every 16 KiB to a function that carries a stack check. It works (0.049 ms) but costs 5.5-8.9% geomean and ~16% at 32 B, and the cost persisted even when the small-input path executed code byte-identical to master, so it is package layout rather than the added logic. Removing This PR does not appear to buy anythingM4 Max,
No size improves, 500 KB regresses. That matches your Your accuracy points — all confirmed
Left alone deliberately: the |
The arm64
TEXTdirective declared a 200-byte stack frame that the function never uses — the entire Keccak state lives in V0–V24 and never spills (this is likely a leftover from an earlier version that spilled to the stack). A non-zero frame makes the function non-leaf-like: the assembler emits a stack-bound check plus frame setup/teardown on every permutation call, i.e. once per 136-byte block.Change to
NOSPLIT, $0-16.What actually changes
Disassembly of the prologue, before:
after:
plus the matching epilogue and the morestack tail.
$0alone is what removesthem (a leaf with a zero frame gets no prologue at all);
NOSPLITisdocumentation — it records the requirement for anyone who later adds a frame.
Benchmark
Honest answer: not measurable on a wide out-of-order core. Apple M4 Max,
-count=8, benchstat:Seven instructions are noise next to 24 permutation rounds here. The change
stands on correctness and code size rather than a speedup, and it is more
likely to show up on narrow/in-order ARM cores where the removed check is a
larger share of per-call work. (An earlier revision of this description quoted
-6.8% on 32B; that did not reproduce and has been retracted.)
Verification
go vet ./...(asmdecl validates the frame/argument sizes against the Godeclaration) — clean, native and
GOOS=linux GOARCH=arm64go test ./...on native arm64 — the path this assembly actually takes