Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. Walkthrough
ChangesARM64 register-to-memory addition
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change is intended to correct ARM64 register-to-memory addition and add regression coverage. No concrete remaining failure is established in the supplied review context, so no actionable merge blocker is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Correct ARM64 MacroAssembler register-to-memory add64 destination order, restoring DFG typed-array accounting while preserving the source register. Add the 169-pair testmasm regression and native ARM64 qualification across interpreter, baseline, DFG, FTL and concurrent-GC modes. Bring the four already-qualified promise stack goldens from the release line to main so their expected columns match the existing syntax-token semantics. Preserve every assertion. Keep qualification environment variables out of JSC's engine-option namespace. Native macOS and Linux ARM64 pass the assembler regression and full qualification selection. The upstream two-file correction is oven-sh#770; immutable artifact publication is handled by the release-line counterpart.
ARM64Assembler::add takes destination first. Correct MacroAssemblerARM64's register-to-memory add64 so it writes the sum to memory and preserves the source register, restoring DFG typed-array allocation accounting. Add a testmasm regression covering 169 operand pairs, 64-bit wraparound, source preservation and neighboring memory. Qualify native macOS and Linux ARM64 with 466 assembler tests, 1,778 stress and 1,639 module configurations, FFI and five accounting/sampling modes. Require five archive-bound Windows ARM64 accounting modes before publication while preserving the ten-archive matrix and upstream recipes. The native regression fails before the correction and passes after. Upstream patch: oven-sh#770.
1 participant
MacroAssemblerARM64::add64(RegisterID src, Address dest)passes its operands toARM64Assembler::addin the wrong order. That lower-level API takes(destination, left, right), so the current helper overwritessrcwith twice the old memory value and then stores the unchanged temporary. PutdataTempRegisterfirst to implement the existing*dest += srccontract and preservesrc.The testmasm regression checks 169 pairs of 64-bit operands, including wraparound and signed extrema, as well as source-register preservation and neighboring memory. On native macOS ARM64 the regression fails before the fix (expected memory 1, actual 0), passes after, and the complete 466-test assembler suite passes. The same two changed files also pass all 466 assembler tests on native Linux ARM64. The downstream engine qualification passes 1,778 stress configurations, 1,639 module configurations, FFI, and five explicit accounting and sampling modes on each platform. Native Linux ARM64 proof.
An added downstream DFG typed-array accounting path exposed this. At the current pinned source, the upstream allocation helper's register-to-memory add64 callers are guarded by
CPU(X86_64), and Air permits these memory forms only on x86-64. I found no production ARM64 caller in this revision. The sibling ARM64 add/sub/and/or memory helpers and xor64 counterpart use the correct operand order.