majit: resolve boxing clusters across phi links - #1185
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (1)
WalkthroughChangesBoxing Header Fusion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CFGBlocks
participant store_roots
participant fuse_boxing_alloc
participant malloc_typed
CFGBlocks->>store_roots: pass block-input and predecessor header values
store_roots->>fuse_boxing_alloc: return resolved operation roots
fuse_boxing_alloc->>fuse_boxing_alloc: compare ob_type and w_class identities
fuse_boxing_alloc->>malloc_typed: preserve malloc_typed when roots do not resolve or agree
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: f5249009d5
ℹ️ 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".
| } | ||
| } | ||
| } | ||
| !out.is_empty() |
There was a problem hiding this comment.
When a header phi merges a locally constructed header with a function-entry input (or another inputarg block with no incoming edge) and the local predecessor is visited first, the recursive call for the entry input traverses zero links but returns true because the shared out already contains the earlier root. resolve_vtable_addr then validates only the local arm and replaces malloc_typed with its vtable even though the other arm can carry an arbitrary header, miscompiling ob_type/w_class; require each invocation to contribute at least one root, or collect roots independently per arm.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
![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
🤖 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/model.rs`:
- Around line 2992-2993: Remove the hard-coded eight-hop traversal limit used by
store_roots and its caller. Update the header-resolution traversal around
store_roots and the budget setup near chain(8) to derive a cycle-safe bound from
the graph size or track visited (block, inputarg slot) states, allowing valid
chains of eight or more links while terminating cycles. Add a chain(8)
regression case covering successful fusion.
🪄 Autofix
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: 790b9203-9f66-4a02-9792-ef1032983887
📒 Files selected for processing (2)
majit/majit-translate/src/model.rsmajit/majit-translate/src/translator/rtyper/flowspace_adapter.rs
| if depth == 0 { | ||
| return false; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Remove the fixed eight-hop header-resolution limit.
Line 3144 sets the traversal budget to 8. store_roots returns false at Line 2992 before it reaches a producer after eight links. A valid boxing header with eight or more preceding call boundaries remains unfused. It then reaches the fail-closed malloc_typed path in flowspace_adapter.rs and falls back to the legacy walker.
Derive the cycle-safe budget from the graph size, or track visited (block, inputarg slot) states. Add a chain(8) regression case.
Proposed change
- if !store_roots(graph, &header, 8, &mut roots) {
+ let max_phi_hops = graph.blocks.len().min(u32::MAX as usize) as u32;
+ if !store_roots(
+ graph,
+ &header,
+ max_phi_hops.saturating_add(1),
+ &mut roots,
+ ) {
return 0;
} ("header crosses two links", &|| chain(2), 1),
+ ("header crosses eight links", &|| chain(8), 1),Also applies to: 3143-3145, 7495-7507
🤖 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/model.rs` around lines 2992 - 2993, Remove the
hard-coded eight-hop traversal limit used by store_roots and its caller. Update
the header-resolution traversal around store_roots and the budget setup near
chain(8) to derive a cycle-safe bound from the graph size or track visited
(block, inputarg slot) states, allowing valid chains of eight or more links
while terminating cycles. Add a chain(8) regression case covering successful
fusion.
The comment and the TyperError text enumerated the fusing owners as W_Float/W_Int/W_Complex/W_Long. That list named W_LongObject, which declines -- its header pairs &LONG_TYPE with get_instantiate(&INT_TYPE), so the subclass guard refuses it -- and fuse_boxing_alloc is not numeric-only (see fuse_boxing_alloc_lowers_non_numeric_struct_generically). State the condition the pass applies instead of listing types. The substring `survived fuse_boxing_alloc unfused` is unchanged; cutover.rs matches on it at two sites. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b1e8c11). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b96132d5c0
ℹ️ 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".
| // declines. Using the root also keeps `Site.aggregate` naming the | ||
| // variable a `core::ptr::write` arm stores, which is what | ||
| // `sink_fused_boxing_aggregates_at_raw_writes` matches on. |
There was a problem hiding this comment.
When the aggregate crosses the branch links—the production shape this change newly supports—the raw-write arm consumes its block inputarg rather than the producer root stored in Site.aggregate (the added fixture itself writes gc_args[0]). sink_fused_boxing_aggregates_at_raw_writes only checks args[1] == aggregate, so it misses that write and leaves the aggregate/header construction in the dominator, causing it to remain on the ordinary JIT allocation path instead of performing the documented RPython-equivalent escape-sensitive motion. Resolve the write operand through the links, or retain the phi aliases for matching.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
The aggregate reaches `malloc_typed` as a `Block.inputargs` phi whenever the constructor builds the struct in a dominator and the blocks between end in calls: `w_float_new` builds the `W_FloatObject` before `gc_interp::enabled()` and `try_gc_alloc_stable_raw`. Owner, payload stores and `ob_header` are all looked up by exact `%agg`, so the cluster declined for want of a ctor -- the canonical float constructor never fused, and `w_list_getitem` / `w_tuple_getitem_known` reached the legacy walker through it. Resolve `%agg` to its ctor root before those lookups, and decline when the roots disagree: a merge of two aggregates has no single owner or payload set. Taking the root also keeps `Site.aggregate` naming the variable a `core::ptr::write` arm stores, which is what `sink_fused_boxing_aggregates_at_raw_writes` matches on. Assisted-by: Claude
1 participant
fuse_boxing_allocdecides whether a boxing cluster can become aNewWithVtableby looking up four things — the aggregate's owner, its payload stores, itsob_headerstore, and the header'sob_type/w_class. Every one of those lookups matches on an exact variable. Butresolve_addr, in the same function, followsBlock.inputargslinks, and its doc comment explains exactly why it has to: "each call ends a block, so the header value crosses the boundary as a link arg while its producer stays in the predecessor."A field store is recorded against the producing variable. So any value that crosses a block boundary — one preceding call is enough — becomes a phi at the use site, and every exact-match lookup silently finds nothing. The cluster then declines, however constant its type pointer is. The comment describing the hazard sits two functions above the code that ignores it.
This turned out to bite at two levels.
The header crosses (commit 1)
A per-reason census of the build-time lowering put all nine declining sites on one verdict — and not the one the code reported.
resolve_vtable_addrcollapsed "noob_typestore" and "stored value isn't constant" into a singleelsevia.and_then, so the reason read as unresolvable constant when nothing was being resolved at all:header-from=phiwithheader-fields=-, uniform across 9/9.store_rootsresolves a phi back to the op-result variables it can be. The field lookup then runs per root, and every root must agree on the vtable — the same agreement ruleresolve_addralready applies to a merged pointer. A header that is one of two types still declines.Fused clusters went 6 → 14. Newly fusing:
w_str_new,w_str_from_wtf8,w_str_from_wtf8_immortal,w_bytes_from_bytes,w_bytearray_alloc,w_set_new,w_frozenset_new,w_dict_proxy_new.The ninth site,
w_long_from_raw, now reportswclass-is-a-subclass, which is correct: it pairsob_type: &LONG_TYPEwithw_class: get_instantiate(&INT_TYPE), so the deliberate subclass guard refuses it. The old code declined it for the wrong reason and never reached that check.The aggregate crosses (commit 3)
Fixing the header exposed the same defect one level up. Investigating what still rejected showed the residual sites were not the interesting ones I expected — three of five were
w_float_new, the canonical float constructor the pass's own showcase test is written around, and it had never fused in production.w_float_newbuilds the wholeW_FloatObjectbeforegc_interp::enabled()andtry_gc_alloc_stable_raw, each of which ends a block, then stores it either throughcore::ptr::write(the GC arm) orlltype::malloc_typed. So the aggregate arrives as a phi, all three%agg-keyed lookups miss, and the verdict isno-ctor— 21 census sites.w_list_getitemandw_tuple_getitem_knownwere reaching the legacy walker through it.Same remedy: resolve
%aggto its ctor root before those lookups, declining when the roots disagree, since a merge of two aggregates has no single owner or payload set. Taking the root also keepsSite.aggregatenaming the variable acore::ptr::writearm stores, which is whatsink_fused_boxing_aggregates_at_raw_writesmatches on.What still rejects
Graphs rejected with
survived fuse_boxing_alloc unfusedwent from 24 (steady at 24–26 over several days before this work) to 9, and all nine are now one cause:w_long_from_raw, directly and throughunary_invert_value/unary_negative_value. That is the by-design subclass refusal above, so it is not a missed fusion — closing it would mean teaching the rewrite to emit an explicitw_classstore rather than letting the vtable stand for it, which is a design change, not a fix.The count rising from 5 to 9 midway is first-blocker unmasking, not a regression: with the float constructor fused, graphs that used to stop there now get far enough to reach the long refusal.
Tests
fuse_boxing_alloc_resolves_a_header_that_crosses_a_link— header crossing one link, two links, a merge naming one type (fuses), a merge naming two types (declines).fuse_boxing_alloc_resolves_an_aggregate_that_crosses_a_link— thew_float_newshape: shared aggregate,core::ptr::writearm andmalloc_typedarm. It also asserts the GC arm still stores a fully constructed object after the rewrite.Both are mutation-checked: reverting the corresponding resolution fails that test and no other. The pre-existing
..._only_when_the_links_agreetest was blind to both, because itscluster_inhelper builds the header ctor, the outer ctor and themalloc_typedin one block, so only theob_type/w_classvalues ever crossed a boundary.Commit 2
The reject's comment and
TyperErrortext enumerated the fusing owners asW_Float/W_Int/W_Complex/W_Long. That list was already wrong before this change — it namedW_LongObject, which declines, and the pass is not numeric-only (fuse_boxing_alloc_lowers_non_numeric_struct_generically) — and this change widens the set further. It now states the condition the pass applies instead of listing types. The substringsurvived fuse_boxing_alloc unfusedis unchanged;cutover.rsmatches on it at two sites.Verification
cargo test -p majit-translate— 3187 lib tests plus the integration binaries, all passing.python3 ./pyre/check.py—cranelift 424/424,wasm 417/417.One pre-existing failure, checked rather than assumed
The full gate reports
test.test_pickle PASS -> FAILon dynasm (test_deep_nested_struct_frozenset, 3 errors).origin/mainfails identically under the same command, so it is not from this change.Getting that right took a second attempt, and the first answer was wrong in a way worth recording.
w_set_new/w_frozenset_neware among the constructors this PR newly fuses, so a frozenset failure looked like an obvious consequence, and there was a plausible mechanism to hand. But the two arms were not comparable: the base had been measured with--no-syntheticwhile the failing runs were the full gate, which runs 425 synthetic benchmarks first. Re-running the base with the same command reproduced the failure exactly.The trigger is that preceding load, not the code: the module passes standalone (
run.py --filter test_pickle, at both--jobs 1and--jobs 17) and undercheck.py --no-synthetic, on both arms.🤖 Generated with Claude Code