Repository navigation
majit: resolve a forced instance's w_class in optimize_getfield, not virtualize (#915 P1) - #920
Conversation
…virtualize `virtualize.py:184-195 optimize_GETFIELD_GC_*` folds a field read only under `opinfo.is_virtual()` and otherwise emits, leaving a non-virtual's read to `heap.py`. The w_class arm added in f20f992 folded for `PtrInfo::Instance` too, which runs a pass earlier than that layering allows. `force_box` installs the forced instance with an EMPTY field list (info.rs:1113-1118, so heap's `do_setfield` cannot MUST_ALIAS-elide the materializing SETFIELD_GC) and emits the header write as an ordinary `SetfieldGc`, where it sits in `CachedField::lazy_set`. The virtualize arm therefore saw no stored value on every forced instance and answered from the size descr's canonical class, discarding a pending retag to a user subclass. Move both resolutions into `optimize_getfield`, after the cache consult: a value already in the instance's own slot wins, otherwise the layout's class answers only when no `w_class` store is still pending. The pending check scans every `is_w_class()` descr rather than the one being read, because the caches are keyed by `Arc::as_ptr` (`cached_field_pos_for_descr`) and pyre mints more than one descr for this header, so a miss on the read descr does not prove the field is untouched. The slot is looked up through the layout's own w_class fielddescr, never the read descr's index. Append loop steady body stays at 31 ops. Assisted-by: Claude
The loop reads `grown[num_short_jump_args..]` right after testing `grown.len() == num_short_jump_args`, so a list that ever came back shorter would slice out of bounds instead of taking the "did not grow" exit. Test with `<=`. Raised on #556, which added the surrounding guard. Assisted-by: Claude
WalkthroughThe optimizer now preserves forced Changesw_class read handling
Short-preamble fixpoint
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 54e8fc9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![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: 54e8fc976e
ℹ️ 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".
| if !self.w_class_set_pending() { | ||
| if let Some(w_class) = size_descr |
There was a problem hiding this comment.
When forcing a virtual whose w_class was retagged, force_box can queue the materializing SETFIELD_GC under the layout descriptor while the subsequent read uses the walker descriptor. Detecting that pending store here merely suppresses the constant fold; execution then falls through and emits the GETFIELD_GC before the lazy store is flushed, so the load observes the allocation's canonical class and a following guard emits the retag too late. Resolve the pending value for the same object or force its store before allowing the load to be emitted.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
| // `<=`, not `==`: the list only grows here, but the slice below | ||
| // indexes from `num_short_jump_args`, so a shrink would panic | ||
| // rather than fall into the "did not grow" exit. | ||
| if grown.len() <= num_short_jump_args { |
There was a problem hiding this comment.
Upstream rpython/jit/metainterp/optimizeopt/unroll.py:425 uses equality here, and the new comment itself states that this list can only grow, so the <= branch adds behavior only for an unreachable shrink rather than fixing a real slicing path. Keep the literal upstream condition instead of introducing an unnecessary structural divergence in this line-by-line port.
AGENTS.md reference: AGENTS.md:L194-L196
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-metainterp/src/optimizeopt/heap.rs`:
- Around line 2250-2269: Update the special `w_class` lookup in the surrounding
heap optimization logic to derive the layout slot via `Self::field_slot_index()`
rather than `index_in_parent()`, matching `force_lazy_set_field` and
`w_class_descr()`. Ensure `w_class_set_pending()` checks pending stores under
both the parentless and parented `w_class` descriptor cache entries.
🪄 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: 763a0aa7-e5d9-4441-b6c9-c87f0f374fc0
📒 Files selected for processing (3)
majit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/optimizeopt/unroll.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rs
| if descr.as_field_descr().is_some_and(|fd| fd.is_w_class()) { | ||
| let obj_box = ctx.get_box_replacement_operand(obj); | ||
| if let Some(crate::optimizeopt::info::PtrInfo::Instance(iinfo)) = | ||
| ctx.peek_ptr_info(&obj_box) | ||
| { | ||
| let size_descr = iinfo.descr.as_ref().and_then(|d| d.as_size_descr()); | ||
| let stored = size_descr | ||
| .and_then(|sd| { | ||
| sd.all_fielddescrs() | ||
| .iter() | ||
| .find(|fd| fd.is_w_class()) | ||
| .map(|fd| fd.index_in_parent() as u32) | ||
| }) | ||
| .and_then(|widx| { | ||
| iinfo | ||
| .fields | ||
| .iter() | ||
| .find(|(idx, _)| *idx == widx) | ||
| .map(|(_, e)| e.clone()) | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare w_class descr spellings and the slot index used by reads vs writes.
set -euo pipefail
# All w_class descr constructions and their parent/index setup.
rg -nP -C 6 '\bis_w_class\b' --type=rust
# Slot index helper and every structinfo write site.
rg -nP -C 4 'fn field_slot_index|structinfo_setfield\(' --type=rustRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked rust files around majit:"
git ls-files | rg '(^|/)majit/majit-metainterp/src/optimizeopt/heap\.rs$|.*\.rs$' | rg 'majit|maji' | head -200 || true
echo
repo_files=$(git ls-files '*.rs' | wc -l)
echo "total rust tracked files: $repo_files"
if [ "$repo_files" -gt 2000 ]; then
echo "limitting rust search by repository dirs"
fi
echo
echo "locating is_w_class and w_class-related text in Rust files:"
rg -n --glob '*.rs' -C 4 'is_w_class|w_class|field_slot_index|structinfo_setfield|force_lazy_set_field|lazy_set' . || true
echo
echo "file size for target file:"
wc -l majit/majit-metainterp/src/optimizeopt/heap.rs 2>/dev/null || trueRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="majit/majit-metainterp/src/top/optimizeopt/heap.rs"
if [ -f "$target" ]; then
echo "FOUND $target"
wc -l "$target"
else
echo "target missing at $target"
git ls-files | rg 'majit/.*/optimizeopt/heap\.rs|maxit/.*/optimizeopt/heap\.rs|.*opt/heap\.rs$' || true
fi
echo
echo "search is_w_class in target if present:"
if [ -f "$target" ]; then
rg -n -C 6 'is_w_class|field_slot_index|structinfo_setfield|force_lazy_set_field|w_class_set_pending|lazy_set|PendingFieldInfo|PENDINGFIELDSTRUCT' "$target"
fi
echo
echo "line ranges around cited code if present:"
if [ -f "$target" ]; then
sed -n '880,955p' "$target" | nl -ba -v880
echo "---"
sed -n '2230,2290p' "$target" | nl -ba -v2230
echo "---"
sed -n '2710,2760p' "$target" | nl -ba -v2710
fiRepository: youknowone/pyre
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="majit/majit-metainterp/src/optimizeopt/heap.rs"
wc -l "$target"
echo
echo "targeted search in heap.rs:"
rg -n -C 4 'is_w_class|field_slot_index|structinfo_setfield|force_lazy_set_field|w_class_set_pending|lazy_set|PENDINGFIELDSTRUCT' "$target"
echo
echo "cited line ranges in heap.rs:"
sed -n '880,955p' "$target" | nl -ba -v880
echo "---"
sed -n '2230,2290p' "$target" | nl -ba -v2230
echo "---"
sed -n '2710,2760p' "$target" | nl -ba -v2710
echo
echo "file size and module-level imports for types:"
sed -n '1,120p' "$target" | nl -ba -v1Repository: youknowone/pyre
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="majit/majit-metainterp/src/optimizeopt/heap.rs"
echo "field descriptions around header:"
sed -n '190,300p' "$target" | nl -ba -v190
echo
echo "structinfo_setfield implementations:"
rg -n -C 12 'fn structinfo_setfield|structinfo_setfield\(' majit/majit-metainterp/src/optimizeopt majit/majit-lower src -g '*.rs' || true
echo
echo "cached_fields keys and w_class lookup implementation:"
sed -n '22,27p' "$target" | nl -ba -v22
sed -n '533,565p' "$target" | nl -ba -v533
sed -n '846,930p' "$target" | nl -ba -v846
sed -n '1030,1065p' "$target" | nl -ba -v1030
sed -n '2240,2305p' "$target" | nl -ba -v2240
sed -n '2358,2480p' "$target" | nl -ba -v2358
sed -n '2522,2555p' "$target" | nl -ba -v2522Repository: youknowone/pyre
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="majit/majit-metainterp/src/optimizeopt/heap.rs"
echo "field descriptions around header:"
sed -n '190,300p' "$target"
echo
echo "structinfo_setfield implementations:"
rg -n -C 12 'fn structinfo_setfield|structinfo_setfield\(' majit/majit-metainterp/src/optimizeopt majit/majit-lower pyre/pyre-jit-trace/src -g '*.rs' || true
echo
echo "cached_fields keys and w_class lookup implementation:"
sed -n '190,230p' "$target"
echo "---"
sed -n '533,565p' "$target"
echo "---"
sed -n '846,930p' "$target"
echo "---"
sed -n '1030,1065p' "$target"
echo "---"
sed -n '2240,2305p' "$target"
echo "---"
sed -n '2358,2480p' "$target"
echo "---"
sed -n '2522,2555p' "$target"
echo "---"
sed -n '2701,2775p' "$target"Repository: youknowone/pyre
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "OptContext::structinfo_setfield full implementation:"
sed -n '8124,8170p' majit/majit-metainterp/src/optimizeopt/mod.rs
echo
echo "cached_fields tuple semantics:"
sed -n '22,28p' majit/majit-metainterp/src/optimizeopt/heap.rs
echo
echo "pyre w_class descriptor construction relevant parts:"
sed -n '1908,1954p' pyre/pyre-jit-trace/src/descr.rs
sed -n '1038,1007p' pyre/pyre-jit-trace/src/descr.rs || sed -n '996,1058p' pyre/pyre-jit-trace/src/descr.rs
echo
echo "read-only verifier of field_slot_index semantics for w_class spellings:"
python3 - <<'PY'
class FieldDescr:
def __init__(self, index, parent):
self.index = index
self.parent = parent
def get_parent_descr(self):
return self.parent
def index_in_parent(self):
return self.index_in_parent
def field_slot_index(descr):
if descr.get_parent_descr() is None:
return descr.index
return descr.index_in_parent()
# Mirrors pyre: singleton PyObject.w_class has index_in_parent=0 and parent=None.
parentless = type("Descr", (), {"index": 10, "as_field_descr": lambda self: type("Fd", (), {"index_in_parent": lambda: 0, "get_parent_descr": lambda: None})()})()
# Mirrors size_descr sibling/w_class field: has parent; parent-local index = 1.
parented = type("Descr", (), {"index": 0, "as_field_descr": lambda self: type("Fd", (), {"index_in_parent": lambda: 1, "get_parent_descr": lambda: object()})()})()
print("parentless_w_class_slot", field_slot_index(parentless))
print("size_sibling_w_class_slot", field_slot_index(parented))
print("parented_structure_match", field_slot_index(parented) == 1)
PYRepository: youknowone/pyre
Length of output: 5431
Align the flushed w_class stored field slot with the layout spelling.
force_lazy_set_field restores flushed w_class stores with Self::field_slot_index(&put_back_op.getdescr().unwrap()), which uses index() for the parentless w_class_descr(). The special lookup here reads only the parented layout spelling, and w_class_set_pending() only checks that cache entry. Use Self::field_slot_index() for the layout’s w_class fielddescr and check pending stores across both w_class cachings.
🤖 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-metainterp/src/optimizeopt/heap.rs` around lines 2250 - 2269,
Update the special `w_class` lookup in the surrounding heap optimization logic
to derive the layout slot via `Self::field_slot_index()` rather than
`index_in_parent()`, matching `force_lazy_set_field` and `w_class_descr()`.
Ensure `w_class_set_pending()` checks pending stores under both the parentless
and parented `w_class` descriptor cache entries.
|
Both P1s and the CodeRabbit finding are correct, and this merged before they could be acted on. Addressed in #922. They converge on one conclusion: the heap-side fold is still unsound in two distinct ways. When a store is pending under the other descr spelling, suppressing the fold only declines the constant — the Neither is patchable without stacking another special case on the descr split, and upstream has no counterpart to port: The — commented by Claude |
…izer (#920 review) (#922) * Revert "majit: exit the short-preamble force loop on a non-growing arg list" This reverts commit 54e8fc9. `unroll.py:425` uses equality, and the list at this point can only grow, so `<=` covered an unreachable shrink at the cost of a divergence from the line-by-line port. #556 left the condition literal for that reason. Assisted-by: Claude * majit: stop resolving a forced instance's w_class header in the optimizer The fold cannot be made sound while reads and writes of this header carry different descr spellings. `optimize_getfield` resolved it after the cache consult, gated on no store being pending. Two paths still answer with the layout's canonical class while a retag to a user subclass is outstanding: - when a store IS pending under the other spelling, suppressing the fold only declines the constant — the GETFIELD_GC is then emitted ahead of the still unflushed SETFIELD_GC, so the load reads the pre-retag word; - when a store has already been flushed, `put_field_back_to_info` slots it by `field_slot_index(write_descr)`, which for the parentless walker spelling is `Descr::index()`, not the `index_in_parent()` the lookup reads, so the value is invisible and `lazy_set` is empty by then. Upstream has no counterpart to repair: `jtransform.py:1004-1009 handle_getfield_typeptr` deletes the read at codewriter time, so no typeptr getfield reaches the optimizer at all, and `descr.py:236-238` cannot mint the parentless FieldDescr the walker uses here. Remove both arms. Append loop steady body 31 -> 40 ops; re-earning that needs the descr split closed first, not another special case above it. Assisted-by: Claude
Follow-up to #915, which merged before its Codex review could be acted on. The P1 raised there is real, and worse than the report described — the fold it names is live on main right now.
The defect
#915's
f20f9925c0extended thew_classheader fold fromPtrInfo::VirtualtoPtrInfo::Instance, so the class stayed usable after an allocation escaped. Butvirtualize.py:184-195 optimize_GETFIELD_GC_*folds only underopinfo.is_virtual()and otherwise emits, leaving a non-virtual's read toheap.py. That layering is load-bearing for this field:force_boxinstalls the forced instance with an empty field list (info.rs:1113-1118— deliberately, so heap'sdo_setfieldcannot MUST_ALIAS-elide the materializing SETFIELD_GC),SetfieldGc, which lands inCachedField::lazy_set,So the
Instancearm saw an empty slot on every forced instance and answered from the size descr's canonical class. Two consequences: the recorded-value branch was effectively dead code, and the entire 43 → 37 op win came from the unsound fallback. An instance retagged to a user subclass and then forced would read back as the base class — a silent miscompile of a class check, not a crash.The fix
Both resolutions move into
optimize_getfield, after the cache consult: a value already in the instance's own slot wins, and only when the header was never written does the layout's canonical class answer — gated on now_classstore still being pending.The pending check scans every
is_w_class()descr rather than the one being read. The caches are keyed byArc::as_ptr(heap.rs cached_field_pos_for_descr) and pyre mints more than one descr for this header (a walker singleton with no parent, and the size descr's ownall_fielddescrsentry), so a miss on the read descr does not prove the field is untouched — the store may sit in the other spelling'slazy_set.The slot is still looked up through the layout's own
w_classfielddescr, never the read descr's index: a parent-bearing spelling hasindex_in_parent == 0, which is the first value field, and folding through it forwardsRef <- Int.One wrong turn worth recording
The first gate also refused on
!cached_structs.is_empty()and dropped the loop to 40 ops. Plain reads fillcached_structsviaregister_info, so the firstw_classread blocked every later fold. Onlylazy_setdistinguishes a pending write; a store that has already been flushed is readable from the instance slot instead. Gating onlazy_setalone restores 31 ops.Regression coverage
w_subclass2.pydoes not cover this shape — it passed throughout the unsound window. Addedretag_force.py:MyTuple/MyIntinterleaved with their base types, class read both before and after the forcing store.Also here
majit: exit the short-preamble force loop on a non-growing arg list— the loop readsgrown[num_short_jump_args..]immediately after testinggrown.len() == num_short_jump_args, so a list that ever came back shorter would slice out of bounds instead of taking the "did not grow" exit. Raised on #556, which added the surrounding guard.Verification
check.py --backend dynasm,cranelift: dynasm 349/349, cranelift 349/349cargo test --release -p majit-metainterp --features dynasm: 1425 passed, 0 failedw_subclass2.pyandsynth/callee_store_global_read_after_call(240008) unchanged on both backends and underPYRE_NO_JITNot addressed
The parity review's §2 on
unroll.rs(signal_invalid_loopwhere upstream'smapping[box]raisesKeyError) is left as-is. #556 already ported upstream's own answer for an unmapped jump arg in this function —unroll.py:425-434, which defers to the outer replay loop rather than raising — and the same carrier is used 25 lines earlier foroptimizer.flush(). That reads as a documented structural adaptation (§4), not a new divergence.— authored by Claude
Summary by CodeRabbit