Optimize JIT for list operations and enhance method dispatch

[lifthrasiir]

This pull request adds fast-path JIT support for list.pop() in Pyre, mirroring the existing optimized handling for list.append(). It introduces a specialized trace-time lowering for lst.pop() that avoids unnecessary boxing and improves performance, especially for integer and float list strategies. Additionally, it lays the groundwork for more robust method-object handling in the JIT and improves the maintainability and adaptability of the codebase for future convergence with upstream RPython.

The most important changes are:

JIT Specialization for list.pop()

• Added generated_list_pop_by_strategy in majit-translate/src/codegen.rs to efficiently trace lst.pop() for object, integer, and float list strategies, using dynamic length reads and avoiding hard-coded constants.
• Added list_pop_value to MIFrame in pyre-jit-trace/src/trace_opcode.rs to select the appropriate fast path or fall back to generic dispatch, paralleling the approach for list.append().

Bound Method Specialization and Descriptors

• Introduced W_METHOD_DESCR_GROUP and corresponding field descriptors (method_w_function_descr, method_w_self_descr) in pyre-jit-trace/src/descr.rs to allow the JIT to recover the receiver and function from bound method objects, supporting robust specialization of method calls. [1] [2]
• Enhanced trace_call_callable in MIFrame to recognize and specialize calls to lst.append() and lst.pop() bound methods by extracting the receiver and guarding on the method function, enabling effective trace-time specialization.

Backend Robustness

• Replaced strict paired-guard checks for forceable calls in the Cranelift backend with check_paired_guard_not_forced, allowing for virtual-forcing ops between the call and its paired guard, improving compatibility with Pyre's optimizer and preventing spurious trace rejections. [1] [2] [3]

Benchmark and Documentation Updates

• Updated several benchmarks and added comments clarifying why certain functions are kept at module scope, noting JIT and specialization issues to be addressed in future work. [1] [2] [3] [4]

Imports and Minor Infrastructure

• Added imports for method object helpers in pyre-jit-trace/src/trace_opcode.rs to support new specialization logic.

Summary by CodeRabbit

• New Features

  • Strategy-specific fast-paths for list.pop() covering object/int/float storage.
  • Trace-time specialization for method-backed list operations (append/pop) for faster calls.

• Improvements

  • Stricter, more informative error messages when expected trace guards/adjacency are missing.
  • New descriptor helpers to improve method-object dispatch reliability.

• Documentation

  • Clarified benchmark notes; one benchmark moved into a main() wrapper to control execution.

[coderabbitai]

Warning

Rate limit exceeded

@youknowone has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 47 minutes and 41 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dcb0e40e-1c12-4b7f-b615-dddc4dd812ce

📥 Commits

Reviewing files that changed from the base of the PR and between b43a166 and 1263849.

📒 Files selected for processing (6)

• majit/majit-backend-cranelift/src/compiler.rs
• majit/majit-translate/src/codegen.rs
• pyre/pyre-jit-trace/src/descr.rs
• pyre/pyre-jit-trace/src/trace_opcode.rs
• pyre/pyre-object/src/float_array.rs
• pyre/pyre-object/src/int_array.rs

Walkthrough

Adds a typed-list pop trace path and method-object specialization in the tracer, a centralized compiler check enforcing a paired GuardNotForced immediately after certain call opcodes, descriptor helpers for method-object fields, a small assembler lookup ordering change, and minor benchmark comments/structural edits. (50 words)

Changes

Compiler Guard Validation

Layer / File(s)	Summary
Helper Extraction / Validation majit/majit-backend-cranelift/src/compiler.rs	Adds private check_paired_guard_not_forced(ops, op_idx, opcode, label) which enforces that ops[op_idx + 1] exists and is GuardNotForced or GuardNotForced2, otherwise returns unsupported_semantics with a message referencing the expected guard at +1 and trace-shape details.
Opcode Lowering Wiring majit/majit-backend-cranelift/src/compiler.rs	Replaces inline adjacency checks in lowering paths for CallMayForce* and CallReleaseGil* with calls to the new helper.
Tests majit/majit-backend-cranelift/...	Updates test_call_may_force_with_intervening_ops to assert the new BackendError message contains expected guard_not_forced(_2) at +1 and mentions SameAsI.

List.pop Trace Feature (Tracer + Codegen + Descriptors)

