Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion majit/majit-backend-wasm/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,9 @@ pub fn build_wasm_module(
// home-slot writes overflow into the next arena slot.
let bridge_finish_fi = guards
.iter()
.find(|g| g.is_finish)
.find(|g| {
g.is_finish && !crate::failguard::meta_descr_is_exit_frame_with_exception(&g.meta_descr)
})
.map(|g| g.fail_index)
.unwrap_or(0);
// CA frames execute the source loop and this bridge on the same frozen
Expand Down
15 changes: 15 additions & 0 deletions majit/majit-backend-wasm/src/failguard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,21 @@ pub struct WasmFailDescr {
pub meta_descr: Option<DescrRef>,
}

/// `compile.py:658-662` ExitFrameWithExceptionDescrRef parity: whether a FINISH
/// exit is an ExitFrameWithException (the callee raised; slot 0 holds the
/// exception) rather than a DoneWithThisFrame. `is_finish` alone is true for
/// both, so the self-recursive CALL_ASSEMBLER arm must exclude the exception
/// variant when it picks the "clean callee finish" `fail_index` — an
/// ExitFrameWithException must route to `wasm_ca_resume_deopt`, which propagates
/// the exception, not be short-circuited to its output slot.
pub fn meta_descr_is_exit_frame_with_exception(meta_descr: &Option<DescrRef>) -> bool {
meta_descr
.as_ref()
.and_then(|d| d.as_fail_descr())
.map(|fd| fd.is_exit_frame_with_exception())
.unwrap_or(false)
}

impl Descr for WasmFailDescr {
fn index(&self) -> u32 {
self.fail_index
Expand Down
5 changes: 4 additions & 1 deletion majit/majit-backend-wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1783,7 +1783,10 @@ impl majit_backend::Backend for WasmBackend {
.fail_descrs
.borrow()
.iter()
.find(|descr| descr.is_finish)
.find(|descr| {
descr.is_finish
&& !failguard::meta_descr_is_exit_frame_with_exception(&descr.meta_descr)
})
.map(|descr| descr.fail_index)
.unwrap_or(0);
Comment on lines +1787 to 1791

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the unknown sentinel when no clean CA finish exists

When the only FINISH on a compiled CALL_ASSEMBLER target is ExitFrameWithException (or there is no DoneWithThisFrame), this new filter leaves no clean match but the following .unwrap_or(0) still publishes fail index 0 as the clean finish. The wasm CA arm treats fi == loop_finish_fi as a direct return from F'[1], so an exception finish or guard assigned fail_index 0 bypasses wasm_ca_resume_deopt and is still misrouted; use the existing WASM_CA_FINISH_FI_UNKNOWN sentinel here, and similarly for the bridge finish selector, when no clean finish exists.

Useful? React with 👍 / 👎.

// For a pending self target this is the exact map already embedded in
Expand Down
4 changes: 4 additions & 0 deletions majit/majit-ir/src/effectinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,10 @@ pub enum PyreHelperKind {
/// the in-flight iteration to the live frame instead of dropping it (the
/// iterator advance is an irreversible side effect with no journal undo).
ForIterNext,
/// `get_iter(obj)` — the GET_ITER residual (`iter(obj)`). The full-body
/// walker recognises exact machine-word `range` objects and emits the
/// virtual `W_IntRangeIterator` allocation shape directly.
GetIter,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Append GetIter at the enum tail

PyreHelperKind is repr(u8), and the enum's tail comment explicitly warns that inserting helpers in the middle changes discriminants consumed by serialized/stable helper metadata. Placing GetIter before StoreDeref shifts the numeric tags for StoreDeref, ListAppendValue, CallFunctionEx, and later helpers; any reused serialized JitCode/descriptor metadata generated with the old tags can then be decoded as the wrong helper, causing the walker to miss body-effect handling or run an unrelated specialization. Add the new helper at the tail or pin explicit discriminants so existing tag values stay stable.

Useful? React with 👍 / 👎.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
/// `store_deref_value(cell, value)` — the STORE_DEREF residual
/// (`bh_store_deref_value_fn` via `cpu.store_deref_value_fn`). It mutates
/// the cell's contents in place and RETURNS the slot value (`Ref`), so it
Expand Down
23 changes: 18 additions & 5 deletions majit/majit-metainterp/src/optimizeopt/unroll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4819,11 +4819,24 @@ fn assemble_peeled_trace_with_jump_args(

// Label position
let label_pos = next_free_pos(max_pos);
let mut full_label_args: Vec<OpRef> = label_args
.iter()
.copied()
.filter(|arg| !is_trace_constant_ref(*arg, constants))
.collect();
// compile.py:327-328 splices `label_op` verbatim: the label arg list produced
// by import_state (make_inputargs) IS the loop-header contract, and the jump
// args (assemble_jump below) enumerate the SAME VirtualState positionally, so
// the two must stay arity-aligned. Constant virtual-state slots were already
// dropped at enum time (virtualstate.rs NotVirtual `position_in_notvirtuals`
// only assigned to non-LEVEL_CONSTANT leaves), so every arg present here is a
// live carried position. Do NOT re-derive the slot set from the post-hoc
// backend-constants map: a phase-2 guard postprocess may const-forward a
// still-live loop-carried box (e.g. the exhaust-guard's `remaining` proving
// `<=0 ∧ >=0 ⇒ ==0`), and dropping that label slot while the jump keeps
// rebinding it desyncs the label/jump contract and orphans the head guard's
// operand. RPython performs no such filter.
let mut full_label_args: Vec<OpRef> = label_args.iter().copied().collect();
debug_assert!(
full_label_args.iter().all(|arg| !arg.is_constant()),
"base label arg is an inline-Const OpRef; LEVEL_CONSTANT virtual-state \
slots must be dropped at make_inputargs enum time, not at assembly"
);

// Collect preamble-defined OpRefs BEFORE adding extra label args,
// so we can filter out virtual remnants (removed New ops).
Expand Down
55 changes: 55 additions & 0 deletions pyre/pyre-jit-trace/src/descr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,45 @@ static RANGE_ITER_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(||
)
});

static RANGE_DESCR_GROUP: LazyLock<PyreObjectDescrGroup> = LazyLock::new(|| {
build_object_descr_group_with_def_path(
std::mem::size_of::<W_Range>(),
pyre_object::functional::W_RANGE_GC_TYPE_ID,
&pyre_object::functional::RANGE_TYPE as *const _ as usize,
&[
(
"W_Range.start",
RANGE_START_OFFSET,
8,
Type::Ref,
false,
true,
false,
),
(
"W_Range.step",
RANGE_STEP_OFFSET,
Comment on lines +920 to +922

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Include W_Range.stop in the descriptor group

When this new W_Range SizeDescr is used, the field list jumps from start to step, but W_Range also has a stop: PyObjectRef field (pyre/pyre-object/src/functional.rs:685-689). build_object_descr_group_with_def_path derives gc_fielddescrs() from only the supplied Type::Ref fields and publishes this SizeDescr under W_RANGE_GC_TYPE_ID, so traced/materialized range objects described by it will not report the stop edge for GC clearing/scanning or field-descr lookup. Add a RANGE_STOP_OFFSET entry between start and step so the runtime descriptor matches the actual object layout.

Useful? React with 👍 / 👎.

8,
Type::Ref,
false,
true,
false,
),
(
"W_Range.length",
RANGE_LENGTH_OFFSET,
8,
Type::Ref,
false,
true,
false,
),
],
"W_Range",
"functional::W_Range",
)
});

Comment on lines +905 to +943

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n 'field_size|PyreFieldDescr|wasm32|wasm' pyre majit

Repository: youknowone/pyre

Length of output: 50372


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the W_Range descriptor and nearby type definitions.
sed -n '850,980p' pyre/pyre-jit-trace/src/descr.rs

printf '\n---\n'

# Find the W_Range struct definition and PyObjectRef alias.
rg -n 'struct W_Range|type PyObjectRef|pub struct PyObjectRef|enum PyObjectRef' pyre/pyre-interpreter/src pyre/pyre-jit-trace/src

printf '\n---\n'

# Inspect how field_size is interpreted in the JIT/runtime.
sed -n '9738,9845p' pyre/pyre-jit-trace/src/state.rs

printf '\n---\n'

# Inspect any explicit pointer-size handling around descriptor field sizes.
rg -n 'size_of::<PyObjectRef>|pointer_size|field_size: std::mem::size_of::<usize>|field_size: 8' pyre/pyre-jit-trace/src pyre/pyre-interpreter/src

Repository: youknowone/pyre

Length of output: 10762


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Narrower probe: look for W_Range fields and any target-layout annotations.
ast-grep outline pyre/pyre-interpreter/src pyre/pyre-jit-trace/src --match W_Range

Repository: youknowone/pyre

Length of output: 167


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Focused search for the exact descriptor group and related comments.
rg -n -A6 -B6 'RANGE_DESCR_GROUP|W_Range\.start|W_Range\.step|W_Range\.length' pyre/pyre-jit-trace/src/descr.rs

Repository: youknowone/pyre

Length of output: 2360


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the PyObjectRef definition and any target-width assumptions.
rg -n -A4 -B4 'pub type PyObjectRef|struct PyObjectRef|enum PyObjectRef|type PyObjectRef' pyre

printf '\n---\n'

# Inspect the interpreter-side type alias/definition if found.
for f in $(rg -l 'pub type PyObjectRef|struct PyObjectRef|enum PyObjectRef|type PyObjectRef' pyre); do
  echo "FILE: $f"
  sed -n '1,120p' "$f" | rg -n 'PyObjectRef|PyObject|usize|u64|u32|NonZero|Tagged'
  echo
done

printf '\n---\n'

# Check the descriptor API for any notes on Ref field sizing.
sed -n '1,140p' pyre/pyre-jit-trace/src/descr.rs

Repository: youknowone/pyre

Length of output: 9102


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect references to the exact field-size convention for Ref fields.
rg -n -A3 -B3 'Type::Ref|field_size.*Ref|wrapped PyObjectRef|pointer_size\(\)|size_of::<PyObjectRef>' pyre/pyre-jit-trace/src pyre/pyre-interpreter/src pyre/pyre-object/src

Repository: youknowone/pyre

Length of output: 50372


Use pointer width for W_Range ref fields.
PyObjectRef is pointer-sized, so hard-coding 8 makes this descriptor wrong on wasm32 and can corrupt field/GC handling. Use std::mem::size_of::<PyObjectRef>() for start, step, and length.

🤖 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 905 - 943, Update
RANGE_DESCR_GROUP’s W_Range.start, W_Range.step, and W_Range.length field
descriptors to use std::mem::size_of::<PyObjectRef>() instead of the hard-coded
width 8, preserving the existing Type::Ref and descriptor metadata.

/// `Method` field layout — `w_function`, `w_self`, `w_class` per
/// `function.rs:9-15`. All three are Ref slots; the JIT only consumes
/// `w_function` (for guarding which method) and `w_self` (for recovering
Expand Down Expand Up @@ -1689,6 +1728,7 @@ pub fn make_array_descr_with_full_id(
use pyre_object::floatobject::{FLOAT_FLOATVAL_OFFSET, W_FloatObject};
use pyre_object::functional::{
RANGE_ITER_CURRENT_OFFSET, RANGE_ITER_REMAINING_OFFSET, RANGE_ITER_STEP_OFFSET,
RANGE_LENGTH_OFFSET, RANGE_START_OFFSET, RANGE_STEP_OFFSET, W_Range,
};
use pyre_object::interp_exceptions::{
EXC_ARGS_W_OFFSET, EXC_KIND_COUNT, EXC_KIND_OFFSET, EXC_W_ATTR_OBJ_OFFSET, EXC_W_CAUSE_OFFSET,
Expand Down Expand Up @@ -1763,6 +1803,21 @@ pub fn range_iter_step_descr() -> DescrRef {
field_descr_from_group(&RANGE_ITER_DESCR_GROUP, 2)
}

/// Field descriptor for `W_Range.start` (wrapped PyObjectRef).
pub fn range_start_descr() -> DescrRef {
field_descr_from_group(&RANGE_DESCR_GROUP, 0)
}

/// Field descriptor for `W_Range.step` (wrapped PyObjectRef).
pub fn range_step_descr() -> DescrRef {
field_descr_from_group(&RANGE_DESCR_GROUP, 1)
}

/// Field descriptor for `W_Range.length` (wrapped PyObjectRef).
pub fn range_length_descr() -> DescrRef {
field_descr_from_group(&RANGE_DESCR_GROUP, 2)
}

/// `Method.w_function` — the underlying function (`Function` or
/// `BuiltinFunction`) bound by `getattr(obj, name)`. Marked immutable
/// per `pypy/interpreter/function.py:567` `_Method._immutable_fields_`,
Expand Down
11 changes: 11 additions & 0 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,15 @@ pub fn dispatch_via_miframe<Sym: WalkSym>(
pyre_object::PY_NULL
},
class_of_last_exc_is_const: sym.class_of_last_exc_is_const(),
// A guard-failure bridge resumes at the opcode boundary, so
// its first `jit_merge_point` crossing at this python-pc is
// the same op it is resuming INTO, not a loop crossing. The
// merge-point arm skips exactly that first crossing. Seeded
// only for bridge walks; a loop compile leaves it `None`.
bridge_entry_merge_pc: match (trace_ctx.is_bridge_trace, entry_py_pc) {
(true, EntryPyPc::Py(pc)) => Some(pc as usize),
_ => None,
},
..Default::default()
},
session,
Expand Down Expand Up @@ -710,6 +719,7 @@ pub(crate) fn drive_bridge_frame_subwalk<Sym: WalkSym>(
.then_some(root_sym.last_exc_box()),
current_exception_seed_concrete: root_sym.last_exc_value(),
class_of_last_exc_is_const: root_sym.class_of_last_exc_is_const(),
..Default::default()
},
session,
registers_r: &mut regs_r,
Expand Down Expand Up @@ -1056,6 +1066,7 @@ pub(crate) fn drive_outer_frame_continuation<Sym: WalkSym>(
.then_some(root_sym.last_exc_box()),
current_exception_seed_concrete: root_sym.last_exc_value(),
class_of_last_exc_is_const: root_sym.class_of_last_exc_is_const(),
..Default::default()
},
session,
registers_r: &mut regs_r,
Expand Down
32 changes: 32 additions & 0 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,21 @@ pub struct FbwWalkMode<Sym: WalkSym> {
/// This is shared logically across recursive MIFrame walks; catch routing
/// writes the proven-class state back into the caller's copy.
pub class_of_last_exc_is_const: bool,
/// Python-pc of a guard-failure bridge walk's own resume coordinate.
/// `Some` only on the top-level walk of a bridge trace, `None` otherwise
/// (loop compiles and sub-walks).
///
/// `generate_guard(resumepc=orgpc)` (`pyjitpl.py:2610-2626`) places a
/// guard's resume coordinate INSIDE the guarded opcode's implementation,
/// strictly past the dispatch-top `jit_merge_point`, so an RPython MIFrame
/// resumed from a guard never re-crosses the loop-header merge point at
/// position zero. Pyre's bridge walker instead resumes at the opcode
/// BOUNDARY and would re-cross the header immediately with an empty body,
/// closing a 0-progress no-op bridge. The merge-point arm consumes this
/// (via `take()`) to skip exactly the first crossing that lands on the
/// bridge's own resume coordinate, restoring the RPython positional
/// semantics.
pub bridge_entry_merge_pc: Option<usize>,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

impl<Sym: WalkSym> Clone for FbwWalkMode<Sym> {
Expand Down Expand Up @@ -567,6 +582,7 @@ impl<Sym: WalkSym> Default for FbwWalkMode<Sym> {
current_exception_seed: None,
current_exception_seed_concrete: pyre_object::PY_NULL,
class_of_last_exc_is_const: false,
bridge_entry_merge_pc: None,
}
}
}
Expand Down Expand Up @@ -8222,6 +8238,22 @@ fn handle<Sym: WalkSym>(
// jdindex is the op's leading `c` byte (pyjitpl.py:1537
// `jdindex = ord(self.jitcode.code[orgpc+1])`).
let jdindex = code[op.pc + 1] as i8 as usize;
// pyjitpl.py:2610-2626: a guard's resume coordinate
// (`resumepc=orgpc`) lies INSIDE the guarded opcode's
// implementation, past the dispatch-top `jit_merge_point`, so an
// RPython MIFrame resumed from a guard never re-crosses the
// loop-header merge point at position zero. The walker resumes a
// bridge at the opcode BOUNDARY, so its first crossing at the
// resume coordinate is the same op it is resuming INTO — not a
// loop crossing. Skip exactly once. `take()` clears on the first
// crossing regardless of pc, so a mid-body-resume bridge whose
// first crossing is a DIFFERENT header is unaffected.
if ctx.is_top_level
&& ctx.trace_ctx.seen_loop_header_for_jdindex < 0
&& ctx.fbw_mode.bridge_entry_merge_pc.take() == Some(next_instr)
{
return Ok((DispatchOutcome::Continue, op.next_pc));
}
if ctx.trace_ctx.seen_loop_header_for_jdindex < 0 {
// pyjitpl.py:1548 `if not any_operation: return`.
if ctx.trace_ctx.num_ops() == 0 {
Expand Down
12 changes: 12 additions & 0 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1891,6 +1891,18 @@ pub(crate) fn dispatch_residual_call_iRd_kind<Sym: WalkSym>(
}
}

// Range GET_ITER: virtualize exact machine-word `range` into the same
// `W_IntRangeIterator` shape PyPy's inlined `descr_iter` would trace.
if ctx.is_authoritative_executor
&& ctx.is_full_body_walk
&& ei.pyre_helper == majit_ir::PyreHelperKind::GetIter
{
if let Some(iter_op) = try_walker_specialize_get_iter(ctx, op.pc, &r_args, dst, dst_bank)? {
write_residual_call_result_to_dst(ctx, op.pc, dst, dst_bank, iter_op)?;
return Ok((DispatchOutcome::Continue, op.next_pc));
}
}

// Range FOR_ITER is a C-level iterator advance. Re-emit its field
// updates so the opaque ForIterNext residual cannot invalidate optheap;
// other iterator families retain the residual and its Python semantics.
Expand Down
Loading
Loading