Optimize info transfer in replace_op for AbstractInfo handling by lifthrasiir · Pull Request #6 · youknowone/pyre

lifthrasiir · 2026-05-07T12:56:22Z

This pull request refactors and improves the replace_op method in the OptContext struct to better match the logic and invariants of the upstream RPython optimizer. The main changes enhance correctness, prevent cross-type forwarding errors, and ensure proper transfer of metadata when replacing operations.

Key improvements and bug fixes:

Correctness and Invariants:

The method now immediately returns if old is constant, preventing illegal replacements and ensuring only non-constant operations are modified.
Added an explicit check and early return if new is None, ensuring that forwarding is cleared and consistent with RPython's handling.
The type check for cross-type forwarding is now performed after resolving the latest replacement for old, preventing silent type errors and surfacing them at the correct location.

Metadata and Info Transfer:

When replacing an operation, any existing info (such as bounds or pointer info) is now transferred from old to new, unless new is a constant. This mirrors RPython's behavior and ensures that metadata is not lost during optimization passes.

Code structure and clarity:

The logic for resizing the forwarded vector and updating forwarded info is refact

Summary by CodeRabbit

Refactor
- Enhanced internal optimization logic for improved code clarity and control flow handling during operation replacement.

…396 parity) RPython's make_equal_to transfers existing AbstractInfo (IntBound, PtrInfo) from the old box to the new target when forwarding. The pyre replace_op was silently dropping this info by overwriting the slot unconditionally. This could cause the optimizer to lose bounds/nonnull information when values are forwarded through make_equal_to. Also adds get_box_replacement chain walk before operating (optimizer.py:388) and separates the NONE-clear path which has no RPython make_equal_to analog.

coderabbitai · 2026-05-07T12:56:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c27efcfb-c43e-4783-be45-2b3d2d79e7b1

📥 Commits

Reviewing files that changed from the base of the PR and between 64aa654 and e717d60.

📒 Files selected for processing (1)

majit/majit-metainterp/src/optimizeopt/mod.rs

Walkthrough

OptContext::replace_op refactors its control flow to provide explicit handling for constant and NONE replacements, box-replacement resolution, type validation, and stricter forwarding metadata transfer with synchronized box_pool mirroring.

Changes

OptContext::replace_op Forwarding Refactoring

Layer / File(s)	Summary
Core Forwarding Logic Refactoring `majit/majit-metainterp/src/optimizeopt/mod.rs`	replace_op now early-returns on constant old, explicitly clears forwarding when new.is_none(), resolves old through box-replacement before no-op checks, adds debug cross-type forwarding assertions, transfers only Info/IntBound metadata while skipping Const forwarding, and mirrors updates to box_pool.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through forwarding flows,
Where constants stay and new ones go,
Metadata transferred with careful grace,
Box mirrors gleam in their proper place. 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: optimizing info transfer in replace_op for AbstractInfo handling, which aligns with the PR's core objective of transferring metadata during optimization.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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 and usage tips.}

youknowone · 2026-05-10T10:56:56Z

could you please try rebase?

lifthrasiir · 2026-05-10T17:28:20Z

Superseded by 3fe4611 and 851709c.

#13: Fix bridge knowledge lost by setup() — store as pending, apply after setup #12: Add OpRef remapping for bridge knowledge using guard fail_args #14: Set PtrInfo fields when deserializing bridge knowledge #3: Add UNKNOWN_ALIAS force in getfield_from_cache (heap.py:109-111) #4: Add post-force recheck in do_setfield (heap.py:84-101) #5: Skip invalidate for is_always_pure fields (heap.py:189-191) #6: Check postponed_op before emit in force_lazy_set (heap.py:131-135)

…pe __init__ lookup #8 dict arbitrary keys (PyPy dictobject.py parity): - W_DictObject entries: (i64, PyObjectRef) → (PyObjectRef, PyObjectRef) - dict_keys_equal: pointer identity → int equality → str equality - w_dict_lookup/w_dict_store: general PyObjectRef key API - w_dict_getitem_str/w_dict_setitem_str: str key convenience wrappers - All callers updated: build_map, py_getitem, py_setitem, dict methods #6 classmethod binding (PyPy function.py ClassMethod parity): - load_method: classmethod found in instance MRO → bind class (w_type) - load_method: classmethod found on type → bind type (class arg) - Foo.create() now receives cls=Foo as first argument #9 call_type_object __init__ lookup (PyPy typeobject.py parity): - lookup_in_type_mro_pub(w_type, "__init__") instead of py_getattr(w_type) - Avoids ATTR_TABLE shadowing of __init__ on type objects #7 W_TypeObject MRO cache (PyPy typeobject.py parity): - Added mro_w field to W_TypeObject - Computed once at type creation (build_class_inner) - lookup_in_type_mro uses cached MRO when available

