majit: reject ambiguous boxing stores across phi roots by youknowone · Pull Request #1191 · youknowone/pyre

youknowone · 2026-08-13T01:26:19Z

What changed

make boxing-field lookup reject conflicting graph-wide stores instead of taking the first block-order match
follow block-input phis back to the producer variables that own header stores
require all merged roots to resolve to the same vtable
add regression coverage for conflicting and redundant payload/header stores
cite the corresponding RPython malloc lifetime/aliasing rules and keep the rationale concise

Why

fuse_boxing_alloc searched the whole graph for a matching (base, field) store but selected the first result. When a field was written differently on one branch, the chosen value depended on block order because this local pass has no reaching-definition analysis. Conversely, a header crossing a block boundary arrived as a phi while its stores remained attached to the producer variable, so a valid cluster could be declined.

The pass now fails closed on disagreeing stores and resolves phi roots before reading their stores. This matches RPython's rule that multiple creation points disable malloc optimization when aliasing cannot be proven safe.

Validation

cargo fmt --all -- --check
cargo check --features dynasm
cargo test --features dynasm
cargo test -p majit-translate fuse_boxing_alloc (9 passed)
python3 pyre/check.py --backend dynasm --no-synthetic --no-cpython-suite (17/17 passed)

Summary by CodeRabbit

Bug Fixes
- Improved fusion handling when multiple writes target the same field.
- Prevented fusion when conflicting values are written.
- Preserved fusion for repeated writes with identical values.
- Applied consistent validation across payload, header, and type fields.

`fuse_boxing_alloc` resolved a payload store and a header store by taking the first `FieldWrite` matching `(base, field)` anywhere in the graph. The scan is graph-wide because the stores feeding an aggregate sit in whichever block built it, and the pass has no reaching-definition analysis, so with two stores on one field it cannot tell which the malloc sees. Collect every matching store instead and return one only when they carry equal values; disagreeing stores leave `malloc_typed` residual for the fail-closed reject. `rpython/translator/backendopt/malloc.py:176-186` applies the same rule, downgrading a variable with more than one creation point to a plain use so the malloc optimisation is disabled for it. The reject census over the build-time lowering is unchanged at 9, all `w_long_from_raw` directly and through `unary_invert_value` / `unary_negative_value`, so no cluster that fused before declines now. Assisted-by: Claude

`store_roots` walks `Block.inputargs` / `Link.args` backwards to recover the op result a phi stands for. `rpython/translator/backendopt/malloc.py:169` reaches the same relation forwards, unioning each link arg with the inputarg it feeds into one lifetime; it can, because it owns a persistent whole-graph lifetime table this local rewrite has none of. `resolve_vtable_addr` derives the type pointer from the constructed value's `ob_header.ob_type`. `jtransform.py:1023 rewrite_op_malloc` reads it off the operation instead, because `malloc` carries the `STRUCT` and `heaptracker.get_vtable_for_gcstruct` answers from the type alone. `malloc_typed(value)` carries a value, which is also why the `w_class` check exists: a value can pair a base `ob_type` with a subclass `w_class` where a `STRUCT` cannot. Comments only. Assisted-by: Claude

coderabbitai · 2026-08-13T01:26:31Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fb2f5adb-a043-4dcf-90f1-34af84889230

📥 Commits

Reviewing files that changed from the base of the PR and between 0d83827 and 138cec8.

📒 Files selected for processing (1)

majit/majit-translate/src/model.rs

Walkthrough

fuse_boxing_alloc now validates that field stores are unique or identical before fusion. Conflicting payload, header, or type stores prevent fusion. Repeated identical stores remain eligible.

Changes

Boxing fusion validation

Layer / File(s)	Summary
Unique store resolution `majit/majit-translate/src/model.rs`	Field and payload resolution now uses `unique_store`. Conflicting writes no longer select the first matching store.
Fusion regression coverage `majit/majit-translate/src/model.rs`	Tests cover conflicting payload, header, and `ob_type` stores, plus identical repeated payload stores that still fuse.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