Layer / File(s)	Summary
Tracer Specialization pyre/pyre-jit-trace/src/trace_opcode.rs	Adds W_MethodObject fast path in call_callable_value for builtin-coded method-backed append/pop, guarded via METHOD_TYPE and pinned w_function; dispatches append to list_append_value and pop to list_pop_value when concrete_len > 0.
Tracer Helper pyre/pyre-jit-trace/src/trace_opcode.rs	Adds MIFrame::list_pop_value which inspects concrete list and length, selects storage strategy (object/int/float), and delegates lowering; falls back to generic trace-call when preconditions fail.
Codegen Generation majit/majit-translate/src/codegen.rs	Adds pub fn generated_list_pop_by_strategy(frame, ctx, list, strategy_id) -> OpRef that guards class/strategy, reads runtime len (opimpl_getfield_gc_i), emits non-empty deopt guard, decrements/sets length (SetfieldGc + setfield_cached), and returns the last element per strategy (clearing object slot to const_ref(0) or boxing primitives).
Descriptors pyre/pyre-jit-trace/src/descr.rs	Adds W_METHOD_DESCR_GROUP and public helpers method_w_function_descr() and method_w_self_descr() to access method-object w_function/w_self field descriptors used by the tracer.
Fallbacks & Imports pyre/pyre-jit-trace/src/trace_opcode.rs	Adds pyre_object::methodobject imports and preserves fallback to trace_call_callable when storage strategy, length, or other guards prevent specialization.
Tests / Integration (implicit)	No new explicit tests added in this diff; new paths are intended to be covered by existing tracing/integration tests.

Assembler OpRef Resolution Change

Layer / File(s)	Summary
Lookup Order & Raw() Use majit/majit-backend-dynasm/src/x86/assembler.rs	opref_type_at now uses opref.raw() for lookups and changes non-constant resolution order to check inputarg_index via raw() before falling back to op_index via raw(); constant handling also uses raw() into constant_types.

Benchmarks / Docs

Layer / File(s)	Summary
Comments / Documentation pyre/bench/list_insert.py, pyre/bench/list_reverse.py, pyre/bench/list_setslice.py	Adds module-level notes explaining why benchmarks are kept at module scope and describing known quirks/bugs when wrapped in main().
Structure Change pyre/bench/list_pop_append.py	Moves benchmark body into a new def main(): and invokes it at module level so the script executes the benchmark when run.

Sequence Diagram

sequenceDiagram
    participant Tracer as Tracer\n(trace_opcode)
    participant Codegen as Codegen\n(codegen)
    participant Compiler as Compiler\n(cranelift)

    Tracer->>Tracer: Detect method-backed list.pop call
    Tracer->>Tracer: Guard method object (METHOD_TYPE) and pin w_function
    Tracer->>Tracer: Recover receiver and compute concrete_len

    alt concrete_len > 0
        Tracer->>Tracer: Determine storage strategy (obj/int/float)
        Tracer->>Codegen: generated_list_pop_by_strategy(frame, ctx, list, strategy)
        Codegen->>Codegen: Read runtime len via opimpl_getfield_gc_i
        Codegen->>Codegen: Emit guard len > 0, compute last_index = len - 1
        Codegen->>Codegen: Update length field (SetfieldGc + setfield_cached)
        Codegen->>Codegen: Load last element, clear slot (object) or read & box (int/float)
        Codegen-->>Tracer: Return popped OpRef
    else zero/unknown
        Tracer->>Tracer: Fallback to generic trace_call_callable
    end

    Tracer->>Compiler: Lower CallMayForce/CallReleaseGil
    Compiler->>Compiler: check_paired_guard_not_forced ensures GuardNotForced at +1
    Compiler-->>Tracer: Emit lowered code or unsupported_semantics error

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hop through traces with a curious stare,
I pop the last item, box numbers with care,
A strict guard at +1 keeps surprises away,
Descriptors and codegen stitch night into day,
The JIT hums softly — the rabbit is gay.

