zstd: bump avo pin, regenerate arm64 asm with BMI2 lowering#1167
Merged
klauspost merged 2 commits intoJul 10, 2026
klauspost:masterklauspost/compress:masterfrom
honeycombio:lizf.zstd-arm64-bmi2-avo-bumphoneycombio/compress:lizf.zstd-arm64-bmi2-avo-bumpCopy head branch name to clipboard
Merged
zstd: bump avo pin, regenerate arm64 asm with BMI2 lowering#1167klauspost merged 2 commits intoklauspost:masterklauspost/compress:masterfrom honeycombio:lizf.zstd-arm64-bmi2-avo-bumphoneycombio/compress:lizf.zstd-arm64-bmi2-avo-bumpCopy head branch name to clipboard
klauspost merged 2 commits into
klauspost:masterklauspost/compress:masterfrom
honeycombio:lizf.zstd-arm64-bmi2-avo-bumphoneycombio/compress:lizf.zstd-arm64-bmi2-avo-bumpCopy head branch name to clipboard
Conversation
Bumps the pinned avo commit (honeycombio/avo) past today's fixes: a flag-liveness bug and a 32-bit CMPL/TESTL fold bug (neither actually reachable from this codebase - seqdec's generated output is byte-identical without them, and CMPL/TESTL aren't used here at all), plus a new EQ/NE-only sub-32-bit CMPW/TESTB lowering that fse_decoder's buildDtable needs. Checked whether the sub-word compare/test fold (the general form of which the new avo now refuses to lower) actually produced wrong output in the already-shipped fse_decoder_arm64.s: it didn't. All three call sites happened to operate on registers already clean above the compared width (via an upstream sign/zero-extending load, or a preceding bounds check), verified by tracing register provenance by hand. Real risk, but mitigated by accident rather than by design - which is exactly why the stricter check is worth having going forward. The new avo also prefers the BMI2 twin over the generic one when lowering to arm64 (arm64 has native flag-free equivalents for the BMI2 idioms), so seqdec_arm64.s now lowers sequenceDecs_decode_bmi2/decodeSync_bmi2 instead of the generic variants. Symbol names are unaffected (arm64Name strips the _bmi2/_amd64 suffix either way). Validated: go vet and go test ./zstd/... pass natively on arm64; FuzzDecodeAll (26.4M execs), FuzzDecoder (9.6M execs), and FuzzDecAllNoBMI2 pass with zero crashers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR updates arm64 FSE decoder comparison logic, changes generated arm64 BMI2 skip annotations, and adjusts the generator module’s avo dependency and replace target. ChangesARM64 FSE Decoder Fix
ARM64 BMI2 Skip Annotation Update
Code Generator Dependency Update
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
klauspost
approved these changes
Jul 9, 2026
klauspost
left a comment
Owner
There was a problem hiding this comment.
How does the changes affect performance?
lizthegrey
added a commit
to honeycombio/avo
that referenced
this pull request
Jul 9, 2026
… generic Benchmarking klauspost/compress#1167 on real hardware (Apple M4 and Cortex-A72) showed the unconditional "prefer BMI2" policy is a real regression, not the win its doc comment assumed: seqdec_decode/decodeSync are 13-50% slower with the BMI2 twin lowered, on both chips. BMI2 x86 code is tuned for x86 (BEXTR packs into one instruction what arm64 needs two UBFX to unpack; the flag-free shifts avoid an x86 partial-register stall arm64 doesn't have), so mechanically lowering it is not reliably faster once ported. BMI2 lowering support itself isn't wrong to have -- a generator may only emit a BMI2 variant with no generic twin, and that still needs to lower correctly. What was wrong was defaulting to prefer it whenever a choice exists without measuring. Add Config.ARM64PreferBMI2 (default false) so a caller who HAS measured a win for their case can opt in; a function with only one variant is unaffected either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Benchmarking the BMI2-preferred seqdec on real hardware (Apple M4, Cortex-A72) showed it's 13-50% slower than the generic twin, not faster as assumed -- see the follow-up avo commit for why. Bump past that fix; avo's arm64 lowering now defaults to the generic twin (ARM64PreferBMI2 defaults false), which this repo doesn't override. seqdec_arm64.s is now byte-identical to what klauspost#1160 shipped, aside from the skip-reason comment text; fse_decoder_arm64.s is unchanged (the CMPW/TESTB fix has no BMI2 twin to choose between). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Performance Good question to ask — it caught a real regression. This was mostly a correctness fix, but it also switched With BMI2 preferred, M4: with BMI2 preferred (reverted)M4: this PR (BMI2 off) vs #1160Cortex-A72: with BMI2 preferred (reverted)Cortex-A72: this PR (BMI2 off) vs #1160 |
Contributor
Author
|
Thanks. For changelog note PR title is deceptive, I didn't fix it in time. |
Owner
|
Ah, yeah, I fixed the title when merging only. No biggie. |
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
2 participants
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.
Follow-up to #1160. Bumps the pinned avo commit (mmcloughlin/avo#486, still open upstream) past a few fixes made since the pin was cut, and regenerates
seqdec_arm64.s/fse_decoder_arm64.s.What changed in avo
MOVbetween a flag producer and theJcc/CMOVconsuming it could leave the branch reading stale flags. Fixed with a backward dataflow scan instead of a single-instruction lookahead.CMPL/TESTLfolding to a 64-bit compare (reads garbage from unproven upper bits). Now lowered natively at 32-bit width.CMPW/CMPB/TESTW/TESTBfor the narrow case where every consumer is EQ/NE only — equality doesn't depend on sign, so zero-extending both operands before a full-width compare is correct regardless of the surrounding dataflow.buildDtable'snewState == uint16(u)check needs this; ordering consumers still fail loud.seqdec_arm64.snow lowerssequenceDecs_decode_bmi2/decodeSync_bmi2instead of the generic_amd64variants. Symbol names are unaffected.Each fix has an executable differential test (amd64 native as reference, arm64 native/qemu under test) in avo's
tests/arm64lower/.Did this affect already-shipped v1.19.0?
I checked whether the sub-word compare fold (the general form of which avo now refuses to lower) actually produced wrong output in the currently-shipped
fse_decoder_arm64.s. It didn't, in any of the three places it's used:v == -1: the source was already sign-extended via aMOVHload, so the old 64-bitCMNwas comparing an already-correct value.nBits == 0self-test: the source was already zero-extended viaMOVBU, so the old 64-bitTSTwas equivalent to the intended 8-bit self-test.newState == u: both operands are provably ≤tableSizeby that point (the immediately preceding bounds check, andubeing a small loop counter), so the 64-bit compare and a correct 16-bit compare agree.I also isolated the flag-liveness and
CMPL/TESTLfixes by regenerating against an intermediate avo commit with only those two fixes:seqdec_arm64.scame out byte-identical to what's shipping, and neither generator usesCMPL/TESTLat all. So none of today's avo fixes changed actual decoder behavior — the risky pattern was real but happened to be safe everywhere it was actually used, which is exactly why the stricter check is worth having going forward rather than relying on that being true forever.Validation
go vetandgo test ./zstd/...pass natively on arm64 (darwin/arm64).FuzzDecodeAll: 26.4M execs, 0 crashers.FuzzDecoder: 9.6M execs, 0 crashers.FuzzDecAllNoBMI2: pass..sconfirmed to still match the//go:noescapedeclarations inseqdec_arm64.go/fse_decoder_asm.go.avo#486 is still open upstream; this bump points at the honeycombio/avo fork the same way #1160 did.
Summary by CodeRabbit