majit: stop resolving a forced instance's w_class header in the optimizer (#920 review) - #922
Conversation
…izer 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughThe optimizer stops folding ChangesOptimizer updates
Estimated code review effort: 3 (Moderate) | ~20 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 4dcad94). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
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: 4dcad9490b
ℹ️ 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".
| // the header write into `OptHeap`, where it sits in | ||
| // `CachedField::lazy_set`. A retag to a user subclass would be | ||
| // discarded and the base class answered instead. Resolving it | ||
| // in `OptHeap` does not close the hole either: reads and writes | ||
| // of this header carry different descr spellings, and the |
There was a problem hiding this comment.
When a virtual with an overridden w_class is forced by a side-effect-free operation before a later class read, its materializing SETFIELD_GC is cached here under the layout descriptor while the subsequent GETFIELD_GC uses the standalone header descriptor. The read therefore misses the pending store, and because GETFIELD_GC is side-effect-free, OptHeap::emitting_operation does not flush lazy sets before emitting it; the generated order is the class load followed by the retag store, so the load observes the allocation's original base class and can make type/class guards silently take the wrong path. Merely removing the fold does not fix this case—the pending cross-spelling store must be forced before the read, or the descriptors must first be unified.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
1 participant
Acts on the two P1s and the CodeRabbit finding on #920, which merged before they could be addressed.
All three land on the same conclusion: the
w_classfold cannot be made sound while reads and writes of that header carry different descr spellings. Removing it is the fix; re-earning the ops needs the split closed first.Why the heap-side version was still wrong
#920 moved the resolution into
optimize_getfield, 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:GETFIELD_GCwhile the materializingSETFIELD_GCis still sitting unflushed inCachedField::lazy_set, so at runtime the load reads the pre-retag word and the store lands after it. Upstream'sUNKNOWN_ALIASpath callsforce_lazy_setbefore returning a miss precisely to prevent this; the cross-spelling case never reaches that call.put_field_back_to_infoslots it byfield_slot_index(write_descr), which for the parentless walker spelling isDescr::index()— not theindex_in_parent()the lookup reads. The value is invisible to the read, andlazy_setis empty by then, so the gate passes and the canonical class wins anyway.Why not patch it again
Upstream has no counterpart to repair.
jtransform.py:1004-1009 handle_getfield_typeptrdeletes this read at codewriter time — no typeptr getfield reaches the optimizer at all — anddescr.py:236-238cannot mint the parentless FieldDescr the walker uses here. Every optimizer-side repair so far has been a special case stacked on a split that upstream does not have, and each one has needed another. AGENTS.md's parity rule points the other way.Cost
Append loop steady body 31 → 40 ops. That is 9, not the 6 the original 43 → 37 measurement implied — the fold was also carrying part of the
_immutable_fields_win. Correctness first; the ops come back when the read and the write name the same descr.Also reverted
majit: exit the short-preamble force loop on a non-growing arg list(==→<=).unroll.py:425uses equality and the list can only grow at that point, so it covered an unreachable shrink at the cost of a divergence in a line-by-line port — #556 left it literal for that reason. Reverted.Verification
check.py --backend dynasm,cranelift: dynasm 349/349, cranelift 349/349retag_force.py(class read before AND after the forcing store) matches thePYRE_NO_JIToracle on both backends:25000 50000 1249975000 tuple MyTuplew_subclass2.pyunchanged:799600000 50000 MyInt intFollow-up
The blocker named here is the same one blocking the
LOAD_ATTRmethod-cache pair: the walker mints ad-hoc field descrs with no parent, so its reads share neither the OptHeap cache key nor thePtrInfoslot with the layout's own fielddescrs, andAbstractStructPtrInfo.produce_short_preamble_ops(info.py:255-271) has no_fieldsto export. Unifying the walker's field-descr minting with the layout's is the prerequisite for both.— authored by Claude
Summary by CodeRabbit
Bug Fixes
Documentation