🚥 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 'Optimize JIT for list operations and enhance method dispatch' directly summarizes the main changes: JIT optimization for list.pop() and list.append(), improved method dispatch via bound-method specialization, and backend improvements.
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

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@majit/majit-translate/src/codegen.rs`:
- Around line 1570-1577: Compute last_index only after guarding that the list is
non-empty: before calling ctx.record_op(OpCode::IntSub, &[len, one]) for
last_index/new_len, insert a non-empty check on len (use ctx.record_op with
OpCode::IntEq or OpCode::IntLE as appropriate) and emit a conditional
guard/branch that jumps to the fallback path when len == 0; only when the guard
passes (len > 0) compute last_index = ctx.record_op(OpCode::IntSub, &[len, one])
and set new_len = last_index. Reference symbols: opimpl_getfield_gc_i, len, one,
last_index, new_len, and ctx.record_op to locate where to add the check and
guard.
- Around line 1580-1585: The pop implementation leaves the removed reference in
storage, which can keep objects alive; after retrieving the popped value via
crate::state::trace_items_block_getitem_value(ctx, items_block, last_index) you
should overwrite that slot with a nil/none sentinel so the GC no longer sees a
live reference. Concretely, after obtaining item, record a SetfieldGc (or
equivalent write) to the items block at last_index to store the nil value and
update any heap cache entry for that slot (similar to the existing
ctx.record_op_with_descr(OpCode::SetfieldGc, &[list, new_len], len_descr) and
ctx.heap_cache_mut().setfield_cached calls) so both the emitted op and
heap_cache reflect the cleared slot.

In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 3981-4010: The fallback incorrectly calls
trace_call_callable(list, &[]) which traces invoking the receiver as a callable
(list()), not the bound pop method; in list_pop_value replace those fallback
calls so they invoke the bound pop callable instead of the receiver — obtain or
construct the OpRef that represents the bound method (the same callable you
would pass into generated_list_pop_by_strategy when using strategies) and pass
that OpRef into trace_call_callable with the empty args; update both
early-return and final-return paths in list_pop_value to use that bound-pop
OpRef (use existing helpers for attribute/method lookup if available, e.g., the
code path that produces the callable used by generated_list_pop_by_strategy).

🪄 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

Run ID: 94444d26-ce96-4d0e-a367-1924581efe35

📥 Commits

Reviewing files that changed from the base of the PR and between 95a8cda and 246edd9.

📒 Files selected for processing (8)

• majit/majit-backend-cranelift/src/compiler.rs
• majit/majit-translate/src/codegen.rs
• pyre/bench/list_insert.py
• pyre/bench/list_pop_append.py
• pyre/bench/list_reverse.py
• pyre/bench/list_setslice.py
• pyre/pyre-jit-trace/src/descr.rs
• pyre/pyre-jit-trace/src/trace_opcode.rs

[ever0de]

CI test asserts stale error contract after check_paired_guard_not_forced was relaxed

Commit 9c234d0788 relaxed the adjacency check (forward-scan instead of strict +1), but test_call_may_force_with_intervening_ops still called .unwrap_err() on the compile result — a compile-time panic since the backend now returns Ok. The test comment already said "This proves the codegen accepts non-adjacent placement" but the assertion contradicted it. Fixed by replacing the .unwrap_err() + error-message check with .unwrap() + execution assertions (non-forced path returns 42, forced path fires GuardNotForced with correct fail_args and savedata).

in majit/majit-backend-cranelift/src/compiler.rs

@@ fn test_call_may_force_with_intervening_ops() {
        let _guard = may_force_test_lock()
            .lock()
            .unwrap_or_else(|err| err.into_inner());
-        // RPython backend parity requires GUARD_NOT_FORCED to be the
-        // immediate next operation after CALL_MAY_FORCE.
+        // RPython backend parity: GUARD_NOT_FORCED may have intervening ops
+        // between CALL_MAY_FORCE and itself; the codegen must accept non-adjacent placement.
+        // (Relaxed in check_paired_guard_not_forced forward-scan.)

@@
-        let err = backend
-            .compile_loop(&inputargs, &ops, &mut token)
-            .unwrap_err();
-        match err {
-            BackendError::Unsupported(msg) => {
-                assert!(msg.contains("CallMayForceI"));
-                assert!(msg.contains("ops[position+1] must be guard_not_forced(_2)"));
-            }
-            other => panic!("expected unsupported error, got {other:?}"),
-        }
+        // Non-adjacent GuardNotForced placement is now accepted.
+        backend.compile_loop(&inputargs, &ops, &mut token).unwrap();
+
+        // Not forced: function returns 42; SameAsI propagates it to Finish.
+        let frame = backend.execute_token(&token, &[Value::Int(20), Value::Int(0)]);
+        assert!(backend.get_latest_descr(&frame).is_finish());
+        assert_eq!(backend.get_int_value(&frame, 0), 42);
+
+        // Forced: GuardNotForced fires; fail_args = [flag=1, call_result=42, inputarg0=10].
+        let frame = backend.execute_token(&token, &[Value::Int(10), Value::Int(1)]);
+        assert_eq!(backend.get_latest_descr(&frame).fail_index(), 0);
+        assert_eq!(backend.get_int_value(&frame, 0), 1);
+        assert_eq!(backend.get_int_value(&frame, 1), 42);
+        assert_eq!(backend.get_int_value(&frame, 2), 10);
+        assert_eq!(backend.get_savedata_ref(&frame).unwrap(), GcRef(0xBABA));
}

[ever0de]

list_pop_value fallback traces list() instead of calling the bound method

The original fallback path called trace_call_callable(list, &[]) where list is the receiver OpRef (the list object), not the W_MethodObject. At runtime this would trace a call to list as a constructor (list() → TypeError). Fixed by adding callable: OpRef as a parameter and using it in all fallback sites.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@majit/majit-translate/src/codegen.rs`:
- Around line 1617-1624: The module path for boxing is inconsistent: replace the
direct call to crate::trace_box_float with crate::generated::trace_box_float to
match crate::generated::trace_box_int, or better yet switch both boxing calls to
use the established state helpers crate::state::wrapint and
crate::state::wrapfloat (used elsewhere) so boxing is consistent across the
file; update the calls at the locations invoking trace_box_int/trace_box_float
(and the similar block at 1637-1644) to use either crate::generated::trace_box_*
consistently or to call crate::state::wrapint/crate::state::wrapfloat instead.

