jit, interp: inline bound methods and defaults, specialize builtin calls, and fold the per-opcode polls into the eval breaker word - #878
Conversation
WalkthroughThis PR updates interpreter safepoints, builtin dispatch, JIT specialization, indirect-call target generation, MIR lowering, frame reconstruction, and related build/runtime metadata across Pyre and Majit. ChangesRuntime and JIT coordination
Translation and dispatch
Inlining and frame state
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
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 |
![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
https://github.com/youknowone/pyre/blob/700457b710bcbf5b7f0dd90f0128920b441947d4/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L5244-L5245
When the recorded argument is True or False, the admission check accepts it because pyre_object::is_int includes BOOL_TYPE, but this unboxes it behind an INT_TYPE class guard and then guards its w_class as canonical int. A bool cannot satisfy those guards, so a hot loop containing valid calls such as math.isqrt(True) produces an invalid or always-deopting trace instead of compiling. Use int_or_bool_unbox_type_descr for this path or explicitly decline bool arguments.
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 42ab57c). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/216b2fa8d89356b908b231af33ccd4bf96367c93/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L4696-L4699
In the free-threaded runtime, another thread can insert or remove a key after the separate keys_version guard succeeds but before jit_dict_nth_value acquires the dictionary lock. Because these mutations can shift the IndexMap entry at index, the helper may return an unrelated key's value while the compiled dict.get operation should return the requested key's value or its default. The version check and indexed read need to occur atomically under the same dict lock, with a guard/side exit when the version no longer matches.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6931323ac
ℹ️ 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".
| wrapper_items.extend_from_slice(&r_args[2..]); | ||
| wrapper_item_concretes.extend_from_slice(&arg_concretes[2..]); |
There was a problem hiding this comment.
For a hot keyword call to a generated builtin method, r_args[2] is the kwnames tuple and the remaining entries are the unbound positional/keyword values, but this copies all of them directly into the wrapper's positional argument array. For example, a method call such as rng.seed(w_n=5) enters the wrapper with [self, kwnames, 5] instead of [self, 5] (or the gateway's marker-dict representation), so the wrapper records an arity error for a call that the interpreter accepts. Reorder/bind CallKw operands before descending into the wrapper, or limit this specialization to CallFn.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
| // The ordinary caller is `eval_with_jit_inner`, immediately after its | ||
| // one authoritative `unsupported_jit_shape` check. The other caller, | ||
| // `portal_runner_dispatch`, is recursive re-entry for a portal that could | ||
| // only have obtained compiled code after passing that same check. Keep |
There was a problem hiding this comment.
When portal_runner_dispatch re-enters the portal for a callee reached from compiled/blackhole code, that callee has not necessarily passed eval_with_jit_inner's earlier shape check; this function can tick its entry counter and start tracing it even when it is CurrentFrameOnly, has a nested-break bridge-resume shape, or exceeds the constant encoding. Those are precisely the shapes unsupported_jit_shape prevents from being assembled or resumed, so such a recursive call can now enter an unsupported trace instead of remaining interpreted. Keep a cached shape check on this entry path rather than assuming every recursive frame was previously admitted.
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyre/pyre-macros/src/lib.rs (1)
1962-1974: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute the variable-arity “too few” branch through a residual arity-helper.
Lines 1962-1973 still build the error with
format!(...)inside every generated wrapper body, while the fixed-arity/no-arg branches use#[dont_look_inside]$crate::gateway::method_arity_failure/method_noarg_failurespecifically to keep formatting out of traced code. Add a cold helper for the “expected at least N arguments” message and route this branch through it so no rawformat!remains in the wrapper.🤖 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 `@pyre/pyre-macros/src/lib.rs` around lines 1962 - 1974, Add a cold residual helper for constructing the variable-arity “expected at least N arguments” error, then update the visible variable-arity too-few branch in the generated wrapper to call that helper instead of formatting inline. Match the existing method_arity_failure and method_noarg_failure pattern, passing fn_name, visible_required, and the adjusted argument count while preserving pluralization and the current error message.Source: Coding guidelines
majit/majit-translate/src/codewriter/call.rs (2)
3267-3277: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
builtin_wrapper_indirect_graphs()recomputation inside the BFS op loop.
builtin_wrapper_indirect_graphs()is already computed once at the top offind_all_graphs_bfs(line 3126); nothing mutatesfunction_fnaddrs/function_graphsmid-walk, so recomputing the full HashMap scan + per-address sort here for every matchingIndirectCallop is wasted work. See the consolidated comment for the cross-file fix.🤖 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-translate/src/codewriter/call.rs` around lines 3267 - 3277, In find_all_graphs_bfs, reuse the builtin wrapper indirect-graphs map computed at the function start instead of calling builtin_wrapper_indirect_graphs() inside the IndirectCall branch for empty graphs. Keep cloning explicitly attached graphs and skipping None unchanged, while preserving the existing empty-graphs behavior through the cached result.
1-1: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
CallControl::builtin_wrapper_indirect_graphs()is recomputed redundantly across several call sites. It performs a fullfunction_fnaddrsHashMap scan plus a per-address sort, and its result is stable once wrapper registration completes — yet it's invoked repeatedly with no caching.
majit/majit-translate/src/codewriter/call.rs#L3267-3277: hoist the call outside the BFS's per-op loop — it's already computed once at line 3126 with nothing mutatingfunction_fnaddrs/function_graphsin between, so this inner-loop call is pure redundant work.majit/majit-translate/src/codewriter/call.rs#L4538-4566: consider adding an internal cache (e.g. aRefCell<Option<Vec<CallPath>>>onCallControl, populated on first call) so every external caller benefits without each one needing to hoist manually.majit/majit-translate/src/translator/rtyper/rpbc.rs#L326-345:lower_indirect_callstakes a single graph, suggesting the enclosing rtype driver invokes it once per candidate graph; verify that call frequency and, if it's per-graph, route through the same cache rather than recomputing per graph across the whole program.🤖 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-translate/src/codewriter/call.rs` at line 1, Cache the result of CallControl::builtin_wrapper_indirect_graphs() inside CallControl so repeated callers reuse one computed Vec<CallPath> after wrapper registration completes. Update the method and its callers, including the BFS per-op path and translator/rtyper lower_indirect_calls flow, to use the cache while preserving the existing graph results.
🤖 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-translate/src/codewriter/call.rs`:
- Around line 4538-4566: Make alias selection in builtin_wrapper_indirect_graphs
deterministic by adding a path-content tie-breaker after descending segment
count, such as lexicographic ordering of the path segments. Keep grouping by
function address and selecting the most-qualified alias unchanged, while
ensuring equal-length aliases always produce the same representative regardless
of function_fnaddrs iteration order.
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 927-933: Update the impl-method branch in the graph identity
construction to set source_identity from owner and name only, using the
“{owner}::{name}” form without module_path. Leave the non-impl branch’s
fn_path-based identity unchanged.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1729-1738: Update the eligibility guard in the inline-call
dispatch path to accept only PyreHelperKind::CallFn, excluding CallKw before
residual operands are passed to the builtin wrapper. Keep the existing
argument-count, destination-bank, and authoritative-executor checks unchanged.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 2896-2929: Extract the shared BINARY_OP tag decoding from the
neighboring helpers into a single helper such as body_binop_tag, and centralize
the And/Or/Xor/InplaceAnd/InplaceOr/InplaceXor check in one is_int_only_bitwise
predicate. Update both callers to reuse these symbols, and rename
residual_call_is_specialized_plain_int_binop to reflect that it only checks the
decoded operator, without claiming validation of d.key or the BinaryOp helper
kind.
---
Outside diff comments:
In `@majit/majit-translate/src/codewriter/call.rs`:
- Around line 3267-3277: In find_all_graphs_bfs, reuse the builtin wrapper
indirect-graphs map computed at the function start instead of calling
builtin_wrapper_indirect_graphs() inside the IndirectCall branch for empty
graphs. Keep cloning explicitly attached graphs and skipping None unchanged,
while preserving the existing empty-graphs behavior through the cached result.
- Line 1: Cache the result of CallControl::builtin_wrapper_indirect_graphs()
inside CallControl so repeated callers reuse one computed Vec<CallPath> after
wrapper registration completes. Update the method and its callers, including the
BFS per-op path and translator/rtyper lower_indirect_calls flow, to use the
cache while preserving the existing graph results.
In `@pyre/pyre-macros/src/lib.rs`:
- Around line 1962-1974: Add a cold residual helper for constructing the
variable-arity “expected at least N arguments” error, then update the visible
variable-arity too-few branch in the generated wrapper to call that helper
instead of formatting inline. Match the existing method_arity_failure and
method_noarg_failure pattern, passing fn_name, visible_required, and the
adjusted argument count while preserving pluralization and the current error
message.
🪄 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: 258d92aa-edc6-4519-9945-9ebcb720cd4e
📒 Files selected for processing (35)
Cargo.tomlmajit/majit-gc/src/collector.rsmajit/majit-ir/src/eval_breaker_word.rsmajit/majit-translate/src/codegen.rsmajit/majit-translate/src/codewriter/call.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/pipeline.rsmajit/majit-translate/src/translator/rtyper/rpbc.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/module/math/interp_math.rspyre/pyre-interpreter/src/module/math/mod.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/call.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/gc_interp.rspyre/pyre-object/src/gc_roots.rs
| /// Candidate PBC family for the generated `BuiltinCode.func` | ||
| /// function-pointer field. | ||
| /// | ||
| /// RPython obtains this list from the annotator's `SomePBC` | ||
| /// descriptions. Pyre's generated wrappers publish real fnaddrs through | ||
| /// `jit_trace_fnaddrs`; pair those addresses with their registered source | ||
| /// graphs here. Aliases sharing an address name the same wrapper; select | ||
| /// the most-qualified source identity for the one graph object entered | ||
| /// into the PBC family. | ||
| pub fn builtin_wrapper_indirect_graphs(&self) -> Vec<CallPath> { | ||
| let mut by_address: std::collections::BTreeMap<i64, Vec<CallPath>> = | ||
| std::collections::BTreeMap::new(); | ||
| for (path, &fnaddr) in &self.function_fnaddrs { | ||
| let Some(leaf) = path.last_segment() else { | ||
| continue; | ||
| }; | ||
| if !leaf.starts_with("__pyre_wrap_") || !self.function_graphs.contains_key(path) { | ||
| continue; | ||
| } | ||
| by_address.entry(fnaddr).or_default().push(path.clone()); | ||
| } | ||
| let mut result = Vec::new(); | ||
| for mut aliases in by_address.into_values() { | ||
| aliases.sort_by_key(|path| std::cmp::Reverse(path.segments.len())); | ||
| result.push(aliases.remove(0)); | ||
| } | ||
| result | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-deterministic tie-break when multiple wrapper aliases share the same address and segment count.
aliases is built by iterating self.function_fnaddrs (std::collections::HashMap), whose iteration order varies across process runs. sort_by_key(|path| Reverse(path.segments.len())) is stable, so when two aliases of the same address tie on segment length, the chosen "most-qualified" representative depends on HashMap iteration order rather than the path content — a reproducibility gap for something the doc comment describes as a deterministic "most-qualified" selection.
🔧 Proposed fix: deterministic secondary sort key
- aliases.sort_by_key(|path| std::cmp::Reverse(path.segments.len()));
+ aliases.sort_by(|a, b| {
+ b.segments.len().cmp(&a.segments.len()).then_with(|| a.segments.cmp(&b.segments))
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Candidate PBC family for the generated `BuiltinCode.func` | |
| /// function-pointer field. | |
| /// | |
| /// RPython obtains this list from the annotator's `SomePBC` | |
| /// descriptions. Pyre's generated wrappers publish real fnaddrs through | |
| /// `jit_trace_fnaddrs`; pair those addresses with their registered source | |
| /// graphs here. Aliases sharing an address name the same wrapper; select | |
| /// the most-qualified source identity for the one graph object entered | |
| /// into the PBC family. | |
| pub fn builtin_wrapper_indirect_graphs(&self) -> Vec<CallPath> { | |
| let mut by_address: std::collections::BTreeMap<i64, Vec<CallPath>> = | |
| std::collections::BTreeMap::new(); | |
| for (path, &fnaddr) in &self.function_fnaddrs { | |
| let Some(leaf) = path.last_segment() else { | |
| continue; | |
| }; | |
| if !leaf.starts_with("__pyre_wrap_") || !self.function_graphs.contains_key(path) { | |
| continue; | |
| } | |
| by_address.entry(fnaddr).or_default().push(path.clone()); | |
| } | |
| let mut result = Vec::new(); | |
| for mut aliases in by_address.into_values() { | |
| aliases.sort_by_key(|path| std::cmp::Reverse(path.segments.len())); | |
| result.push(aliases.remove(0)); | |
| } | |
| result | |
| } | |
| /// Candidate PBC family for the generated `BuiltinCode.func` | |
| /// function-pointer field. | |
| /// | |
| /// RPython obtains this list from the annotator's `SomePBC` | |
| /// descriptions. Pyre's generated wrappers publish real fnaddrs through | |
| /// `jit_trace_fnaddrs`; pair those addresses with their registered source | |
| /// graphs here. Aliases sharing an address name the same wrapper; select | |
| /// the most-qualified source identity for the one graph object entered | |
| /// into the PBC family. | |
| pub fn builtin_wrapper_indirect_graphs(&self) -> Vec<CallPath> { | |
| let mut by_address: std::collections::BTreeMap<i64, Vec<CallPath>> = | |
| std::collections::BTreeMap::new(); | |
| for (path, &fnaddr) in &self.function_fnaddrs { | |
| let Some(leaf) = path.last_segment() else { | |
| continue; | |
| }; | |
| if !leaf.starts_with("__pyre_wrap_") || !self.function_graphs.contains_key(path) { | |
| continue; | |
| } | |
| by_address.entry(fnaddr).or_default().push(path.clone()); | |
| } | |
| let mut result = Vec::new(); | |
| for mut aliases in by_address.into_values() { | |
| aliases.sort_by(|a, b| { | |
| b.segments.len().cmp(&a.segments.len()).then_with(|| a.segments.cmp(&b.segments)) | |
| }); | |
| result.push(aliases.remove(0)); | |
| } | |
| result | |
| } |
🤖 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-translate/src/codewriter/call.rs` around lines 4538 - 4566, Make
alias selection in builtin_wrapper_indirect_graphs deterministic by adding a
path-content tie-breaker after descending segment count, such as lexicographic
ordering of the path segments. Keep grouping by function address and selecting
the most-qualified alias unchanged, while ensuring equal-length aliases always
produce the same representative regardless of function_fnaddrs iteration order.
| let graph = if let Some(owner) = &self_ty_root { | ||
| graph | ||
| .with_owner_root(owner.clone()) | ||
| .with_source_identity(format!("{module_path}::{owner}::{name}")) | ||
| } else { | ||
| graph.with_source_identity(fn_path.clone()) | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect impl_method_owner_for_fundecl's return shape and a sample name_path()
# rendering for an impl method, to confirm the module_path/owner overlap.
ast-grep run --pattern 'fn impl_method_owner_for_fundecl($$$) { $$$ }' --lang rust majit/majit-translate/src/front/mir.rs
rg -n -A5 'fn impl_method_owner_for_fundecl' majit/majit-translate/src/front/mir.rsRepository: youknowone/pyre
Length of output: 454
🏁 Script executed:
#!/bin/bash
set -e
# Map and inspect the relevant source_identity construction, fn_path derivation,
# impl_method_owner_for_fundecl implementation, and nearby call_target_segments behavior.
wc -l majit/majit-translate/src/front/mir.rs
sed -n '860,945p' majit/majit-translate/src/front/mir.rs
printf '\n--- impl_method_owner_for_fundecl ---\n'
sed -n '12500,12575p' majit/majit-translate/src/front/mir.rs
printf '\n--- nearby references to impl_method_owner_for_fundecl ---\n'
rg -n 'impl_method_owner_for_fundecl|with_source_identity|source_identity|owner_root' majit/majit-translate/src/front/mir.rsRepository: youknowone/pyre
Length of output: 13115
🏁 Script executed:
#!/bin/bash
set -e
# Inspect strip_crate_prefix and search the repository for sample impl-method name/path spellings.
rg -n 'fn strip_crate_prefix|strip_crate_prefix\(' majit/majit-translate/src/front/mir.rs majit -g '*/mir.rs' -g '!**/target/**'
printf '\n--- sample Charon/type path references around Self and impl methods ---\n'
rg -n -A2 -B2 'pyframe::<PyFrame>|pyre_interpreter::pyframe::<' majit -S
printf '\n--- call_target_segments implementation around impl_method_owner_for_fundecl users ---\n'
sed -n '8245,8275p' majit/majit-translate/src/front/mir.rs
echo '--- all format!("{module_path}::{owner}::{name}" occurrences in tracked files ---'
rg -n 'format!("{module_path}::{owner}::{name}"|with_source_identity\(' majit/majit-translate/src/front/mir.rsRepository: youknowone/pyre
Length of output: 4977
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- strip_crate_prefix ---'
sed -n '15245,15275p' majit/majit-translate/src/front/mir.rs
printf '%s\n' '--- module/name derivation and source identity ---'
sed -n '865,932p' majit/majit-translate/src/front/mir.rs
printf '%s\n' '--- with_source_identity occurrences ---'
rg -n 'with_source_identity\(' majit/majit-translate/src/front/mir.rs
printf '%s\n' '--- exact module_path owner name source_identity occurrences ---'
rg -n --fixed-strings 'format!("{module_path}::{owner}::{name}"' majit/majit-translate/src/front/mir.rs
printf '%s\n' '--- behavioral simulation from source text ---'
python3 - <<'PY'
sample = "pyre_interpreter::pyframe::pyframe::<Impl>::pyframe::<Impl>::pop"
stripped = sample.removeprefix("pyre_").removeprefix("majit_").removeprefix("pyre_")
print(stripped)
parts = stripped.rsplit("::", 1)
module_path, name = (parts[0], parts[1]) if len(parts) == 2 else ("", stripped)
print("module_path=", module_path)
print("name=", name)
# If Charon name_path includes an Impl segment before the method, the owner qualified path will likewise include it,
# because it resolves the impl payload to the ADT's name_path, not to the function's rsplit parent.
owner = module_path
print("current=", f"{module_path}::{owner}::{name}")
print("suggested=", f"{owner}::{name}")
PYRepository: youknowone/pyre
Length of output: 5814
Don’t prepend module_path for impl-method owner identities.
For impl methods, stripping the crate prefix from Charon paths can be a no-op when the path does not start with the current crate name, so module_path may still already contain the implementation module and owner before owner is resolved. Baking it again as "{module_path}::{owner}::{name}" produces a malformed source_identity; use "{owner}::{name}" for the impl-method branch.
🤖 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-translate/src/front/mir.rs` around lines 927 - 933, Update the
impl-method branch in the graph identity construction to set source_identity
from owner and name only, using the “{owner}::{name}” form without module_path.
Leave the non-impl branch’s fn_path-based identity unchanged.
| pub(crate) fn residual_call_is_specialized_plain_int_binop( | ||
| body_code: &[u8], | ||
| d: &DecodedOp, | ||
| num_regs_i: usize, | ||
| constants_i: &[i64], | ||
| ) -> bool { | ||
| let Some(&i_len) = body_code.get(d.pc + 2) else { | ||
| return false; | ||
| }; | ||
| if i_len == 0 { | ||
| return false; | ||
| } | ||
| let Some(&tag_reg) = body_code.get(d.pc + 3) else { | ||
| return false; | ||
| }; | ||
| let Some(&tag) = (tag_reg as usize) | ||
| .checked_sub(num_regs_i) | ||
| .and_then(|constant_index| constants_i.get(constant_index)) | ||
| else { | ||
| return false; | ||
| }; | ||
| use pyre_interpreter::bytecode::BinaryOperator; | ||
| matches!( | ||
| pyre_interpreter::runtime_ops::binary_op_from_tag(tag), | ||
| Some( | ||
| BinaryOperator::And | ||
| | BinaryOperator::Or | ||
| | BinaryOperator::Xor | ||
| | BinaryOperator::InplaceAnd | ||
| | BinaryOperator::InplaceOr | ||
| | BinaryOperator::InplaceXor | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the shared BINARY_OP tag decode + int-only operator set.
Lines 2902-2928 duplicate the constants-window tag decode and the And/Or/Xor/Inplace* set already present at Lines 2862-2891. The two copies must stay in lockstep — adding a bitwise operator to one and not the other silently mis-gates replay safety. Also note this helper, unlike its neighbour, validates neither d.key nor the BinaryOp helper kind, so its name over-promises relative to what it checks.
♻️ Suggested shape
fn body_binop_tag(
body_code: &[u8],
d: &DecodedOp,
num_regs_i: usize,
constants_i: &[i64],
) -> Option<pyre_interpreter::bytecode::BinaryOperator> {
let &i_len = body_code.get(d.pc + 2)?;
if i_len == 0 {
return None;
}
let &tag_reg = body_code.get(d.pc + 3)?;
let &tag = (tag_reg as usize)
.checked_sub(num_regs_i)
.and_then(|i| constants_i.get(i))?;
pyre_interpreter::runtime_ops::binary_op_from_tag(tag)
}
fn is_int_only_bitwise(op: pyre_interpreter::bytecode::BinaryOperator) -> bool { /* one set */ }🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs` around lines 2896
- 2929, Extract the shared BINARY_OP tag decoding from the neighboring helpers
into a single helper such as body_binop_tag, and centralize the
And/Or/Xor/InplaceAnd/InplaceOr/InplaceXor check in one is_int_only_bitwise
predicate. Update both callers to reuse these symbols, and rename
residual_call_is_specialized_plain_int_binop to reflect that it only checks the
decoded operator, without claiming validation of d.key or the BinaryOp helper
kind.
`cargo run --release` gets the same cross-crate optimization the `dist` profile already inherits. Assisted-by: Claude
`all_thread_hooks_current` compares the execution context's `trace_all_generation` / `profile_all_generation` against the process counters, and `bytecode_trace` calls the updater only when they differ. `bytecode_trace` and `bytecode_only_trace` become `#[inline(always)]`. Assisted-by: Claude
baseobjspace.py:1254-1259 unwraps `_Method` before the Function valuestack path. The CALL handler now writes the receiver into the `null_or_self` stack slot, continues with `w_function`, and passes `methodcall` to `funccall_valuestack` with the receiver counted as one extra argument (callmethod.py:85-94) while the popped width stays the physical `[callable, null_or_self, args...]`. Assisted-by: Claude
jd1 shares the `MetaInterp.tracing` slot with the bytecode portal, so while a residual `next()` runs a generator body the jd1 trace holds only the opaque call and every jd0 merge point that body reaches is suppressed. Restore the opt-in gate; the master JIT off-switches still apply. Assisted-by: Claude
The flag is process-stable after its first env read, so hoist it out of the per-bytecode safepoint poll instead of crossing the `dont_look_inside` gate and reloading its atomic on every iteration. Assisted-by: Claude
`CallControl.graph_jit_shapes` holds the `UnsupportedJitShape` discriminant keyed by code-object address, alongside the graph-keyed `jitcodes`. `eval_with_jit_inner` classifies through the cache, and `maybe_compile_and_run` and `try_function_entry_jit` drop their repeated whole-frame scans, which ran on every back-edge and every Python call. Assisted-by: Claude
`register_jit_builtin_wrappers` records the `py_checked_arity_fn!` wrapper pointer each BuiltinCode actually stores for `math.sqrt`, `math.frexp` and `math.ldexp`; `is_math_sqrt_function` moves onto that comparison and `is_math_frexp_function` / `is_math_ldexp_function` join it. `jit_math_frexp_mantissa`, `jit_math_frexp_exponent` and `jit_math_ldexp_raw` are the raw entry points behind the emitted pure calls. `try_walker_specialize_builtin_type` lowers `space.type(w_obj)`. `try_walker_specialize_builtin_dict_get` guards the new `W_DictObject.keys_version` descriptor and reads the resolved entry value through `jit_dict_nth_value`. `try_walker_specialize_int_call`, `try_walker_specialize_math_frexp` and `try_walker_specialize_math_ldexp` join them in `dispatch_residual_call_iRd_kind`. Assisted-by: Claude
`try_walker_inline_user_call` unwraps a `_Method` callable
(baseobjspace.py:1254-1259), guards the Method class and its
`w_function`, and passes the live `w_self` field as the receiver
argument instead of baking one anchor's receiver.
`try_walker_inline_resolved_user_call` fills a missing positional tail
from `Function.defs_w` (function.py:188-193,217-231) behind a
`GuardValue` on the new live `defs_w` descriptor, then reads each
default out of the pinned tuple's `wrappeditems`. Method-form keyword
calls prepend the receiver before the `kwnames` permutation.
The blanket `POP_JUMP_IF_{NOT_}NONE` callee rejection is removed;
`requires_seeded_callee_frame` declines instead when the callee frame is
not actually seeded.
`fbw_callee_body_replay_safety` takes per-parameter `ExactNumericArg`
provenance and tracks the exact-numeric and exact-int facts separately
per register and per frame slot, so
`residual_call_is_specialized_plain_numeric_binop` judges the actual
operands of each binop; `residual_call_is_specialized_plain_int_binop`
supplies the bitwise-result fact.
Assisted-by: Claude
`tyref_is_niche_option_ptr` folded `Discriminant` to a pointer null test for
`Option<&T>` as well as `Option<&mut T>` and `Option<NonNull<T>>`.
`Option<&T>` is the `Iterator::next` result shape, and `front::iter_next`
recognizes that call by its `__discriminant` match diamond; with the
discriminant folded there is no diamond left to rewrite, so the residual
`Iterator::next()` survives as the unregistered callee the rewrite removes.
`type_node_is_ref` becomes `type_node_is_mut_ref` and reads the kind field of
the Charon `{"Ref": [region, ty, kind]}` node instead of accepting any `Ref`.
Fixes tests/test_iter_next.rs branch_loop_sum_lifts_next_to_iter_next_op and
tests/test_desugar_mir.rs lowers_desugar_mix_with_aggregate_and_question_mark.
Assisted-by: Claude
`try_walker_inline_resolved_user_call` declines again on: - a method-form callee whose body `method_form_callee_body_supported` rejects. The `requires_seeded_callee_frame` exemption is removed (synth/float_subclass_binop_dispatch, synth/polymorphic_binary_receiver). - a `Dirty` replay-safety body. Its residual can raise, and the local `except` that catches it is a callee-owned catch edge the inline path does not compile, so the exception escapes the caller instead of being handled where the source handles it (synth/bool_dunder_error_no_leak, synth/pickle_ctor_args). With `Dirty` rejected, `requires_seeded_callee_frame` is always false; it and the three branches it guarded are removed. Assisted-by: Claude
`try_walker_inline_builtin_call` propagated `OrthodoxSubWalkTraceUnsupported` out of its sub-walk with `?`. `try_execute_residual_call_via_executor` raises that error before running the call, and this walk is the authoritative executor, so the aborted trace resumed past a Python CALL whose effect never happened: `collections.deque.popleft()` returned its value and left the element in the deque (synth/gc_deque_backing_list grow_each_round 2966 -> 987234, one `popleft` dropped per trace transition). The descent now cuts the trace back to the position recorded before its first emitted op, resets the heap cache and returns `Ok(None)` so the ordinary residual call runs — the rollback `orthodox_list_append_commit`'s callers already use. It is gated on the descent having executed no journaled or unjournaled effect, since a descent that applied one cannot be rewound this way and keeps the abort. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ab57c7a0
ℹ️ 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".
| let positional_defaults = if callee_args.len() < nparams { | ||
| let Some(defaults) = | ||
| (unsafe { positional_defaults_for_inline(callable, callee_args.len(), nparams) }) | ||
| else { |
There was a problem hiding this comment.
For a hot call such as def f(a=1, *args): return args; f(), this new default fill makes callee_args.len() == nparams, so the call becomes eligible for inlining even though nparams excludes the *args local. The inline frame seeds only the defaulted a slot and never constructs the empty tuple that the interpreter binds to args, so the compiled body can read an uninitialized/null local instead of (). Require fbw_callee_scope_is_positional_only(w_code) on this path or explicitly initialize the extra argument slots.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
| let value = unsafe { | ||
| if !pyre_object::is_exact_builtin_instance(arg_obj) || !pyre_object::is_int(arg_obj) { | ||
| return Ok(None); |
There was a problem hiding this comment.
When tracing a hot math.isqrt(True) or math.isqrt(False) call, is_int admits the boolean because it includes BOOL_TYPE, but the emitted specialization subsequently unboxes and guards the operand as INT_TYPE with the canonical int class. That contradicts the recording-time boolean layout, causing the trace to be rejected or to side-exit on every execution even though the interpreter accepts bools. Either exclude bool here or select the bool-specific type and field descriptor as the other numeric specializations do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/descr.rs (1)
2761-2792: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet the correct
index_in_parentfor theExecutionContextfieldsThe closure’s
indexis the global descriptor handle, whileindex_in_parentis the positional field slot withinExecutionContext. Hardcoding both fields to0deviates from the static group layout; emit distinct positional indices, e.g.0forsys_exc_valueand1fortopframeref.🤖 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 `@pyre/pyre-jit-trace/src/descr.rs` around lines 2761 - 2792, Update the `field` closure in `EC_DESCR_GROUP` so `index_in_parent` is supplied per field rather than hardcoded to zero, assigning positional slots 0 and 1 to `sys_exc_value` and `topframeref` respectively while preserving their global `index` values.
♻️ Duplicate comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs (1)
1960-1970: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
CallKwresidual operands are still fed into the builtin wrapper unpermuted.The eligibility guard admits
PyreHelperKind::CallKw, but acall_kwresidual's ref list is[callable, null_or_self, kwnames, args...].wrapper_items(Line 2139) is built from&r_args[2..], sokwnamesbecomes wrapper positional 0 and the array length handed to the wrapper'sarraylencheck is one greater than the real argument count. Anydict.get(k, default=…)-shaped builtin call routed here enters the generated wrapper with a shifted argument slice.Restrict this path to
CallFn, or skip/permutekwnamesbefore buildingwrapper_items.🐛 Minimal fix: restrict to the positional helper
- || !matches!( - pyre_helper, - majit_ir::PyreHelperKind::CallFn | majit_ir::PyreHelperKind::CallKw - ) + || pyre_helper != majit_ir::PyreHelperKind::CallFn🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 1960 - 1970, Restrict the eligibility guard in the inline-call dispatch path to majit_ir::PyreHelperKind::CallFn only, excluding CallKw until its kwnames operand is explicitly removed or permuted before wrapper_items is built. Preserve the existing positional argument handling and guard behavior for CallFn.
🤖 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-translate/src/lib.rs`:
- Around line 1909-1926: Update the builtin-wrapper target collection around
builtin_wrapper_indirect_graphs so every path missing from
call_control.jitcodes() emits a diagnostic before being discarded, matching the
mismatch handling in register_trait_families. Preserve the existing conversion
and extension of valid JitCodeHandle targets, and apply the same diagnostic
behavior to the corresponding target-building block as well.
In `@majit/majit-translate/src/translator/rtyper/rpbc.rs`:
- Around line 326-345: Add a focused unit test for lower_indirect_calls that
constructs an IndirectCall with Some([]) and verifies its graphs are replaced by
builtin_wrapper_indirect_graphs(), while an IndirectCall with Some(non_empty)
retains its original graphs.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1613-1627: Separate the stored bound-method state from method_form
in try_walker_inline_resolved_user_call: retain method_form for the LOAD_METHOD
split-receiver shape and use bound_method presence for the unwrapped Method
shape. In pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs lines
1613-1627, stop making the two predicates equivalent; update lines 2586-2596 so
foriter_dirty_bound can be set without requiring !method_form, and rewrite lines
3086-3097 to gate the PopJump scan using the corrected distinction rather than
the currently tautological (bound_method.is_none() || method_form) condition.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs`:
- Around line 5289-5294: Update the guard in the visible unsafe value-extraction
block to explicitly reject bool objects before calling is_int or
w_int_get_value. Mirror the bool exclusion used by
try_walker_call_assembler_self_recursive, while preserving the existing Ok(None)
behavior for rejected arguments and int handling for non-bool exact builtins.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Around line 96-113: Update the JitCode lookup in
random_core_residuals_use_registered_genrand32_address to select the target by
name == "random" only, then assert separately that decoded_ops contains exactly
two residual_call_r_i/iRd>i operations. Correct the expect message from
"rrandom" to clearly identify Random::random.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 2761-2792: Update the `field` closure in `EC_DESCR_GROUP` so
`index_in_parent` is supplied per field rather than hardcoded to zero, assigning
positional slots 0 and 1 to `sys_exc_value` and `topframeref` respectively while
preserving their global `index` values.
---
Duplicate comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1960-1970: Restrict the eligibility guard in the inline-call
dispatch path to majit_ir::PyreHelperKind::CallFn only, excluding CallKw until
its kwnames operand is explicitly removed or permuted before wrapper_items is
built. Preserve the existing positional argument handling and guard behavior for
CallFn.
🪄 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: 588dcb24-b12b-4499-a89b-85a31cf2befb
📒 Files selected for processing (41)
Cargo.tomlmajit/majit-gc/src/collector.rsmajit/majit-ir/src/eval_breaker_word.rsmajit/majit-metainterp/src/compile.rsmajit/majit-translate/src/codegen.rsmajit/majit-translate/src/codewriter/call.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/pipeline.rsmajit/majit-translate/src/translator/rtyper/flowspace_adapter.rsmajit/majit-translate/src/translator/rtyper/rpbc.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/module/_random/mod.rspyre/pyre-interpreter/src/module/math/interp_math.rspyre/pyre-interpreter/src/module/math/mod.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit-trace/src/unpack_state.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/call.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/bufferview.rspyre/pyre-object/src/gc_interp.rspyre/pyre-object/src/gc_roots.rs
| // `BuiltinCode.func` is a SomePBC function-pointer field. The ordinary | ||
| // translated indirect-call op contributes this family through | ||
| // `IndirectCallTargets`; pyre's interpreter call boundary hides that op | ||
| // behind the runtime `call_fn` helper, so publish the annotator's same | ||
| // finite wrapper family on the shared Assembler explicitly. The handles | ||
| // are the exact `CallControl.jitcodes` objects materialized by | ||
| // `grab_initial_jitcodes`, preserving RPython object identity. | ||
| let builtin_wrapper_targets: Vec<jitcode::JitCodeHandle> = call_control | ||
| .builtin_wrapper_indirect_graphs() | ||
| .into_iter() | ||
| .filter_map(|path| call_control.jitcodes().get(&path).cloned()) | ||
| .map(jitcode::JitCodeHandle::from) | ||
| .collect(); | ||
| codewriter | ||
| .assembler | ||
| .indirectcalltargets | ||
| .extend(builtin_wrapper_targets); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Silent gaps between the compile-time wrapper family and the runtime indirect-call target list.
builtin_wrapper_indirect_graphs() paths that are missing from call_control.jitcodes() are dropped here with no diagnostic, unlike other registry-mismatch sites in this file (e.g. register_trait_families) which eprintln! on a mismatch. Since the very same family list is baked into every IndirectCall.graphs marker in rpbc.rs::lower_indirect_calls for compile-time analysis, a silent drop here can desynchronize the runtime-validated target set from what the compiler assumed was callable.
♻️ Suggested diagnostic on drop
let builtin_wrapper_targets: Vec<jitcode::JitCodeHandle> = call_control
.builtin_wrapper_indirect_graphs()
.into_iter()
- .filter_map(|path| call_control.jitcodes().get(&path).cloned())
+ .filter_map(|path| {
+ let found = call_control.jitcodes().get(&path).cloned();
+ if found.is_none() {
+ eprintln!(
+ "make_jitcodes: builtin wrapper {path:?} was never drained into jitcodes; \
+ omitted from indirectcalltargets"
+ );
+ }
+ found
+ })
.map(jitcode::JitCodeHandle::from)
.collect();Also applies to: 1937-1944
🤖 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-translate/src/lib.rs` around lines 1909 - 1926, Update the
builtin-wrapper target collection around builtin_wrapper_indirect_graphs so
every path missing from call_control.jitcodes() emits a diagnostic before being
discarded, matching the mismatch handling in register_trait_families. Preserve
the existing conversion and extension of valid JitCodeHandle targets, and apply
the same diagnostic behavior to the corresponding target-building block as well.
| pub fn lower_indirect_calls(graph: &mut JitFunctionGraph, call_control: &CallControl) { | ||
| // Generated gateway wrappers enter the MIR graph as a plain function- | ||
| // pointer `IndirectCall` with `Some([])` as a deferred PBC-family marker. | ||
| // At rtype time CallControl owns both the translated graphs and the | ||
| // linker-resolved wrapper addresses, so fill the same `c_graphs` list | ||
| // `FunctionReprBase.call()` appends in rpbc.py:216. | ||
| let builtin_wrappers = call_control.builtin_wrapper_indirect_graphs(); | ||
| for block in &mut graph.blocks { | ||
| for op in &mut block.operations { | ||
| if let OpKind::IndirectCall { | ||
| graphs: Some(graphs), | ||
| .. | ||
| } = &mut op.kind | ||
| && graphs.is_empty() | ||
| { | ||
| *graphs = builtin_wrappers.clone(); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider a dedicated unit test for the deferred-marker fill.
The new pre-pass silently depends on the invariant that Some([]) is only ever the gateway-wrapper marker (never a legitimately-empty family, since those are normalized to None elsewhere). A small regression test asserting a Some([]) IndirectCall gets filled with builtin_wrapper_indirect_graphs() and a Some(non_empty) one is left untouched would protect this invariant from silent regressions.
🤖 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-translate/src/translator/rtyper/rpbc.rs` around lines 326 - 345,
Add a focused unit test for lower_indirect_calls that constructs an IndirectCall
with Some([]) and verifies its graphs are replaced by
builtin_wrapper_indirect_graphs(), while an IndirectCall with Some(non_empty)
retains its original graphs.
| let bound_method = if !method_form && unsafe { pyre_object::is_method(callable) } { | ||
| let function = unsafe { pyre_object::w_method_get_func(callable) }; | ||
| let receiver = unsafe { pyre_object::w_method_get_self(callable) }; | ||
| if function.is_null() || receiver.is_null() { | ||
| return Ok(None); | ||
| } | ||
| method_form = true; | ||
| Some(BoundMethodInline { | ||
| method_op: r_args[0], | ||
| function, | ||
| receiver, | ||
| }) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
method_form is set on the same branch that builds BoundMethodInline, making both new bound-method predicates tautological. Because method_form = true is assigned inside the is_method(callable) arm, bound_method.is_some() always implies method_form, and every other caller of try_walker_inline_resolved_user_call passes bound_method: None. Both downstream predicates that try to separate "stored bound method" from "LOAD_METHOD split receiver" therefore evaluate to a constant.
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L1613-L1627: carry the stored-bound-method distinction separately frommethod_form(e.g. keepmethod_formfor the split-receiver shape and rely onbound_methodalone for the unwrapped-Methodshape).pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L2586-L2596: drop the!method_formterm soforiter_dirty_boundcan actually be set, or theDirtyadmission and the Lines 2751-2753 gate remain dead code.pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L3086-L3097: rewrite the(bound_method.is_none() || method_form)gate against the corrected distinction; today it is alwaystrueand the PopJump scan runs unconditionally.
📍 Affects 1 file
pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L1613-L1627(this comment)pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L2586-L2596pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L3086-L3097
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 1613 -
1627, Separate the stored bound-method state from method_form in
try_walker_inline_resolved_user_call: retain method_form for the LOAD_METHOD
split-receiver shape and use bound_method presence for the unwrapped Method
shape. In pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs lines
1613-1627, stop making the two predicates equivalent; update lines 2586-2596 so
foriter_dirty_bound can be set without requiring !method_form, and rewrite lines
3086-3097 to gate the PopJump scan using the corrected distinction rather than
the currently tautological (bound_method.is_none() || method_form) condition.
| let value = unsafe { | ||
| if !pyre_object::is_exact_builtin_instance(arg_obj) || !pyre_object::is_int(arg_obj) { | ||
| return Ok(None); | ||
| } | ||
| pyre_object::w_int_get_value(arg_obj) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
is_int accepts bool, so math.isqrt(True) records an int-payload unbox against a bool object.
is_exact_builtin_instance + is_int admits W_BoolObject (the same hazard try_walker_call_assembler_self_recursive calls out at Lines 714-719 of inline_call.rs). The subsequent walker_unbox_int(..., INT_TYPE) and w_int_get_value(arg_obj) then read the bool through the int accessor/descr. The exact-w_class guard emitted afterwards makes the compiled trace deopt, but the recording-time concrete stamped onto raw_int is read through the wrong accessor.
Decline bool explicitly, as the CALL_ASSEMBLER arm does.
🐛 Proposed fix
let value = unsafe {
- if !pyre_object::is_exact_builtin_instance(arg_obj) || !pyre_object::is_int(arg_obj) {
+ if !pyre_object::is_exact_builtin_instance(arg_obj)
+ || !pyre_object::is_int(arg_obj)
+ || pyre_object::is_bool(arg_obj)
+ {
return Ok(None);
}
pyre_object::w_int_get_value(arg_obj)
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let value = unsafe { | |
| if !pyre_object::is_exact_builtin_instance(arg_obj) || !pyre_object::is_int(arg_obj) { | |
| return Ok(None); | |
| } | |
| pyre_object::w_int_get_value(arg_obj) | |
| }; | |
| let value = unsafe { | |
| if !pyre_object::is_exact_builtin_instance(arg_obj) | |
| || !pyre_object::is_int(arg_obj) | |
| || pyre_object::is_bool(arg_obj) | |
| { | |
| return Ok(None); | |
| } | |
| pyre_object::w_int_get_value(arg_obj) | |
| }; |
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs` around lines 5289 -
5294, Update the guard in the visible unsafe value-extraction block to
explicitly reject bool objects before calling is_int or w_int_get_value. Mirror
the bool exclusion used by try_walker_call_assembler_self_recursive, while
preserving the existing Ok(None) behavior for rejected arguments and int
handling for non-bool exact builtins.
| #[test] | ||
| fn random_core_residuals_use_registered_genrand32_address() { | ||
| let expected = pyre_interpreter::jit_trace_fnaddrs() | ||
| .into_iter() | ||
| .find_map(|(path, address)| { | ||
| (path == "module::_random::Random::genrand32").then_some(address) | ||
| }) | ||
| .expect("genrand32 runtime fnaddr"); | ||
| let random = crate::jitcode_runtime::all_jitcodes() | ||
| .iter() | ||
| .find(|jitcode| { | ||
| jitcode.name == "random" | ||
| && crate::jitcode_runtime::decoded_ops(&jitcode.code) | ||
| .filter(|op| op.key == "residual_call_r_i/iRd>i") | ||
| .count() | ||
| == 2 | ||
| }) | ||
| .expect("rrandom Random::random jitcode"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This selector will panic opaquely if the lowering changes.
Identifying the target JitCode by name == "random" and exactly two residual_call_r_i/iRd>i ops folds the assertion's premise into the lookup: a lowering change that emits three residual calls turns the intended assertion failure into .expect("rrandom Random::random jitcode"). Prefer selecting on name alone and asserting the residual-call count separately (the panic message also reads rrandom).
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs` around lines 96 - 113,
Update the JitCode lookup in
random_core_residuals_use_registered_genrand32_address to select the target by
name == "random" only, then assert separately that decoded_ops contains exactly
two residual_call_r_i/iRd>i operations. Correct the expect message from
"rrandom" to clearly identify Random::random.
* perf(jit): cache exact jitcode assembly declines * jit: expose manual builtin gateways to translation * jit: preserve runtime hints and float bitcasts * jit: inline stored bound methods through dirty bodies * jit: preserve Python frames through builtin gateways * jit: lower rebuilt Result shells as allocations * jit: preserve nested loop and field identity * jit: restrict the builtin-wrapper fold to positional calls `try_walker_inline_builtin_call` admitted both `PyreHelperKind::CallFn` and `CallKw`, then built the generated wrapper's flat positional argument array from `r_args[2..]`. A `call_kw` residual carries its kwnames tuple at arg index 2 — `Instruction::CallKw` emits `callable, null_or_self, kwnames, arg0..argN-1` and `bh_call_kw_<n>` consumes that same order — so the tuple became the wrapper's first positional value and the array length exceeded the real argument count by one. Keywords reach a generated wrapper as the trailing `__pyre_kw__` marker dict that `split_builtin_kwargs` strips, and this fold builds no such dict. Decline `CallKw` so those calls keep the ordinary residual, as `CallFunctionEx` already does. Assisted-by: Claude * jit: require a positional-only callee scope for the walker inline lever `try_walker_inline_resolved_user_call` admitted a callee on `callee_args.len() == nparams` alone, where `nparams` is `co_argcount` as returned by `resolve_inlinable_callee`. `co_argcount` counts neither `*args` nor `**kwargs` nor keyword-only parameters, while the inline frame seeding stores only `param_boxes[0..nparams]` into a `NewArrayClear` array, so a `*args` local read PY_NULL where `pack_varargs` binds `()`. The positional-defaults fill widened the set of calls that reach the arity test. Consult `fbw_callee_scope_is_positional_only` beside the arity gate in the shared resolved half, which covers every admission path, and drop the now redundant check in the CALL_FUNCTION_EX branch. Assisted-by: Claude * jit: decline bool operands in the math.isqrt specialization `pyre_object::is_int` accepts a bool, but the emitted specialization unboxes through `INT_TYPE` and guards the canonical `int` `w_class`, neither of which holds for a bool singleton. Assisted-by: Claude * jit: re-check the dict keys_version under the dict lock The `dict.get` specialization guarded `W_DictObject.keys_version` with an unlocked field read and then called `jit_dict_nth_value`, which took the dict lock only for the indexed read. A key-set mutation between the two compacts the `IndexMap`, so the promoted index named a different key. Replace the helper with `jit_dict_nth_value_versioned`, which holds one reentrant `w_dict_lock` across the version re-check and the read and returns PY_NULL on a mismatch, and emit a `GuardNonnull` on its result. Assisted-by: Claude * jit: return the accepted binop class from one decode `residual_call_is_specialized_plain_numeric_binop` and `residual_call_is_specialized_plain_int_binop` each decoded the body `BINARY_OP` tag out of the constants window and each carried the `And`/`Or`/`Xor` (+ in-place) operator set, which had to stay in lockstep. Return `Option<SpecializedBinop>` from the first and delete the second. Assisted-by: Claude * jit(descr): rank ExecutionContext field descrs by byte offset `EC_DESCR_GROUP` built both fields through one closure that hardcoded `index_in_parent: 0`. `make_simple_descr_group` copies that value verbatim and binds a parent SizeDescr, and `OptHeap::field_slot_index` prefers `index_in_parent` over `descr.index()` whenever a parent is bound, so `sys_exc_value` and `topframeref` resolved to one `PtrInfo._fields` slot. Sort the specs by offset and stamp `index_in_parent` from that position, and resolve both accessors by offset rather than by declaration order. Assisted-by: Claude * majit: order the builtin-wrapper alias pick totally and memoise the family `builtin_wrapper_indirect_graphs` bucketed aliases by iterating the `function_fnaddrs` HashMap and picked with `sort_by_key(Reverse(segment count))`, which is stable, so two aliases of one address with equal segment counts resolved on iteration order. Compare the segment sequences and demote the `crate` placeholder so the order is total. The family was also rebuilt per IndirectCall op and per drained graph. Memoise it in a `OnceCell`; `function_fnaddrs` and `function_graphs` are written only in the setup phase that precedes every reader. `lib.rs` indexes `jitcodes()` instead of `filter_map`, since `grab_initial_jitcodes` has already inserted every path in the family. Assisted-by: Claude * jit: consult the cached frame-shape classification on the portal entry paths `try_function_entry_jit` and `maybe_compile_and_run` stopped consulting `unsupported_jit_shape` on the premise that `eval_with_jit_inner` classifies every frame first. `portal_runner_dispatch` reaches both without that: `compile_tmp_callback` bakes `portal_runner_adr` as the whole callee body and the `!is_resolved` CALL_ASSEMBLER force leg calls the same shim, so the counter tick could start a trace for an excluded shape. Consult `cached_unsupported_jit_shape`, a pointer-keyed lookup into `CallControl.graph_jit_shapes`, rather than the whole-frame scan that classification cache replaced. Assisted-by: Claude * interp: route the variable-arity argument-count error through a gateway helper The generated wrapper's "expected at least N arguments" branch built its message with `format!` inline in the traced body, while the fixed-arity and no-arg branches call `#[dont_look_inside]` `method_arity_failure` / `method_noarg_failure`. Add `method_min_arity_failure` carrying the same attribute, register its fnaddr aliases, and call it from the macro. Assisted-by: Claude * jit: preserve struct identity across field owner spellings * jit(descr): compare the reconciled field description on a get_field_descr cache hit `GcCache::get_field_descr` mints a descr with reconciled metadata, but its cache-hit `debug_assert!` compared the caller's raw arguments against it. `derive_index_in_parent` re-derives the stored `index_in_parent` from the parent that will actually be indexed, so a caller's own numbering never reaches the cached descr. Pyre reaches one struct through several `all_fielddescrs` walks that number their lists independently: the runtime group over the declared payload numbers `W_IntObject.intval` 0, and the walk that models the inherited `PyObject` header numbers it 2. `heaptracker.py:62-64` / `:102-103` skip `typeptr` in both the list and the index, so the header-free numbering is the upstream one. The assert reported every such split as a disagreement; it now derives the caller's index the same way before comparing. `front/mir.rs` leaves `SemanticProgram::immutable_fields` empty for the whole LLBC pipeline — Charon serializes doc comments but not the `#[jit_immutable_fields]` hint — so a spec built from that side reports `(is_immutable, is_quasi_immutable) == (false, false)` for every field. The hit path already resolves that by keeping the cached descr's flags; the assert now compares those. A caller claiming purity a cached descr denies still trips. Both fired only in debug builds, where they cost 28 `cargo test` failures: one panic plus 27 tests failing on the descr mutex the panic poisoned. Downgrading the assert to a print reports 16 distinct pairs over `W_IntObject` `W_FloatObject` `W_LongObject` `Method` `W_Range` `W_IntRangeIterator` `PyFrame` — 9 index-only, 7 immutability-only — and the suite passes with it downgraded, so the cached descr already won. The cache-hit message also names both descrs' owners, which is what identified the two producers. Assisted-by: Claude * jit(bh): register new/d>r in the production blackhole builder Lowering a rebuilt `Result` shell as an allocation made `OpKind::New` reachable from the codewriter, so `build_emitted_insns()` now records `new/d>r` while `build_inline_call_only_bh_builder`'s curated `setup_insns` map did not carry the byte. `handler_new` was already wired, so this was a registration gap, not an implementation gap: `wire_handler` no-ops without the map entry and the byte stays unwired until a forward resume lands on it and `dispatch_step` panics. The operand shape matches the wired decoder. `assembler.rs OpKind::New` emits a 2-byte little-endian descr index then the 1-byte ref register holding the result; `handler_new` reads exactly that through `read_descr` + `code[pos]`. That is `new_with_vtable/d>r`'s shape, already registered beside it, and both read `bh.cpu`, which this builder sets. `production_bh_builder_covers_every_build_emitted_opname` and `production_bh_builder_overlay_only_gap_snapshot` failed on this opname; the snapshot drops it and records why it left, as it does for `vtable_method_ptr/rd>i`. cargo test --all --no-default-features --features dynasm: 0 failed. Assisted-by: Claude * test(jit): cover nested virtual append payloads * test(jit): select the keyword wrapper's argument slice by descr, not by position `keyword_builtin_wrapper_finds_colored_argument_slice_item_descr` picked the wrapper's argument slice as "the first `arraylen_gc`" and pinned the entry call's result colour to `inline_call_r_r/dR>r`. Both name a shape rather than the property, and both stopped naming it once `split_builtin_kwargs` inlined further: the entry call now yields the leading `args.is_empty()` test by value (`inline_call_r_i/dR>i`) instead of the `(&[PyObjectRef], Option<PyObjectRef>)` pair by reference, and the `args.len()` that inlined body reads off the wrapper's own `r0` is now the first `arraylen_gc`. The property held throughout. Every `getarrayitem_gc_r` carrying the argument-slice item descr reads `r6`, and the only `arraylen_gc` on `r0` is that pre-split `args.len()`. So the item descr is selected directly, the off-`r0` assertion is made about the register that read reaches the slice through, and the length read is required on that same register. Reproducing this needs current LLBC: `build/llbc/` predating the lowering change still yields the old shape, and the test passes against it on every target. Verified with a fresh extraction — `pyre-jit-trace --lib`, 312 passed. Assisted-by: Claude * interp: drop the duplicate interp_return_log_enabled definition `eval.rs` carries two definitions of `interp_return_log_enabled`, at :589 and :617. Both are `#[cfg(not(feature = "sandbox"))]` with the same body — a `OnceLock<bool>` over `PYRE_INTERP_RETURN_LOG` — and only their doc comments differ, so `pyre-interpreter` fails to compile with E0428 and the Charon/LLBC extraction step exits before any other CI job runs. The pair is inherited, not produced by the rebase: `origin/main` `25b2442c4e` holds both. #874 added the first; #907 added the second and merged on top without seeing it, since each PR's CI builds only its own merge commit. Keeps the :589 copy and its comment; the sole call site at :2729 is unchanged. Assisted-by: Claude
1 participant
Twelve commits on the interpreter and JIT call paths. PR #862 (the
gc/_iofinalizer work this branch previously carried) is merged; everything below is
new on top of it.
The interpreter's CALL fast path did not know about
Methodbaseobjspace.py:1243-1266takes the Function valuestack path for both plainand method-form calls, and
:1254-1259unwraps_Methodbefore that pathrather than treating it as a generic callable. pyre's CALL handler declined on
a non-null
null_or_selfand on aMethodcallable, soobj.method(...)andevery module alias built as
random.gauss = _inst.gaussallocated anArgumentsvec per call instead of going throughfunccall_valuestack.The handler now reuses the null/self stack slot for
w_instance, continueswith
w_function, and passesmethodcallwith the receiver counted as oneextra argument (
callmethod.py:85-94) while the popped width stays thephysical
[callable, null_or_self, args...]. That is what lets the walker seean ordinary
_flat_pycallforobj.method(...), as PyPy does.Per-opcode polls
bytecode_traceenteredapply_all_thread_hooks— and its mutex-bearing slowarms — at every opcode. PyPy's common path is only its trace check and action
ticker; pyre's extra obligation is noticing process-wide
_settraceallthreads/_setprofileallthreadschanges, whichall_thread_hooks_currentnow answers with two generation loads.The last commit folds the remaining independent polls into the
already-established eval breaker word — PyPy's
ActionFlagis one processbreaker — adding
EB_FINALIZING(bit2, terminal) andEB_GC_INTERP(bit3,process-stable), plus
JIT_BREAKER_MASK. Both eval loops now pay one relaxedload where they previously polled
park_if_finalizing,gc_interp::safepointandgc_sync::safepoint_pollseparately. It alsocarries the translated builtin-call work:
builtin_wrapper_indirect_graphs/build_indirectcalltargets, thebuiltin_kwargs_marker_dictandmethod_arity_failure/method_noarg_failuregateway seams, andshadow_stack_copy_range.The
gc_interp::enabled()commit hoistsgc_interp::enabled()out of the JIT loop's safepointpoll.
EB_GC_INTERPsupersedes it; it is kept as its own commit so it can bedropped independently.
JIT entry cost
unsupported_jit_shape— a pyre-only safety gate that walks a code object'swhole constant tree and bytecode — ran at every back-edge
(
maybe_compile_and_run) and every Python call (try_function_entry_jit),where RPython's
can_enter_jit/maybe_compile_and_runare unconditional.The classification is an immutable per-graph fact, so it now lives in
CallControl.graph_jit_shapesbesidejitcodes, keyed the same way, andeval_with_jit_inneris the one place that computes it.jd1 (
unpackiterable_driver) goes back behindPYRE_JD1=1. Unlike RPython,pyre drives it through the same
MetaInterp.tracingslot as the bytecodeportal, so while a residual
next()runs an arbitrarily large generator bodythe jd1 trace holds only the opaque call — and the shared flag suppresses every
jd0 merge point that body reaches. The second driver stays dormant until it has
RPython's independent recursive-portal behaviour.
Walker specializations
type(x)isspace.type(w_obj)upstream — promote__class__, returngetclass— so it lowers directly instead of residualizing the type object'sfull
descr_call. Exactdict.get(identity_key)guards the newW_DictObject.keys_versiondescriptor (pyre's explicit form of the livestrategy-iterator state
dictmultiobject.py:807-845carries implicitly: keyinsertion/removal/strategy replacement bumps it, value replacement deliberately
does not) to pin the resolved entry index, then reads that entry's value live.
int(),math.frexp,math.ldexpandmath.isqrtjoinmath.sqrt.The math identity probes were comparing against the wrong pointer:
py_checked_arity_fn!wraps each body in a non-capturing closure, so aBuiltinCode stores the wrapper, never
sqrtitself.register_jit_builtin_wrappersrecords the pointers the module namespaceactually installed, and the probes compare those — a rebound
math.frexpstilldeclines the specialization.
ll_math_frexp's pair is emitted as two purecalls because the IR has no multi-result call opcode;
jit_math_ldexp_rawreturns signed infinity on overflow so the finite-result guard deoptimizes and
the ordinary builtin raises
OverflowError.Bound-method and default-argument inlining
try_walker_inline_user_callunwraps a_Methodcallable, guards the Methodclass and its
w_function, and passes the livew_selffield as thereceiver — baking one anchor's receiver would collapse bound methods that
differ only in
self.Function.defs_w(function.py:188-193,217-231) nowfills a missing positional tail behind a
GuardValueon a livedefs_wdescriptor, with each default read out of the pinned tuple's
wrappeditems;that descriptor is deliberately mutable rather than quasi-immutable, because
function_set_defaultsdoes not yet calldo_force_quasi_immutableandmarking it would leave compiled loops alive after
f.__defaults__ = ....Method-form keyword calls prepend the receiver before the
kwnamespermutation.
The replay-safety scan carried its exact-numeric fact as two whole-call
booleans, which cannot describe a method-form call where
selfis nonnumericand a later argument is an exact int. It now takes per-parameter
ExactNumericArgprovenance and tracks exact-numeric and exact-int separatelyper register and per frame slot, so
residual_call_is_specialized_plain_numeric_binopjudges the actual operandsof each binop rather than the call's arguments.
Build
[profile.release]getslto = "thin"andcodegen-units = 1— the samecross-crate optimization
[profile.dist]already inherits — socargo run --release, which is the benchmark surface, is not measuringparallel-CGU barriers through the interpreter's dispatch graph.
Corrections to the above
Three commits fix defects the work above introduced; each is separate so it can
be reviewed against the commit it corrects.
tyref_is_niche_option_ptrhad been widened to foldDiscriminantonOption<&T>to a pointer null test. That is theIterator::nextresult shape,and
front::iter_nextrecognizes the call by its__discriminantmatchdiamond — with the discriminant folded there is no diamond left, so the
residual
Iterator::next()survived as exactly the unregistered callee therewrite exists to remove. The predicate now reads the kind field of the Charon
{"Ref": [region, ty, kind]}node and accepts mutable references only.Two
try_walker_inline_resolved_user_calldeclines are restored. A method-formcallee whose body
method_form_callee_body_supportedrejects is declined again(the
requires_seeded_callee_frameexemption is gone), and aDirtyreplay-safety body is not admitted by seeding its frame: its residual can
raise, and the local
exceptthat catches it is a callee-owned catch edge theinline path does not compile, so the exception escaped the caller instead of
being handled where the source handles it. With
Dirtyrejected,requires_seeded_callee_frameis always false and it and the three branches itguarded are removed.
try_walker_inline_builtin_callpropagatedOrthodoxSubWalkTraceUnsupportedout of its sub-walk with
?.try_execute_residual_call_via_executorraisesthat error before running the call, and this walk is the authoritative
executor, so the aborted trace resumed past a Python CALL whose effect never
happened —
collections.deque.popleft()returned its value and left theelement in the deque, once per trace transition. The descent now cuts the trace
back to the position recorded before its first emitted op, resets the heap
cache and returns
Ok(None)so the ordinary residual call runs, which is therollback
orthodox_list_append_commit's callers already use. It is gated onthe descent having executed no journaled or unjournaled effect, since a descent
that applied one cannot be rewound this way and keeps the abort.
Gate
pyre/check.py --backend dynasm,cranelift,wasmon this branch matchesorigin/mainexactly: both are red only onsynth/str_search_index_bounds(
compile.py:458 assert i == len(inputargs) failed (16 != 26), all threebackends). That failure was reproduced on
origin/mainalone in this worktreewith every branch-changed file reverted and the LLBC corpus re-extracted, and
independently in a second worktree; it is not from this branch.
— authored by Claude
Summary by CodeRabbit
Performance
type,dict.get,int, and several math functions, includingfrexp,ldexp, andisqrt.Bug Fixes