Clear upper vector registers after the LtHash AVX-512 kernels - #4164
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
PR SummaryLow Risk Overview This fixes dirty upper ZMM state left behind when the compiler does not clear registers after AVX-512, which was slowing subsequent legacy-SSE work in callers (e.g. Reviewed by Cursor Bugbot for commit 52b4651. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
LtHash default vs SIMD (
|
LtHash default vs SIMD (
|
There was a problem hiding this comment.
Adds a package-local vzeroupper() asm stub and calls it at the three SIMD→scalar handoff points in the LtHash AVX-512 backend. The change is correct: the stub is gated behind goexperiment.simd && amd64, is only reachable via simdBackend() (which requires AVX-512F+VBMI2, so the VEX-encoded instruction can never fault), the hash output is unaffected, and the len(data) > blake3ChunkLen early return correctly omits the call since no SIMD kernel ran on that path.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The
vzeroupper()call is repeated at the tail of each of the three SIMD kernels, so nothing enforces it for a kernel added later — a fourth*SIMDfunction that forgets the call silently reintroduces the dirty-upper-state penalty this PR removes.simdBackend()is the single point every SIMD entry point is registered through and could wrap the three function values there instead (at the cost of one indirect call), or at minimum a short note on thevzeroupperdeclaration stating that every SIMD kernel must end with it would make the requirement discoverable. Only theexpandSIMDsite carries the rationale comment today; theaddSIMD/subSIMDcalls are bare.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4164 +/- ##
==========================================
- Coverage 66.65% 65.59% -1.07%
==========================================
Files 2200 2091 -109
Lines 169420 157994 -11426
==========================================
- Hits 112933 103636 -9297
+ Misses 56346 54217 -2129
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Taken in 52b4651: the |
Follow-up to #4151. The Go compiler emits no
VZEROUPPERafterarchsimdAVX-512 code, so the LtHash SIMD kernels returned to their callers with dirty upper ZMM halves. Any legacy-SSE code that runs next (memmove, SHA-NI, encoding helpers) then pays an upper-state merge penalty until the registers are cleared; a throwaway benchmark ofExpandfollowed bysha256.Sum256over the 2 KiB serialisation goes 4.36 µs → 3.69 µs (-15%) on an Intel Xeon 8559C once the guard is in place, while the package's own benchmarks are neutral (MixIn +~1 ns, the cost of the instruction itself).This adds a package-local
vzeroupper()asm stub, the same shape as the one in #4157, and calls it from thin wrappers registered insimdBackend(), the one place every SIMD entry point passes through, so a kernel added later cannot skip it. Hash output is unchanged and the differential tests pass under both builds; the stub is only built undergoexperiment.simd && amd64.