youknowone/pyre#606: Both changes update fuse_boxing_alloc in model.rs.
youknowone/pyre#1141: Both changes validate header and type stores before boxing fusion.
youknowone/pyre#1159: Both changes cover cross-block payload, header, and type store conflicts.

Poem

A rabbit checks each store in line,
Conflicts stop the fuse design.
Identical writes may still hop through,
Payload, headers, and types stay true.
Nibble nibble—tests approve!

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch str

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 138cec8235

ℹ️ 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".

chatgpt-codex-connector · 2026-08-13T04:37:18Z

+            if b != base || field.name.as_str() != field_name {
+                continue;


Track stores through block-input aliases

When an aggregate or header enters a branch as a fresh Block.inputargs variable, a FieldWrite in that block names the inputarg rather than the producer variable. The later store_roots call resolves the malloc argument back to the producer, but this exact-base comparison then ignores the aliased write and can fuse using the stale payload or vtable, changing the boxed value or type. The new test hides this case by writing through agg instead of arm_args[0]; the store census needs the same link-arg/inputarg equivalence that upstream establishes in rpython/translator/backendopt/malloc.py:169-179.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

github-actions · 2026-08-13T04:37:32Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 138cec8).
Updated: 2026-08-13T04:37:31.473Z

Files in the reviewed diff

majit/majit-translate/src/model.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

majit/majit-translate/src/model.rs:3000 ↔ rpython/translator/backendopt/malloc.py:489 — Pyre now rejects any differing repeated FieldWrite globally (“Some(_) => return None”), leaving malloc_typed residual. RPython instead updates the per-path flattened field value for every setfield (“newvarsmap[key] = op.args[2]”) and propagates those values through links (malloc.py:107), so sequential overwrites and branch-specific overwrites remain malloc-removable. The new test explicitly enshrines this divergence at model.rs:7874.
majit/majit-translate/src/model.rs:2975 ↔ rpython/translator/backendopt/malloc.py:176 — the new comment misattributes the conservative repeated-store rejection to RPython’s “aliasing problems” check. That upstream check is specifically for the same pointer appearing multiple times in a link when it has multiple creation points; it is not a rule rejecting multiple writes to one field.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

majit/majit-translate/src/model.rs:2950 ↔ rpython/rtyper/rbuiltin.py:349 — Pyre recognizes Rust’s lltype::malloc_typed(value) aggregate-copy idiom, whereas RPython lowers lltype.malloc(STRUCT, flags) with a statically supplied structure. This is a Rust/CPython-compatible compiler adaptation.
majit/majit-translate/src/model.rs:3145 ↔ rpython/jit/codewriter/jtransform.py:1023 — Pyre recovers the vtable from the constructed value’s ob_header.ob_type; PyPy derives it from the static STRUCT descriptor. This follows from the typed aggregate-copy lowering and is a structural adaptation, not a direct 1:1 port.

youknowone added 2 commits August 13, 2026 10:07

youknowone marked this pull request as ready for review August 13, 2026 04:34

youknowone merged commit 1c41feb into main Aug 13, 2026
15 of 17 checks passed

youknowone deleted the str branch August 13, 2026 04:35

chatgpt-codex-connector Bot reviewed Aug 13, 2026

View reviewed changes

Navigation Menu

majit: reject ambiguous boxing stores across phi roots - #1191

majit: reject ambiguous boxing stores across phi roots#1191
youknowone merged 2 commits into
mainfrom
str

youknowone commented Aug 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

Review failed

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Uh oh!

github-actions Bot commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		if b != base \|\| field.name.as_str() != field_name {
		continue;

Conversation

youknowone commented Aug 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Why

Validation

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 13, 2026

🤖 Codex parity review

1. 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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Aug 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading