Align assembly encoders with Go#47
Conversation
Final results (enwik9, -cpu=1) | level | original asm | now | Go (noasm) | speed | |---|---|---|---|---| | `-1` Fastest | 46.97% | **44.91%** (+375 B) | 44.91% | 673 MB/s (Go 563) | | `-2` Balanced | 38.30% | **37.80%** (−875 B) | 37.81% | 390 MB/s (Go 373) | Both asm encoders now match-or-beat the Go reference and stay faster. Every bit of the gap traced to a concrete bug — nothing diffuse. All three bugs (all quality-only → invisible to roundtrip/fuzz) 1. L1 hash1 stored s instead of s+1 → dead s+1 probe (46.97→45.29). 2. L1 post-match JA→JG → unsigned underflow killed near matches in each block's first ~2 MB (45.29→44.91). 3. L2 sparse index stored index1 instead of index2 → half the match interior filed under a stale position (38.30→37.80). Plus: L1 store-order set to Go's (your call — OoO-friendly), L2 repeat-interior indexing + bail aligned.
📝 WalkthroughWalkthroughFixes in ChangesAssembly Generator Correctness Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_generate/gen.go (1)
1397-1416: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not leave the non-CMOV offset path with a nil
minPos.Line 1397 only allocates
minPoswhenmatchOffsetCMOVis true, but the false branch still dereferencesminPosat Line 1415. Flipping this generator knob will panic during generation; allocateminPosfor both branches or remove the dead branch.Proposed fix
- if o.maxOffset > maxOffset && matchOffsetCMOV { + if o.maxOffset > maxOffset { minPos = GP64() LEAL(Mem{Base: s, Disp: -maxOffset + 2}, minPos.As32()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@_generate/gen.go` around lines 1397 - 1416, The non-CMOV offset path in the generator still uses minPos even when it was only initialized inside the matchOffsetCMOV branch, which can cause a nil dereference when the else path is selected. Update the offset-check setup around checkCandidate in gen.go so minPos is allocated and initialized whenever either branch can reference it, or restructure the logic so the non-CMOV path no longer depends on minPos. Keep the fix localized to the offset handling around checkCandidate, matchOffsetCMOV, and minPos.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@_generate/gen.go`:
- Around line 1566-1603: The repeat interior indexing loop in
encodeBlockBetterGo needs the same sLimitL safety check used by the match-emit
path before reading src[index1] and src[index1+1]. Add the guard around the
repeat_index_loop setup so the code only enters this indexing block when s is
within the safe range, preventing out-of-bounds reads while still indexing the
repeat interior into sTab and lTab.
---
Outside diff comments:
In `@_generate/gen.go`:
- Around line 1397-1416: The non-CMOV offset path in the generator still uses
minPos even when it was only initialized inside the matchOffsetCMOV branch,
which can cause a nil dereference when the else path is selected. Update the
offset-check setup around checkCandidate in gen.go so minPos is allocated and
initialized whenever either branch can reference it, or restructure the logic so
the non-CMOV path no longer depends on minPos. Keep the fix localized to the
offset handling around checkCandidate, matchOffsetCMOV, and minPos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd784de2-ea28-440a-9478-fa61c776ee3d
📒 Files selected for processing (2)
_generate/gen.goasm_amd64.s
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
_generate/gen.go (1)
1395-1419: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
minPosallocation condition no longer matches its use in the non-CMOV branch.Adding
&& matchOffsetCMOVmeansminPosis only allocated on the CMOV path, but theelsebranch incheckCandidate(Lines 1415) still dereferencesminPos.As32(). IfmatchOffsetCMOVis ever setfalsewhileo.maxOffset > maxOffset,minPosisniland the generator panics. It's currently masked only becausematchOffsetCMOVis aconst true, so the branch is dead. Gate theelsebranch on the same condition or keep the allocation aligned with both consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ee562fa-934c-4611-a3eb-072df56afee7
📒 Files selected for processing (2)
_generate/gen.goasm_amd64.s
💤 Files with no reviewable changes (1)
- asm_amd64.s
1 participant
Final results (enwik9, -cpu=1)
1Fastest2BalancedBoth asm encoders now match-or-beat the Go reference and stay faster. Every bit of the gap traced to a concrete bug — nothing diffuse.
All three bugs (all quality-only → invisible to roundtrip/fuzz)
Plus: L1 store-order set to Go's (your call — OoO-friendly), L2 repeat-interior indexing + bail aligned.
Summary by CodeRabbit