jit: inline CALL_KW / CALL_FUNCTION_EX and lift the FOR_ITER-body inline gate - #779
Merged
Conversation
… time The FBW inline lever admitted only `PyreHelperKind::CallFn`, so every CALL_KW site stayed a `bh_call_kw` residual that re-ran the whole keyword binding — frame allocation, signature match, kwnames binding, argument boxing — on each iteration. Admit `PyreHelperKind::CallKw` too. `fbw_reorder_call_kw_args` reads the constant kwnames tuple and the callee's `co_varnames`, both known at record time, and reorders the argument boxes into parameter order so the existing positional seeding in `try_walker_inline_resolved_user_call` serves the call unchanged. It declines to the residual for anything it cannot settle statically: a non-constant or non-str kwnames tuple, a name that matches no parameter or one already filled positionally, `*args`/`**kwargs`/kwonly callees, an argument count other than the parameter count, and the bound-method form. The receiver slot (argument index 1) arrives as `ConcreteValue::Null` for `call_kw` where `call_fn` produces `Ref(PY_NULL)`; both are the checked "no receiver" sentinel, so accept either. `synth/call_kw_hot_loop` drops from ~700ns to ~1.0ns per iteration and its compiled loop records no `call_may_force`. Assisted-by: Claude
`fbw_callee_body_side_effect_free` rejected a callee body at its first
`LOAD_GLOBAL` or `box_int` residual. Those two, with `load_const`, carry
`CanRaise` only to satisfy the `_OS_CANRAISE` invariant (effectinfo.rs);
each is a read or a fresh allocation and commits nothing to the live heap,
so the `check_is_elidable() || LoopInvariant` proxy mis-rejected them.
Rename the predicate to `fbw_callee_body_replay_safety` and return
`CalleeReplaySafety::{Clean, DeferredCall, Dirty}`. The three read/allocate
helpers join the provably-side-effect-free set. A `CallFn` / `CallKw`
residual no longer forces `Dirty`: its callee is a runtime value, so the
body reports `DeferredCall` and the decision moves to the call itself.
`fbw_abort_nested_unjournaled_residual` backs that deferral: while a
deferred-admitted sub-walk is active it aborts before executing any residual
that is not provably side-effect-free, and records the outermost deferred
callee in a deny set the gate then consults, so the abort costs one attempt
per callee rather than one per trace attempt.
The static scan clears every direct heap write and the backstop clears every
impure residual, so a deferred sub-walk is write-free wherever it can abort
or deopt, which is what both the walk-abort replay and the caller-boundary
deopt re-execute.
`for i in range(N): total += helper(i)` with `helper(i)` calling
`add(i, 1, 2)` goes from 3.67s to 0.11s at N=2M, matching the `while`-loop
form; `synth/call_kw_star` from 1.15s to 0.18s.
Assisted-by: Claude
…star calls `try_walker_specialize_newtuple_object` re-emits the canonical `W_TupleObject` shape (`new_with_vtable` + `w_class` / `wrappeditems` `setfield_gc` over a fresh items block) for the BUILD_TUPLE arities `makespecialisedtuple2` does not claim, reading the elements out of the array heap-cache so the `new_array_clear` build keeps no consumer. Arity 2 and the empty tuple are declined. Dispatched from `dispatch_residual_call_iRd_kind` after the existing `spec_ii` fold. `fbw_callee_body_replay_safety` tracks `new_array*` results as fresh and accepts a `setarrayitem_gc` into a fresh array as an initialization, adds `NewtupleFromArray` / `NewlistFromArray` to the replay-safe read/alloc set, and adds `CallFunctionEx` to the deferred-call set. `fbw_unpack_call_function_ex_args` binds a `f(*args)` star tuple to positional arguments by reading the element boxes from the heap cache, so the fold applies when the tuple is virtual at the call. It declines a `**` merge, an arity that is not the callee's `co_argcount`, the method form, and a tuple with no cached backing block. synth/call_function_ex_star: 0.89s -> 0.14s user, loops_aborted 0. check.py dynasm 308/308 + cranelift 308/308. Assisted-by: Claude
`fbw_callee_body_replay_safety` cleared its `fresh_ref_regs` set when the linear scan reached a `goto`. A join whose incoming branch sits earlier in the body then kept whatever freshness the fall-through predecessor had: in `r1 = getfield_gc(...); goto L; r1 = new_array; L: setarrayitem_gc r1` the scan reads `r1` as fresh at `L` although one path arrives holding a live-heap reference. Collect every label operand up front (`body_branch_targets`, covering the `goto` family, `catch_exception` and `int_*_jump_if_ovf`) and clear at the target pcs instead. A pc with no incoming branch edge keeps the exact linear state, so a conditional goto's fall-through no longer discards freshness. A label this decode cannot locate reports `Dirty`. No current producer reaches the join: `setarrayitem_gc` is emitted only by the BUILD_TUPLE / BUILD_LIST lowerings, whose target register is defined in the same basic block. Measured neutral on call_kw_hot_loop, call_kw_star, call_ex_kwargs_mapping and call_function_ex_star. Assisted-by: Claude
WalkthroughThe JIT tracer adds replay-safety classification and deferred-inline tracking for ChangesFOR_ITER inline replay safety
Array-backed tuple specialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InlineCall
participant ReplaySafety
participant CalleeWalk
participant ResidualDispatch
participant TupleSpecializer
InlineCall->>ReplaySafety: classify FOR_ITER callee
ReplaySafety-->>InlineCall: return replay safety
InlineCall->>CalleeWalk: run admitted callee sub-walk
CalleeWalk->>ResidualDispatch: process NewtupleFromArray residual
ResidualDispatch->>TupleSpecializer: try array-backed tuple specialization
TupleSpecializer-->>ResidualDispatch: continue with specialized tuple or fallback
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec7b3150e
ℹ️ 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".
Comment on lines
+1328
to
+1330
| let pi = varnames[..nparams] | ||
| .iter() | ||
| .position(|v| v.as_str() == name)?; |
There was a problem hiding this comment.
When a CALL_KW targets a function declared with positional-only parameters, this lookup treats those parameter names as keyword-bindable. For example, def f(x, /): ...; f(x=1) should raise TypeError, but the fold places the value into slot 0 and inlines the function body instead. Check posonlyarg_count before accepting the matched slot so the residual path preserves the interpreter's argument-binding semantics.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
Comment on lines
+1498
to
+1499
| let Some(unpacked) = fbw_unpack_call_function_ex_args(ctx, r_args, &arg_concretes, nparams) | ||
| else { |
There was a problem hiding this comment.
This CALL_FUNCTION_EX path passes only nparams into the unpacker and never rejects VARARGS, VARKEYWORDS, or keyword-only parameters. Consequently, a valid call such as def f(a, *, b=5): return b; f(*(1,)) can enter the inline path without constructing or seeding b; similarly, *args and **kwargs locals are never packed. Inspect the resolved CodeObject flags and kwonlyarg_count, as the keyword-call fold already does, and leave these signatures residual until their complete scope can be seeded.
AGENTS.md reference: AGENTS.md:L14-L20
Useful? React with 👍 / 👎.
Comment on lines
+985
to
+986
| static FBW_FORITER_DEFERRED_DENY: std::cell::RefCell<std::collections::HashSet<usize>> = | ||
| std::cell::RefCell::new(std::collections::HashSet::new()); |
There was a problem hiding this comment.
When multiple threads trace the same deferred callee, this thread-local set records the failed inline only on the thread that encountered it, so every other tracing thread repeats the abort that the comment says should occur once per callee for the rest of the process. It also retains unrooted code-object addresses for the lifetime of each thread. Store this persistent runtime cache on the shared interpreter/process owner rather than duplicating it in TLS.
AGENTS.md reference: AGENTS.md:L148-L162
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
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs`:
- Around line 1503-1516: Update the setfield_gc handling branch around
decode_descr_index so replay safety is evaluated only for the canonical
r<value>d shape. Reject unsupported /iid and /ird opcodes, returning
CalleeReplaySafety::Dirty before reading target or descriptor offsets, while
preserving the existing fresh-target and immutable-field checks for supported
forms.
- Around line 1346-1382: Update body_branch_targets to account for switch/id
case targets, which are stored in the instruction descriptor rather than an L
operand. Either decode and add every switch target to the returned set, or
return None for bodies containing switch/id so replay safety classifies them as
Dirty; preserve existing handling for other branch opcodes.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 1377-1389: In the starargs handling around
heapcache_getfield_cached, add the same concrete tuple-shape validation used by
the kwnames path before reading tuple_wrappeditems_descr from starargs. Reject
non-tuple values before the arity check and preserve the existing cached-length
validation for confirmed tuples.
- Around line 1302-1338: Update the keyword-call handling around the positional
parameter binding loop to reject callees with positional-only parameters, using
the available posonlyarg_count metadata before matching keyword names against
varnames. Preserve the existing positional and keyword duplicate checks for
supported signatures.
🪄 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: 4250a70d-2273-4f17-a2c1-0ed014181888
📒 Files selected for processing (4)
pyre/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.rs
Comment on lines
+1346
to
+1382
| /// Every pc in `body_code` that some op can branch to: the `goto` family and | ||
| /// `catch_exception` carry their target as the label operand, and | ||
| /// `int_*_jump_if_ovf` carries an overflow target ahead of its operands. | ||
| /// | ||
| /// `None` when an op carries a label this decode cannot locate — a var-list | ||
| /// or a pyre payload ahead of the `L` — since a missed target would let a | ||
| /// freshness claim survive a join it does not hold across. | ||
| fn body_branch_targets(body_code: &[u8]) -> Option<std::collections::HashSet<usize>> { | ||
| let mut targets = std::collections::HashSet::new(); | ||
| let mut pc = 0usize; | ||
| while pc < body_code.len() { | ||
| let d = crate::jitcode_runtime::decode_op_at(body_code, pc)?; | ||
| if d.argcodes.contains('L') { | ||
| // Operand widths follow `decode_op_at`; only the fixed-width forms | ||
| // can precede the label, so anything else gives up. | ||
| let mut cursor = d.pc + 1; | ||
| let mut target = None; | ||
| for operand in d.argcodes.chars() { | ||
| match operand { | ||
| 'L' => { | ||
| target = Some(u16::from_le_bytes([ | ||
| *body_code.get(cursor)?, | ||
| *body_code.get(cursor + 1)?, | ||
| ]) as usize); | ||
| break; | ||
| } | ||
| 'i' | 'c' | 'r' | 'f' => cursor += 1, | ||
| 'd' | 'j' => cursor += 2, | ||
| _ => break, | ||
| } | ||
| } | ||
| targets.insert(target?); | ||
| } | ||
| pc = d.next_pc; | ||
| } | ||
| Some(targets) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate every label/branch-bearing op key and its argcodes to confirm which
# control-flow ops carry no 'L' operand.
rg -nP --type=rust -C2 '"(goto|switch|catch_exception|.*jump_if_ovf)[^"]*/' pyre/ majit/ | head -80Repository: youknowone/pyre
Length of output: 7044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate file outline"
ast-grep outline pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs || true
echo
echo "## fbw_state relevant lines 1280-1415"
sed -n '1280,1415p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs | nl -ba -v1280
echo
echo "## jitcode_runtime candidates"
fd -p '.*jitcode_runtime.*|.*decode.*|.*op.*|.*insns.*' pyre/pyre-jit-trace/src | sed -n '1,120p'
echo
echo "## occurrences of switch/id / switch and control-flow opnames"
rg -n --type=rust -C2 '(^|/)"switch|switch/id|catch_exception|goto_if_not|jump_if_ovf|int_add_jump_if_ovf|decode_op_at|decode_descr_index' pyre/pyre-jit-trace | sed -n '1,240p'Repository: youknowone/pyre
Length of output: 5943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## fbw_state relevant lines 1340-1410"
sed -n '1340,1410p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
echo
echo "## jitcode_runtime files"
fd -a 'jitcode_runtime.*|mod.rs|decode.*|insns.*' pyre/pyre-jit-trace/src | sed -n '1,200p'
echo
echo "## decode_op_at and switch declarations/usages"
rg -n --type=rust -C3 'pub\s+fn\s+decode_op_at|fn\s+decode_op_at|decode_desc|r?decode_descr_index|switch/id|switch/|catch_exception|goto_if_not|jump_if_ovf|int_add_jump_if_ovf' pyre/pyre-jit-trace | sed -n '1,260p'Repository: youknowone/pyre
Length of output: 29427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## jitcode_runtime.rs relevant decode_op_at and switch"
rg -n -A120 'pub\s+fn\s+decode_op_at|fn\s+decode_op_at|impl\sOpDescr|struct\sOpDescr|switch/|switch/id|catch_exception/L|goto_if_not/iL|int_add_jump_if_ovf/L' pyre/pyre-jit-trace/src/jitcode_runtime.rs | sed -n '1,340p'
echo
echo "## fbw_callee_body_replay_safety scan 1410-1590"
sed -n '1410,1590p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
echo
echo "## switch exact declarations/usages in pyre-tree"
rg -n -C4 '"switch"|switch/id|switch/' pyre pyre-jit-trace majit 2>/dev/null | sed -n '1,240p' || trueRepository: youknowone/pyre
Length of output: 48481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## switch handler"
sed -n '7980,8115p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
echo
echo "## decode_descriptor helpers"
rg -n -A12 -B4 'decode_descr_index|read_descr|SwitchDictDescr|SwitchDescr|struct.*Switch|impl.*Switch' pyre/pyre-jit-trace/src/jitcode_dispatch pyre/pyre-jit-trace/src/jitcode_runtime.rs | sed -n '1,240p'
echo
echo "## all insns opname keys (if exposed) and switch count"
rg -n 'INSNS_OPNAMES|setup_insns|insns_opname_to_byte|pub static.*INSNS|map!\(|vec\!|switch/id|catch_exception/L|int_add_jump_if_ovf/Lii' pyre/pyre-jit-trace/src -g '*.rs' | sed -n '1,260p'Repository: youknowone/pyre
Length of output: 32158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## switch handler lines around it"
sed -n '7945,8025p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
echo
echo "## switch tests cases and descriptors"
sed -n '1360,1535p' pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
echo
echo "## PyreSwitchDescr and switch-target handling"
rg -n -C5 'PyreSwitchDescr|SwitchDictDescr|struct.*PyreSwitchDescr|as_switch|as_switch_dict|case.*target|dict\(|getint\(|orgpc|resumepc' pyre/pyre-jit-trace/src -g '*.rs' | sed -n '1,280p'
echo
echo "## label operand helper"
rg -n -A12 'fn read_label|label_operand_offset|fn decode_side_other_target' pyre/pyre-jit-trace/src/jitcode_dispatchRepository: youknowone/pyre
Length of output: 41866
Treat untracked switch targets as dirty replay safety.
body_branch_targets() uses 'L' operand positions to discover joins, but switch/id carries each case pc in its descr instead of an L operand. A body containing switch/id can keep fresh_ref_regs alive across a switch join and classify a live-heap store as Clean. Reject such bodies as Dirty, or decode switch targets from the descr if that is intended here.
🤖 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/fbw_state.rs` around lines 1346 -
1382, Update body_branch_targets to account for switch/id case targets, which
are stored in the instruction descriptor rather than an L operand. Either decode
and add every switch target to the returned set, or return None for bodies
containing switch/id so replay safety classifies them as Dirty; preserve
existing handling for other branch opcodes.
Comment on lines
1503
to
1516
| } else if d.opname.starts_with("setfield_gc") { | ||
| // Canonical setfield shapes are `r<value>d`: the target ref is | ||
| // operand 0 and the field descr is operand 2. | ||
| let Some(&target_reg) = body_code.get(d.pc + 1) else { | ||
| return false; | ||
| return CalleeReplaySafety::Dirty; | ||
| }; | ||
| let descr_index = decode_descr_index(body_code, &d, 2); | ||
| let immutable_field = callee_descr_refs | ||
| .get(descr_index) | ||
| .and_then(|descr| descr.as_field_descr()) | ||
| .is_some_and(|field| field.is_immutable()); | ||
| if !fresh_ref_regs[target_reg as usize] || !immutable_field { | ||
| return false; | ||
| return CalleeReplaySafety::Dirty; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate every setfield_gc op key/argcodes to confirm the assumed `r<value>d` shape is exhaustive.
rg -nP --type=rust '"setfield_gc[^"]*"' pyre/ majit/ | sort -uRepository: youknowone/pyre
Length of output: 6753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fbw_state outline =="
ast-grep outline pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs --view compact 2>/dev/null | head -120 || true
echo "== fbw_state relevant lines =="
sed -n '1460,1535p' pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
echo "== opcode width / decoding search =="
rg -n "fn decode_descr_index|decode_opcode|argcode|argcodes|width_map|BYTE_ARG|WORD_ARG|fieldwrite|setfield_gc" pyre/pyre-jit-trace/src pyre/pyre-jit-trace/src/jitcode_dispatch | head -250
echo "== module-level opcode table references =="
rg -n "BC_(SET|CALL|NEW|ARRAY|FIELD)" pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs | head -120Repository: youknowone/pyre
Length of output: 36828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all setfield_gc opname/argcodes from jitcode_runtime.rs and tests =="
python3 - <<'PY'
from pathlib import Path
import re
patterns = [
Path("pyre/pyre-jit-trace/src/jitcode_runtime.rs"),
Path("majit/majit-translate/src/codewriter/insns.rs"),
Path("majit/majit-metainterp/src/blackhole.rs"),
Path("pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs"),
]
seen=set()
for p in patterns:
if p.exists():
for i,line in enumerate(p.read_text(errors='replace').splitlines(),1):
if "setfield_gc" in line:
for m in re.findall(r'"([^"]*setfield_gc[^"]*)"', line):
seen.add((i,m,str(p)))
seen_sorted=sorted(seen, key=lambda x:x[1])
for row in seen_sorted:
print(f"{row[2]}:{row[0]}\t{row[1]}")
PY
echo "== decode_descr_index implementation =="
sed -n '3390,3435p' pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
echo "== decode_op_at decoding of argcodes around setfield =="
sed -n '850,930p' pyre/pyre-jit-trace/src/jitcode_runtime.rs
echo "== jit code opcode width chars =="
sed -n '660,750p' pyre/pyre-jit-trace/src/jitcode_runtime.rsRepository: youknowone/pyre
Length of output: 11097
Gate setfield_gc replay safety on the actual r<value>d shape.
decode_descr_index(.., 2) is applied to every setfield_gc* op, even unsupported /iid and /ird forms whose first operand is not a fresh ref. Those shapes would accept the wrong byte as the target register and can resolve the descriptr to a later field; guard this path by rejecting non-canonical setfield_gc argcodes or returning Dirty before trusting offsets.
🤖 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/fbw_state.rs` around lines 1503 -
1516, Update the setfield_gc handling branch around decode_descr_index so replay
safety is evaluated only for the canonical r<value>d shape. Reject unsupported
/iid and /ird opcodes, returning CalleeReplaySafety::Dirty before reading target
or descriptor offsets, while preserving the existing fresh-target and
immutable-field checks for supported forms.
Comment on lines
+1302
to
+1338
| let flags = unsafe { (*raw).flags }; | ||
| if flags.contains(pyre_interpreter::CodeFlags::VARARGS) | ||
| || flags.contains(pyre_interpreter::CodeFlags::VARKEYWORDS) | ||
| || unsafe { (*raw).kwonlyarg_count } != 0 | ||
| { | ||
| return None; | ||
| } | ||
| let varnames = unsafe { &(*raw).varnames }; | ||
| if varnames.len() < nparams { | ||
| return None; | ||
| } | ||
| let n_pos = nargs - nkw; | ||
| let mut slot_args: Vec<Option<OpRef>> = vec![None; nparams]; | ||
| let mut slot_conc: Vec<Option<ConcreteValue>> = vec![None; nparams]; | ||
| for k in 0..n_pos { | ||
| slot_args[k] = Some(args[k]); | ||
| slot_conc[k] = Some(arg_conc[k]); | ||
| } | ||
| for j in 0..nkw { | ||
| let name_obj = unsafe { pyre_object::w_tuple_getitem(kwnames, j as i64) }?; | ||
| if !unsafe { pyre_object::is_str(name_obj) } { | ||
| return None; | ||
| } | ||
| let name = unsafe { pyre_object::w_str_get_wtf8(name_obj) } | ||
| .as_str() | ||
| .ok()?; | ||
| let pi = varnames[..nparams] | ||
| .iter() | ||
| .position(|v| v.as_str() == name)?; | ||
| // A keyword may only bind a parameter past the positional fill, and each | ||
| // parameter at most once (else Python raises "multiple values for | ||
| // argument"). | ||
| if pi < n_pos || slot_args[pi].is_some() { | ||
| return None; | ||
| } | ||
| slot_args[pi] = Some(args[n_pos + j]); | ||
| slot_conc[pi] = Some(arg_conc[n_pos + j]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the CodeObject carries a positional-only arg count and how it relates to arg_count.
ast-grep run --pattern 'pub struct CodeObject { $$$ }' --lang rust pyre/pyre-interpreter/src \
|| rg -nP --type=rust -A40 'pub struct CodeObject' pyre/pyre-interpreter/src
rg -nP --type=rust '\bposonly\w*' pyre/ | head -30Repository: youknowone/pyre
Length of output: 3057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate inline_call.rs and relevant function =="
fd -a 'inline_call.rs' . | sed 's#^\./##'
echo
echo "== Section around target lines =="
cat -n pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | sed -n '1220,1360p'
echo
echo "== Resolve callee and arg_count usage =="
rg -n --type=rust 'resolver|fn fbw_reorder|arg_count|nparams|varnames' pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | sed -n '1,220p'
echo
echo "== CodeObject definition and posonlyarg_count relationship =="
rg -n --type=rust -A80 'pub struct CodeObject' pyre/pyre-interpreter/src/pycode.rs pyre/pyre-interpreter/src/lib.rs 2>/dev/null || true
rg -n --type=rust 'argcount.*posonly|posonly.*argcount|arg_count' pyre/pyre-interpreter/src/pycode.rs pyre/pyre-interpreter/src/call.rs | sed -n '1,120p'Repository: youknowone/pyre
Length of output: 14213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resolve_inlinable_callee definitions/usages =="
rg -n --type=rust 'fn resolve_inlinable_callee|resolve_inlinable_callee\(' pyre/pyre-jit-trace/src pyre/pyre-interpreter/src -A50 -B5
echo
echo "== pycode.rs CodeObject and argument construction slices =="
cat -n pyre/pyre-interpreter/src/pycode.rs | sed -n '600,660p'
cat -n pyre/pyre-interpreter/src/pycode.rs | sed -n '1080,1115p'
echo
echo "== call.rs call_kw posonly behavior =="
cat -n pyre/pyre-interpreter/src/call.rs | sed -n '1430,1495p'
cat -n pyre/pyre-interpreter/src/call.rs | sed -n '2030,2052p'
echo
echo "== behavioral model of current fbw_ behavior for positional-only kwargs =="
python3 - <<'PY'
def simulate(nparams, arg_count, posonly, kwargs):
# Mirrors fbw_reorder_call_kw_args:
nkw = len(kwargs)
nargs = nparams # caller only reaches this when nargs == nparams
n_pos = nparams - nkw
if nparams == 0 or nkw > nargs or nargs != nparams:
return "None"
varnames = [f"p{i}" for i in range(nparams)]
if len(varnames) < nparams:
return "None"
# positional-fill guard for kwargs: pi < n_pos rejects
slot_filled = [False] * nparams
for name, start_index in [(kwargs[0], min(n_pos, nparams - 1))]:
try:
pi = varnames[:nparams].index(name)
except ValueError:
return "None"
if pi < n_pos or slot_filled[pi]:
return "None"
slot_filled[pi] = True
# would inline normally; remaining positional slots from n_pos before key slots remain None?
# fold only succeeds below if all slots Some. With f(a=a) and nargs=nparams=1: n_pos=0, pi=0, pass, out=[arg] => inline success.
slots_some = all(slot_filled)
return "inline failure (missing args)" if not slots_some else f"inline success, slot={pi}"
for posonlyargcount in [0, 1]:
nparams = 1
code_argcount = posonlyargcount
print("posonlyargcount=", posonlyargcount, "code.arg_count=", code_argcount, "nparams=", nparams,
"with f(a=a)", simulate(nparams, code_argcount, pos_onlyargcount, ["p0"]))
PYRepository: youknowone/pyre
Length of output: 50371
Reject keyword calls when the callee has positional-only parameters.
nparams comes from code.arg_count, which includes positional-only slots, so varnames[..nparams] exposes those names. A keyword like f(a=2) can bind a positional-only parameter and the fold then succeeds, diverging from the interpreter’s TypeError. Either reject any callee with posonlyarg_count != 0 here, or reject any keyword name in the positional-only range.
🐛 Proposed fix
let flags = unsafe { (*raw).flags };
if flags.contains(pyre_interpreter::CodeFlags::VARARGS)
|| flags.contains(pyre_interpreter::CodeFlags::VARKEYWORDS)
|| unsafe { (*raw).kwonlyarg_count } != 0
+ // A keyword may not bind a positional-only parameter; `arg_count` includes
+ // those slots, so `varnames[..nparams]` would otherwise expose their names.
+ || unsafe { (*raw).posonlyarg_count } != 0
{
return None;
}📝 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.
Suggested change
| let flags = unsafe { (*raw).flags }; | |
| if flags.contains(pyre_interpreter::CodeFlags::VARARGS) | |
| || flags.contains(pyre_interpreter::CodeFlags::VARKEYWORDS) | |
| || unsafe { (*raw).kwonlyarg_count } != 0 | |
| { | |
| return None; | |
| } | |
| let varnames = unsafe { &(*raw).varnames }; | |
| if varnames.len() < nparams { | |
| return None; | |
| } | |
| let n_pos = nargs - nkw; | |
| let mut slot_args: Vec<Option<OpRef>> = vec![None; nparams]; | |
| let mut slot_conc: Vec<Option<ConcreteValue>> = vec![None; nparams]; | |
| for k in 0..n_pos { | |
| slot_args[k] = Some(args[k]); | |
| slot_conc[k] = Some(arg_conc[k]); | |
| } | |
| for j in 0..nkw { | |
| let name_obj = unsafe { pyre_object::w_tuple_getitem(kwnames, j as i64) }?; | |
| if !unsafe { pyre_object::is_str(name_obj) } { | |
| return None; | |
| } | |
| let name = unsafe { pyre_object::w_str_get_wtf8(name_obj) } | |
| .as_str() | |
| .ok()?; | |
| let pi = varnames[..nparams] | |
| .iter() | |
| .position(|v| v.as_str() == name)?; | |
| // A keyword may only bind a parameter past the positional fill, and each | |
| // parameter at most once (else Python raises "multiple values for | |
| // argument"). | |
| if pi < n_pos || slot_args[pi].is_some() { | |
| return None; | |
| } | |
| slot_args[pi] = Some(args[n_pos + j]); | |
| slot_conc[pi] = Some(arg_conc[n_pos + j]); | |
| let flags = unsafe { (*raw).flags }; | |
| if flags.contains(pyre_interpreter::CodeFlags::VARARGS) | |
| || flags.contains(pyre_interpreter::CodeFlags::VARKEYWORDS) | |
| || unsafe { (*raw).kwonlyarg_count } != 0 | |
| // A keyword may not bind a positional-only parameter; `arg_count` includes | |
| // those slots, so `varnames[..nparams]` would otherwise expose their names. | |
| || unsafe { (*raw).posonlyarg_count } != 0 | |
| { | |
| return None; | |
| } | |
| let varnames = unsafe { &(*raw).varnames }; | |
| if varnames.len() < nparams { | |
| return None; | |
| } | |
| let n_pos = nargs - nkw; | |
| let mut slot_args: Vec<Option<OpRef>> = vec![None; nparams]; | |
| let mut slot_conc: Vec<Option<ConcreteValue>> = vec![None; nparams]; | |
| for k in 0..n_pos { | |
| slot_args[k] = Some(args[k]); | |
| slot_conc[k] = Some(arg_conc[k]); | |
| } | |
| for j in 0..nkw { | |
| let name_obj = unsafe { pyre_object::w_tuple_getitem(kwnames, j as i64) }?; | |
| if !unsafe { pyre_object::is_str(name_obj) } { | |
| return None; | |
| } | |
| let name = unsafe { pyre_object::w_str_get_wtf8(name_obj) } | |
| .as_str() | |
| .ok()?; | |
| let pi = varnames[..nparams] | |
| .iter() | |
| .position(|v| v.as_str() == name)?; | |
| // A keyword may only bind a parameter past the positional fill, and each | |
| // parameter at most once (else Python raises "multiple values for | |
| // argument"). | |
| if pi < n_pos || slot_args[pi].is_some() { | |
| return None; | |
| } | |
| slot_args[pi] = Some(args[n_pos + j]); | |
| slot_conc[pi] = Some(arg_conc[n_pos + j]); |
🤖 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 1302 -
1338, Update the keyword-call handling around the positional parameter binding
loop to reject callees with positional-only parameters, using the available
posonlyarg_count metadata before matching keyword names against varnames.
Preserve the existing positional and keyword duplicate checks for supported
signatures.
Source: Coding guidelines
Comment on lines
+1377
to
+1389
| let starargs = r_args[2]; | ||
| let items_descr = crate::descr::tuple_wrappeditems_descr(); | ||
| let block = ctx | ||
| .trace_ctx | ||
| .heapcache_getfield_cached(starargs, items_descr.index())?; | ||
| // The cached length pins the arity: the callee takes exactly `nparams` | ||
| // positional parameters, and a mismatch is a runtime TypeError the inline | ||
| // path does not model. | ||
| let len_op = ctx.trace_ctx.heap_cache().arraylen(block)?; | ||
| match len_op.inline_const_to_value() { | ||
| Some(majit_ir::Value::Int(n)) if n as usize == nparams => {} | ||
| _ => return None, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No concrete type check that starargs is a tuple.
The keyword path validates is_tuple(kwnames) before trusting it, but this fold reads the W_Tuple.wrappeditems field straight off r_args[2] with no type check. tuple_wrappeditems_descr() resolves through field_descr_from_group → stable_field_index(offset, size, type, signed), so the descr index is structural: any other object whose slot 0 has the same offset/size/type (a W_List's storage, for instance) yields the same index and can hit a cached getfield entry. f(*some_list) is ordinary Python and would pass the arity check, at which point a list's strategy-specific backing store is unpacked as if it were tuple element refs. The walker_concrete_ref_object requirement per element is incidental cover, not a type guarantee.
Add the same concrete-shape gate the kwnames path uses.
🛡️ Proposed fix
let starargs = r_args[2];
+ // Structural field-descr identity is not type identity: pin the concrete
+ // receiver to an exact tuple before reading `wrappeditems` off it.
+ match arg_concretes[2] {
+ ConcreteValue::Ref(obj)
+ if !obj.is_null() && unsafe { pyre_object::is_tuple(obj) } => {}
+ _ => return None,
+ }
let items_descr = crate::descr::tuple_wrappeditems_descr();📝 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.
Suggested change
| let starargs = r_args[2]; | |
| let items_descr = crate::descr::tuple_wrappeditems_descr(); | |
| let block = ctx | |
| .trace_ctx | |
| .heapcache_getfield_cached(starargs, items_descr.index())?; | |
| // The cached length pins the arity: the callee takes exactly `nparams` | |
| // positional parameters, and a mismatch is a runtime TypeError the inline | |
| // path does not model. | |
| let len_op = ctx.trace_ctx.heap_cache().arraylen(block)?; | |
| match len_op.inline_const_to_value() { | |
| Some(majit_ir::Value::Int(n)) if n as usize == nparams => {} | |
| _ => return None, | |
| } | |
| let starargs = r_args[2]; | |
| // Structural field-descr identity is not type identity: pin the concrete | |
| // receiver to an exact tuple before reading `wrappeditems` off it. | |
| match arg_concretes[2] { | |
| ConcreteValue::Ref(obj) | |
| if !obj.is_null() && unsafe { pyre_object::is_tuple(obj) } => {} | |
| _ => return None, | |
| } | |
| let items_descr = crate::descr::tuple_wrappeditems_descr(); | |
| let block = ctx | |
| .trace_ctx | |
| .heapcache_getfield_cached(starargs, items_descr.index())?; | |
| // The cached length pins the arity: the callee takes exactly `nparams` | |
| // positional parameters, and a mismatch is a runtime TypeError the inline | |
| // path does not model. | |
| let len_op = ctx.trace_ctx.heap_cache().arraylen(block)?; | |
| match len_op.inline_const_to_value() { | |
| Some(majit_ir::Value::Int(n)) if n as usize == nparams => {} | |
| _ => return None, | |
| } |
🤖 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 1377 -
1389, In the starargs handling around heapcache_getfield_cached, add the same
concrete tuple-shape validation used by the kwnames path before reading
tuple_wrappeditems_descr from starargs. Reject non-tuple values before the arity
check and preserve the existing cached-length validation for confirmed tuples.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8ec7b31). 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)None. 4. Structural adaptations
|
This was referenced Jul 25, 2026
This was referenced Aug 6, 2026
builtins: layout-checked object.__new__, buffer request kinds, and six argument-handling sites
#1063
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four commits on the FBW inline-at-residual lever. Each one removes a decline that kept a Python-level call in a
forbody as a residual.8cd58ae18d— inline keyword callstry_walker_inline_user_callreturnedNonefor anyCallKwresidual, so a keyword call was never inlined.fbw_reorder_call_kw_argsfolds thekwnamestuple into a parameter permutation at trace time and hands the positional inline path the reordered boxes. Callees carryingVARARGS/VARKEYWORDS/ keyword-only params decline.d9ac64ebc7— admit deferred-call callees into FOR_ITER-body inliningAn inline sub-walk inside a FOR_ITER body resumes a guard at the caller's CALL boundary, so a deopt re-executes the whole callee; the gate therefore declined any body it could not prove write-free. The static scan rejected on
LOAD_GLOBALandbox_int— not on the nested CALL, which was the obvious-but-wrong hypothesis.effectinfo.rsdocuments thatload_const/load_global/box_intcarryCanRaisepurely to satisfy the_OS_CANRAISEinvariant; each is a read or a fresh allocation.fbw_callee_body_side_effect_freebecomesfbw_callee_body_replay_safetyreturningClean/DeferredCall/Dirty:LoadConst | LoadGlobal | BoxIntjoin the provably-side-effect-free set.DeferredCall— its callee is a runtime value, so replay safety is decidable only at the call.fbw_abort_nested_unjournaled_residualaborts before executing a residual that did not inline underneath a deferred-admitted sub-walk, and denies that callee for the rest of the process.Soundness: the static scan clears every direct heap write, and the backstop clears every impure residual, so a deferred sub-walk is write-free everywhere it can abort or deopt. Denial is per callee code key, so a callee that turns out non-inlinable costs exactly one abort and then declines statically.
for i in range(N): t += helper(i),helper(i){return add(i,1,2)}add(i, c=2, b=1)whileThe
for-vs-whileasymmetry is gone. The residual gap to pypy is the known backend-regalloc gap, out of scope here.732f1193bd— virtualize array-backed BUILD_TUPLE, inlinef(*args)Same shape of surprise: the scan rejected at BUILD_TUPLE's
setarrayitem_gc, before ever reaching theCallFunctionExresidual.try_walker_specialize_newtuple_objectre-emits the canonicalW_TupleObjectshape for the aritiesmakespecialisedtuple2does not claim, reading elements from the array heap-cache so thenew_array_clearbuild keeps no consumer. Arity 2 declines:w_tuple_newrouteslen == 2toCls_ii/Cls_ff/Cls_oo, which has nowrappeditems, so emitting the array-backed shape there would diverge from what the blackhole rebuilds.new_array*results as fresh, acceptssetarrayitem_gcinto a fresh array as an initialization, and addsNewtupleFromArray/NewlistFromArrayandCallFunctionExto the respective sets.fbw_unpack_call_function_ex_argsbinds a star tuple to positional arguments from the cached element boxes, so it folds exactly when the tuple is virtual at the call. Declines a**merge, an arity that is not the callee'sco_argcount, the method form, and a tuple with no cached backing block.synth/call_function_ex_star: 0.89s → 0.14s user,loops_aborted=0.8ec7b3150e— freshness drops at branch targets, not branch sitesThe freshness set was cleared when the linear scan reached a
goto, which leaves a join hole: inr1 = getfield_gc(...); goto L; r1 = new_array; L: setarrayitem_gc r1the scan readsr1as fresh atLalthough one path arrives holding a live-heap reference. Now every label operand is collected up front and cleared at the target pc, which is both sound and strictly more precise (a conditional goto's fall-through keeps its exact state).No current producer reaches the hole —
setarrayitem_gcis emitted only by the BUILD_TUPLE / BUILD_LIST lowerings, whose target register is defined in the same basic block, and a three-case probe does not discriminate the old scan from the new. This is invariant hardening, not a bug fix, and it measured neutral on four benches.Verification
check.pydynasm 308/308 + cranelift 308/308. A later full run showednested_loopat 2.1x against its 2x gate; discriminated as a load flake three ways —jit-statsbyte-identical across the pre-change, post-change and current binaries (loops_compiled=2 bridges_compiled=1 loops_aborted=0 guard_failures=202), interleaved timings overlapping with no separation, and an isolated re-run passing 14/14 at 1.6x. Fivecheck.pyruns were concurrent on the machine at the time.TypeErrortext, defaults /*args/**kwargs/ keyword-only callees, list unpack, parameter-derived tuples, side-effecting and polymorphic callees, tuple unpack / escape /is/==/repr, non-range FOR_ITER shapes (list iter, generator, user__next__, zip, break, continue, mid-loop type-switch deopt).Note
PYRE_FBW_FORITER_INLINEno longer exists — #757 retired it — so there is no env-gate rollback on this path. The pre-change behaviour isClean-only admission.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Performance