history.py:220/261/307 parity — ConstInt / ConstPtr / ConstFloat are distinct Box subclasses, so the same OpRef position must not carry conflicting-typed Values. Previous concern (reviewer #6 blocker): bridge import could seed Value::Ref(0) over a slot already holding Value::Int(0), flipping opref_type Int→Ref and panicking the optimizer's getintbound on subsequent IntLt operands. Add a type-mismatch assertion to seed_constant covering both the const-pool and the resolved-constants paths. Running it across the full dynasm benchmark suite surfaces no violation, so the alias was prophylactic in the current code. Kept as an invariant guard so any future regression fires immediately at the seeding site. Updates the LOAD_FAST_LOAD_FAST / LOAD_FAST_BORROW_LOAD_FAST_BORROW liveness comment with the new evidence and the outstanding spectral_norm regression (separate diagnosis needed); GEN remains disabled pending that trace.

Diagnosis summary for the reviewer-#6 super-instruction enable path: 1. Dynasm baseline (GEN off) for spectral_norm at n=100 already outputs 1.27421999123493 — truncated from the pypy/cranelift correct value 1.2742199912349306. Cranelift matches cpython/pypy with or without the GEN change; this truncation is a dynasm-only precision bug that predates the super-inst GEN discussion. 2. Dynasm baseline also SIGSEGVs at smaller sizes (n=10/11/../15/28.. etc.) in `baseobjspace::getitem` from interpreter-resume dereference of a null local. Pre-existing; not tied to GEN. 3. Enabling GEN for `LoadFastBorrowLoadFastBorrow` (which was the specific super-inst reviewer #6 names) shifts dynasm output to 1.2742199912349301 — closer to correct but still 5 ULP off — and leaves the small-n crashes unchanged. Root cause of the ULP shift: the extra live slots inherited through the super-inst's GEN change the guard-fail layout and the float spills/reloads around it. 4. Enabling GEN for `LoadFastLoadFast` alone leaves the n=100 output unchanged; the regression is specific to the borrow variant because spectral_norm's compiled bytecode uses the borrow form. Reviewer #6 therefore blocks on two separate dynasm-backend fixes (float-precision loss + bridge null-ref re-entry), not on the seed_constant alias that commit 25d0e0e85d already guarded against. Updating the comment in liveness.rs captures the finding so a future pass lands the GEN change together with the dynasm fixes.

…ase 4 + P5.1) model.rs: scaffold SomeValue enum + SomeObjectTrait + primitive Some* variants (SomeInteger/Bool/Float/Char/UnicodeCodePoint/String); port SomeList/Tuple/Dict/Iterator; port SomeInstance/PBC/Builtin/None/ Exception/WeakRef/TypeOf + s_None/s_ImpossibleValue singletons; port union() / unionof / contains 2D pair dispatch including Instance, Exception, PBC, WeakRef pairs; port ClassDef stub + SomeInstance flags dict. Covers rpython/annotator/model.py ~855 LOC. Test port: tests/test_annotator_model.py subset. listdef.rs / dictdef.rs / exception.rs: port rpython/annotator/listdef.py + dictdef.py + exception.py; ListDef/DictDef read_item/generalize/agree land alongside fail-fast r_dict semantics (Phase 5 P5.1). flowspace/operation.rs: fix GetAttr.pure() misclassification + document constfold gaps surfaced by parity review #5+#6.

lifthrasiir closed this May 10, 2026

youknowone mentioned this pull request Jun 14, 2026

box_value() returns None (→ Value::Void) where RPython boxes always carry a payload — value-model decision + skipped sanity checks #181

Open

youknowone mentioned this pull request Jul 4, 2026

JIT: unported opcode (CALL_INTRINSIC_1 etc.) in a hot loop body declines the whole loop → never compiles #373

Closed

This was referenced Jul 13, 2026

jit: drop the WITH_EXCEPT_START gate + with-frame exception-correctness fixes (#389) #532

Merged

jit: PR#607 review follow-ups — abort/bridge decline reclassification, exc-descr-gated seeding, P2 local-dst seed, w_class init #634

Merged

youknowone mentioned this pull request Jul 21, 2026

jit: pc_map read-cutover B–D + vestigial py_pc cleanup; begin backxlat reentry-inverse deletion (jitcode-blackhole Slice 1) #709

Merged

Navigation Menu

Optimize info transfer in replace_op for AbstractInfo handling - #6

Optimize info transfer in replace_op for AbstractInfo handling#6
lifthrasiir wants to merge 1 commit into
youknowone:mainfrom
lifthrasiir:replace-op-info-transfer

lifthrasiir commented May 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 7, 2026 •

edited

Loading

Uh oh!

youknowone commented May 10, 2026

Uh oh!

lifthrasiir commented May 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lifthrasiir commented May 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

youknowone commented May 10, 2026

Uh oh!

lifthrasiir commented May 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lifthrasiir commented May 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 7, 2026 •

edited

Loading