In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 4182-4235: The current fast-path only checks func_name
("append"/"pop") which can match other builtin-coded methods on list subclasses;
instead guard that inner_func is the exact built-in list method by comparing its
code/function object to the canonical list append/pop implementation before
rewiring. Locate inner_func (and where func_name is computed using
pyre_interpreter::function_get_name) and replace or augment the func_name check
with an equality check between pyre_interpreter::getcode(inner_func) (or
inner_func pointer) and the canonical list method code/function object
(obtainable once and stored as a constant or fetched via the runtime for the
list builtin), and only call list_append_value or list_pop_value when that
equality succeeds (leave other cases to trace_call_callable).

🪄 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

Run ID: b33e5d6e-7af3-4393-9655-60e805d409db

📥 Commits

Reviewing files that changed from the base of the PR and between 246edd9 and 7a5a077.

📒 Files selected for processing (4)

• majit/majit-backend-cranelift/src/compiler.rs
• majit/majit-translate/src/codegen.rs
• pyre/pyre-jit-trace/src/descr.rs
• pyre/pyre-jit-trace/src/trace_opcode.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistent module paths for boxing functions.

The int strategy uses crate::generated::trace_box_int while the float strategy uses crate::trace_box_float (missing the generated:: prefix). Since both functions are defined at the same level in the generated code, the paths must be consistent.

Additionally, the rest of this file uses crate::state::wrapint / crate::state::wrapfloat for boxing (e.g., lines 924, 1046, 1743, 1949, 1968). Consider using the same pattern here for consistency.

🔧 Proposed fix using wrapint/wrapfloat pattern

         1 => {
             let items_ptr = crate::state::opimpl_getfield_gc_i(
                 ctx,
                 list,
                 crate::descr::list_int_items_ptr_descr(),
             );
             let raw =
                 crate::state::trace_raw_int_array_getitem_value(ctx, items_ptr, last_index);
             ctx.record_op_with_descr(OpCode::SetfieldGc, &[list, new_len], len_descr);
             ctx.heap_cache_mut().setfield_cached(list, len_descr_idx, new_len);
-            let int_type_addr = &pyre_object::pyobject::INT_TYPE as *const _ as i64;
-            crate::generated::trace_box_int(
-                ctx,
-                raw,
-                crate::descr::w_int_size_descr(),
-                crate::descr::ob_type_descr(),
-                crate::descr::int_intval_descr(),
-                int_type_addr,
-            )
+            crate::state::wrapint(ctx, raw)
         }
         2 => {
             let items_ptr = crate::state::opimpl_getfield_gc_i(
                 ctx,
                 list,
                 crate::descr::list_float_items_ptr_descr(),
             );
             let raw =
                 crate::state::trace_raw_float_array_getitem_value(ctx, items_ptr, last_index);
             ctx.record_op_with_descr(OpCode::SetfieldGc, &[list, new_len], len_descr);
             ctx.heap_cache_mut().setfield_cached(list, len_descr_idx, new_len);
-            let float_type_addr = &pyre_object::pyobject::FLOAT_TYPE as *const _ as i64;
-            crate::trace_box_float(
-                ctx,
-                raw,
-                crate::descr::w_float_size_descr(),
-                crate::descr::ob_type_descr(),
-                crate::descr::float_floatval_descr(),
-                float_type_addr,
-            )
+            crate::state::wrapfloat(ctx, raw)
         }

Also applies to: 1637-1644

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@majit/majit-translate/src/codegen.rs` around lines 1617 - 1624, The module
path for boxing is inconsistent: replace the direct call to
crate::trace_box_float with crate::generated::trace_box_float to match
crate::generated::trace_box_int, or better yet switch both boxing calls to use
the established state helpers crate::state::wrapint and crate::state::wrapfloat
(used elsewhere) so boxing is consistent across the file; update the calls at
the locations invoking trace_box_int/trace_box_float (and the similar block at
1637-1644) to use either crate::generated::trace_box_* consistently or to call
crate::state::wrapint/crate::state::wrapfloat instead.

[youknowone]

Sorry, I accidently closed this pr. not intended