Repository navigation
jit: pin typed-array descr bases to the target word - #733
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughArray descriptor layout calculations now use target word sizing, and ChangesArray layout base corrections
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit e027645). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@majit/majit-translate/src/codewriter/call.rs`:
- Around line 7454-7492: Update the test
typed_block_and_gc_typed_array_bases_diverge_on_a_narrow_word so the equality
between array_items_base and gc_typed_array_items_base is asserted only when
target_word_size() is 8; for a 4-byte word, assert each expected base
independently and preserve the existing divergence check.
- Around line 2172-2175: Update arraydescrof_concrete so its base_size
calculation branches on is_struct: use gc_typed_array_items_base for struct
arrays and retain array_items_base for typed blocks. Ensure both paths use the
shared LLType::Array(path_hash(array_type_id)) descriptor consistently, and add
a regression test exercising both cache-initialization orders on 32-bit targets.
🪄 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
Run ID: 746de709-97da-49cd-8dc0-6118bcb38dbd
📒 Files selected for processing (2)
majit/majit-translate/src/codewriter/assembler.rsmajit/majit-translate/src/codewriter/call.rs
| // Interior-field struct arrays address `GcTypedArray`, whose items sit | ||
| // flat at the length word — NOT the element-aligned offset the list | ||
| // int/float storage (`TypedItemsBlock`) needs in `arraydescrof_concrete`. | ||
| let base_size = self.gc_typed_array_items_base(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Apply the GC base rule to the primary cached descriptor path too.
arraydescrof_concrete still computes struct-array base_size with array_items_base at Lines 1601-1604, then caches it under the same LLType::Array(path_hash(array_type_id)) key used here. On a 32-bit target, whichever path runs first can pin the shared descriptor to the wrong base (for example, 8 instead of the GcTypedArray base 4), causing incorrect interior-field addresses.
Branch the primary calculation on is_struct and use the GC base for struct arrays; retain array_items_base only for typed blocks. Add a regression test covering both cache-initialization orders.
As per coding guidelines, this is a generated-JIT semantic divergence and must be fixed rather than justified by host/Rust layout differences.
Proposed fix
let base_size = match len_offset {
None => 0,
+ Some(off) if is_struct => off + self.gc_typed_array_items_base(),
Some(off) => self.array_items_base(
off + self.array_header_size,
elem_ref,
item_size,
),
};🤖 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 `@majit/majit-translate/src/codewriter/call.rs` around lines 2172 - 2175,
Update arraydescrof_concrete so its base_size calculation branches on is_struct:
use gc_typed_array_items_base for struct arrays and retain array_items_base for
typed blocks. Ensure both paths use the shared
LLType::Array(path_hash(array_type_id)) descriptor consistently, and add a
regression test exercising both cache-initialization orders on 32-bit targets.
Source: Coding guidelines
…escr bases to a 32-bit word Extract the interior-field struct-array base (arraydescrof interior path) into `CallControl::gc_typed_array_items_base`, mirroring `array_items_base`. The two block rules — GcTypedArray flat at the length word vs TypedItemsBlock element-aligned — are now named methods rather than an inline field read, so routing a struct array through the element-aligned rule is a visible change. Add regression tests that pass an explicit 4-byte header_end to reproduce a 32-bit word on the 64-bit host, asserting the element-aligned i64/f64 base (8) diverges from the flat word base (4) that the two blocks coincide on at word size 8.
The `arraydescrof` fallback taken when no `CallControl` is threaded computed its length-prefixed base with the host `size_of::<usize>()`; a cross-target extraction would bake the host stride. Use `target_word_size()`, matching the sweep the `CallControl` path already followed. This path is shape-only for GC arrays (flat at the length word) and is reached only by the codewriter-less `assemble()` test entry, so no generated descr changes.
`vable_arraydescrof` hardcoded its length-prefixed base to the host `size_of::<usize>()`. The array is prefixed by a single length word (`len_offset = Some(0)`), so its items start at the target word; `target_word_size()` bakes the right stride on a narrower target. No-op on the native backends (host word == target word); wasm 288/288.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165ab069fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert_eq!( | ||
| cc.array_items_base(word, Some("i64"), 8), | ||
| cc.gc_typed_array_items_base(), | ||
| ); |
There was a problem hiding this comment.
When this test is run on an actual 32-bit target, target_word_size() returns 4, so array_items_base(4, Some("i64"), 8) returns 8 while gc_typed_array_items_base() returns 4; this assertion fails before the test reaches the intended explicit 32-bit divergence checks. Please gate this equality on word == 8 or pass an explicit 8-byte header for the 64-bit case.
Useful? React with 👍 / 👎.
The extraction stamp hashed the host-filtered dependency closure and the host pass's cargo args; the layout sidecar passes' own inputs sat outside it, so a change to a cross-target-only path dep, to `layout_cargo_args`, or to the layout RUSTFLAGS left a stale sidecar behind an unchanged stamp. Union the fingerprint closure over the host and every layout target platform (`cargo metadata --filter-platform` per platform, memoised), and stamp the resolved layout cargo args and `layout_target_rustflags`. The sidecar flag resolution is extracted into `crate_layout_flags`, shared by the stamp and the extraction pass. Verified: full re-extraction under the new stamp, then a second run skips all three crates on "fingerprint unchanged".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3424fd4834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # drop deps whose every listed kind is `dev`. | ||
| if dep_kinds and all(kind.get("kind") == "dev" for kind in dep_kinds): | ||
| for platform in sorted(platforms): | ||
| meta = metadata(eng, cargo_features, platform) |
There was a problem hiding this comment.
When a CrateSpec sets layout_cargo_args to a different feature set, this metadata walk still uses the host cargo_features rather than the actual layout_flags passed to the sidecar extraction below. If those sidecar-only flags enable a path dependency or switch features for a target layout pass, edits in that dependency are omitted from the stamp, so the fingerprint skip can reuse a stale .layouts.ullbc even though the sidecar compile would consume the changed source. Please walk metadata for the layout cargo args as well as the host args.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
The locals_cells_stack_w vable arraydescr itemsize was a hardcoded 8 in pyre-jit-trace/build.rs; PyObjectRef items are one target word, so the descr would mis-stride FixedObjectArray on a narrower target. Pass majit_translate::layout::target_word_size() there, make the jtransform default and the codewriter-less arraydescrof fallback's Ref arm follow the same rule, and assert in vable_arraydescrof that the itemsize fits the target word — a wider element would be under-aligned by the flat-word items base. Two unit tests pin the pointer-item descr shape and the wider-than-word panic.
…es into the union closure metadata() now keys its memo on the engine root, metadata_feature_crates, cargo_features, platform and the new extra_features instead of (cargo_features, platform). fingerprint_inputs() parses the features a crate's layout_cargo_args enable (features_in_cargo_flags, bare names qualified as crate/feature) and passes them to every metadata query so the union closure includes dependencies enabled only by the layout passes. --no-default-features is ignored: dropping it only widens the graph.
Follow-ups to the target-aware layout work (#707), each keeping a build-time
descr base tied to the target word rather than the build host's.
Name the
GcTypedArrayitems-base rule and pin both typed-array descr bases to a 32-bit wordThe two runtime typed-array blocks place their items differently, and a descr's
base must match the block it addresses:
TypedItemsBlock(unboxed list int/float storage) — items element-aligned(
i64/f64at 8), computed byCallControl::array_items_base.GcTypedArray(the resume/blackhole materialiser and interior-field structarrays) — items flat at the length word.
The interior-field struct-array base was an inline
self.array_header_sizereadwith a long comment. It is now a named
CallControl::gc_typed_array_items_basemirroring
array_items_base, so routing a struct array through theelement-aligned rule (the earlier C5 regression) is a visible change to a named
method rather than an easily-"unified" inline expression.
Added regression tests that reproduce a 32-bit word on the 64-bit host by
passing an explicit 4-byte
header_endtoarray_items_base(the scalar pathnever consults
target_word_size), asserting the element-alignedi64/f64base (8) diverges from the flat word base (4) that the two rules coincide on at
word size 8 — the divergence a host-only assertion cannot otherwise see.
Use the target word for the codewriter-less array-base fallback
The
arraydescroffallback taken when noCallControlis threaded computed itslength-prefixed base with the host
size_of::<usize>(). Usetarget_word_size(),matching the sweep the
CallControlpath already followed. This path isshape-only for GC arrays (flat at the length word, no element alignment) and is
reached only by the codewriter-less
assemble()test entry, so no generateddescr changes.
Verification
pyre/check.pygreen on all three backends (dynasm 291/291, cranelift 291/291,wasm 288/288) after a fresh LLBC extraction.
Summary by CodeRabbit