Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
28 changes: 27 additions & 1 deletion majit/majit-metainterp/src/blackhole.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5190,14 +5190,40 @@ fn bhimpl_jit_leave_portal_frame() {}
/// blackhole.py:1547-1548 `bhimpl_hint_force_virtualizable(r): pass`.
fn bhimpl_hint_force_virtualizable(_r: i64) {}

/// Called at every `-live-` marker, i.e. once per source-level instruction the
/// blackhole replays. Arguments are the interpreter and the marker's own
/// bytecode position.
///
/// RPython's `bhimpl_live` is a plain no-op, and it can be: the frame fields
/// its interpreter writes per instruction (`dispatch_bytecode`'s
/// `self.last_instr = intmask(next_instr)`) are ordinary source-level stores,
/// so they are compiled into the jitcode and the blackhole replays them like
/// any other operation. A consumer whose jitcode cannot carry such a store —
/// because the value is a distinct compile-time constant per instruction and
/// `check_result`'s 256-entry per-kind cap rejects one pool entry per
/// instruction — registers this hook and writes the field itself.
pub type LiveMarkerHook = fn(&BlackholeInterpreter, usize);

static LIVE_MARKER_HOOK: std::sync::OnceLock<LiveMarkerHook> = std::sync::OnceLock::new();

/// Install the [`LiveMarkerHook`]. First registration wins; later calls are
/// ignored, so a consumer may call it from every driver install path.
pub fn register_live_marker_hook(hook: LiveMarkerHook) {
let _ = LIVE_MARKER_HOOK.set(hook);
}

/// Handler for `live/` — liveness marker. Argcodes: empty, but the assembler
/// emits a 2-byte offset after the opcode. Skip those 2 bytes.
/// RPython blackhole.py:146-158 (inside _get_method for `-live-` ops).
fn handler_live(
_bh: &mut BlackholeInterpreter,
bh: &mut BlackholeInterpreter,
_code: &[u8],
position: usize,
) -> Result<usize, DispatchError> {
if let Some(hook) = LIVE_MARKER_HOOK.get() {
// `position` is past the opcode byte; the marker op starts one earlier.
hook(bh, position - 1);
}
// Skip the 2-byte liveness offset (RPython: OFFSET_SIZE = 2).
Ok(position + 2)
}
Expand Down
7 changes: 0 additions & 7 deletions majit/majit-metainterp/src/optimizeopt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,11 +581,6 @@ pub struct OptContext {
/// rewrite.py:282: postprocess_GUARD_NONNULL → mark_last_guard.
/// Deferred until emit adds the guard to new_operations.
pub(crate) pending_mark_last_guard: Option<OpRef>,
/// virtualize.py:84-90 postprocess_FINISH queues the stashed
/// GUARD_NOT_FORCED_2 here so the outer optimizer can insert it at
/// `len(_newoperations) - 1` with full `store_final_boxes_in_guard`
/// semantics.
pub(crate) pending_finish_guard_postprocess: Option<Op>,
// ptr_info merged into forwarded (Forwarded::Info variant)
//
// RPython parity: per-OpRef IntBound storage lives ENTIRELY on
Expand Down Expand Up @@ -1664,7 +1659,6 @@ impl OptContext {
extra_operations_after: VecDeque::new(),
pending_guard_class_postprocess: None,
pending_mark_last_guard: None,
pending_finish_guard_postprocess: None,
imported_short_pure_ops: Vec::new(),
imported_virtual_args: None,
imported_loop_invariant_results: Vec::new(),
Expand Down Expand Up @@ -2270,7 +2264,6 @@ impl OptContext {
extra_operations_after: VecDeque::new(),
pending_guard_class_postprocess: None,
pending_mark_last_guard: None,
pending_finish_guard_postprocess: None,
imported_short_pure_ops: Vec::new(),
imported_virtual_args: None,
imported_loop_invariant_results: Vec::new(),
Expand Down
70 changes: 36 additions & 34 deletions majit/majit-metainterp/src/optimizeopt/virtualize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2084,7 +2084,7 @@ impl Optimization for OptVirtualize {
OptimizationResult::PassOn
}

// virtualize.py:80-90 optimize_FINISH / postprocess_FINISH
// virtualize.py optimize_FINISH / postprocess_FINISH
//
// def optimize_FINISH(self, op):
// self._finish_guard_op = self._last_guard_not_forced_2
Expand All @@ -2099,25 +2099,44 @@ impl Optimization for OptVirtualize {
// assert i >= 0
// self.optimizer._newoperations.insert(i, guard_op)
//
// majit ordering: emit_extra queues the stashed guard for the
// passes after virtualize, and `drain_extra_operations_from`
// (called by propagate_from_pass_range right after this method
// returns) flushes those queued ops through the pipeline before
// the FINISH replacement is propagated. The guard therefore lands
// in `new_operations` first, the FINISH lands second — matching
// RPython's "insert at len-1" final layout. The guard's resume
// data is finalized when `emit_guard_operation` calls
// `store_final_boxes_in_guard` during its emission.
// majit ordering: upstream INSERTS because its postprocess runs
// after the FINISH is already appended. This pass runs BEFORE the
// FINISH reaches the terminal emit, so `emit_extra` queues the
// stashed guard for the passes after virtualize and
// `drain_extra_operations_from` (called right after this method
// returns) flushes it through the pipeline first. The guard lands
// in `new_operations` first, the FINISH second — the same final
// op order.
//
// RPython parity: optimize_FINISH does NOT call the generic
// escaping-op force path here. Forcing the FINISH args in the
// virtualize pass would happen before the stashed
// GUARD_NOT_FORCED_2 is reinserted, and store_final_boxes_in_guard
// would then see the already-forced return box in vable_array.
// The actual arg forcing belongs later in Optimizer._emit_operation,
// after the queued guard has been flushed ahead of FINISH.
// The RESUME DATA is where the two diverge. Upstream finalizes the
// guard in `postprocess_FINISH`, i.e. after `emit(op)` forced the
// FINISH args, so `store_final_boxes_in_guard` sees a return box
// that was virtual as already materialized. Here the guard is
// finalized on the way through the pipeline, before that forcing,
// and encodes the same box as still virtual. Both are consistent
// images, but they are not the same image.
//
// BLOCKER for the faithful order. `propagate_postprocess` (the
// port of optimizer.py's postprocess dispatch) is a method on a
// PASS, and the finalization a guard needs is
// `Optimizer::store_final_boxes_in_guard` with the knowledge
// `collect_optimizer_knowledge_for_resume(&self)` gathers — which
// needs the Optimizer, not a pass. Running it from here with no
// knowledge would drop the bridgeopt sections that
// `serialize_optimizer_knowledge` puts in every other guard, buying
// one ordering divergence with a worse one. Reaching upstream's
// shape needs an Optimizer-side FINISH postprocess that can insert
// at `new_operations.len() - 1` after its own emit.
//
// Nothing arms the token today — the portal-return
// `gen_store_back_in_vable` sets `forced_virtualizable`, so
// `store_token_in_vable` early-returns and no `GUARD_NOT_FORCED_2`
// reaches a FINISH — so neither image is currently observable.
OpCode::Finish => {
self.finish_guard_op = self.last_guard_not_forced_2.take();
if let Some(guard_op) = self.finish_guard_op.clone() {
ctx.emit_extra(ctx.current_pass_idx, guard_op);
Comment on lines +2137 to +2138

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 Preserve FINISH reinsertion in postprocess_FINISH

When FINISH follows a stashed GUARD_NOT_FORCED_2, this queues the guard during optimize_FINISH, while the same change deletes OptVirtualize's postprocess callback. This is not structurally equivalent to upstream: the guard now runs through every pass after virtualize before FINISH is emitted, whereas postprocess_FINISH finalizes its resume boxes and inserts it directly at len(_newoperations) - 1 after FINISH emission. Restore the literal postprocess method rather than changing its timing; the repository rules expressly prohibit deleting an upstream method in favor of a shortcut rewrite.

AGENTS.md reference: AGENTS.md:L141-L146

Useful? React with 👍 / 👎.

}
Comment on lines +2102 to +2139

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Restore Optimizer-side FINISH postprocessing rather than finalizing the guard early.

ctx.emit_extra sends GUARD_NOT_FORCED_2 through the pipeline before terminal FINISH emission forces its arguments. As the comment notes, this captures a different resume-data image from upstream. Implement the Optimizer-side postprocess that finalizes the guard after FINISH argument forcing, then inserts it immediately before FINISH.

As per coding guidelines, “When porting RPython/PyPy, maintain strict line-by-line structural parity; do not shortcut, reimplement from scratch, or declare a phase complete without the literal refactor.”

🤖 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-metainterp/src/optimizeopt/virtualize.rs` around lines 2102 -
2139, Replace the early guard dispatch in the Finish branch of the pass method
with Optimizer-side FINISH postprocessing. After FINISH emission has forced its
arguments, use Optimizer::store_final_boxes_in_guard with
collect_optimizer_knowledge_for_resume and insert the finalized
GUARD_NOT_FORCED_2 immediately before FINISH, preserving upstream
postprocess_FINISH ordering and bridgeopt knowledge.

Source: Coding guidelines

OptimizationResult::PassOn
}

Expand Down Expand Up @@ -2272,23 +2291,6 @@ impl Optimization for OptVirtualize {
self.finish_guard_op = None;
}

fn have_postprocess_op(&self, opcode: OpCode) -> bool {
matches!(opcode, OpCode::Finish)
}

fn propagate_postprocess(&mut self, op: &Op, ctx: &mut OptContext) {
if op.opcode != OpCode::Finish {
return;
}
if let Some(guard_op) = self.finish_guard_op.take() {
debug_assert!(
ctx.pending_finish_guard_postprocess.is_none(),
"postprocess_FINISH queued multiple guards"
);
ctx.pending_finish_guard_postprocess = Some(guard_op);
}
}

fn name(&self) -> &'static str {
"virtualize"
}
Expand Down
139 changes: 139 additions & 0 deletions pyre/bench/frame_lineno_mid_replay_regression.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
# Self-checking regression guard for the coordinate a frame reports WHILE it is
# still running (registered via check.py run_selfcheck, NOT the synthetic suite).
#
# `dispatch_bytecode` (pyopcode.py) stamps `last_instr` before every opcode, so
# a running frame answers `f_lineno`, `f_lasti` and any traceback taken off it
# for the instruction it is on. Compiled code does not run that store, and the
# blackhole replaying a frame syncs only `valuestackdepth`, so the coordinate
# reaches the frame only if the replay publishes it at the `-live-` marker.
# Without that publish a frame the function-entry portal compiled still carries
# the `-1` initialization sentinel, which `offset2lineno` answers with the code
# object's first line -- the `def` line, i.e. offset 0.
#
# Splitting each survey across several calls is what makes it a test: the loop
# compiles part-way through, so a set over the rounds holds the interpreted
# answer and the replayed one together and a divergence appears as a SECOND
# element rather than a shifted single value.
#
# Why this is not a synthetic bench: the wasm backend does not satisfy the
# invariant today, and check.py's synthetic suite has no per-backend scoping.
# Measured on this HEAD -- `plain` below reports `[0, 4]` on wasm against `[4]`
# on pypy3, CPython, dynasm and cranelift, with the divergence appearing from
# the first COMPILED call onward and persisting. Instrumenting both ends showed
# the marker hook writing the right coordinate into the right frame at the right
# offset and reading it back intact, and the interpreter then reading 0 from
# that same address -- so a wasm-side writer clears it between the publish and
# the residual `sys._getframe`. Neither `restore_resume_state_from` nor
# `set_last_instr_from_next_instr` is that writer (probed: neither ever targets
# the caller frame), and a blackhole `setfield_vable_i` cannot be, because the
# wasm blackhole builder carries no cpu and that handler would panic. The
# invariant is asserted here for the native backends while that is open; the
# post-return coordinate, which wasm does satisfy, stays in the synthetic bench.
import sys

N = 4000


def caller_offset():
"""The caller's coordinate, read from a callee while the caller is still
running. Nothing has left the caller's frame yet, so neither exit publish
has fired and the answer can only come from the frame being kept current."""
frame = sys._getframe(1)
return frame.f_lineno - frame.f_code.co_firstlineno


def raises_out(i):
raise KeyError(i)


def plain(n): # +0
k = 0 # +1
while k < n: # +2
k += 1 # +3
return caller_offset() # +4


def mid_replay_getframe(n): # +0
tb = None
k = 0
while k < n:
try:
raise ValueError(k)
except ValueError as e:
tb = e.__traceback__
k += 1
return caller_offset() # +9


def mid_replay_handler(n): # +0
tb = None
k = 0
while k < n:
try:
raise ValueError(k)
except ValueError as e:
tb = e.__traceback__
k += 1
try:
raises_out(k) # +10
except KeyError as e:
t = e.__traceback__
base = t.tb_frame.f_code.co_firstlineno
return (t.tb_lineno - base, t.tb_frame.f_lineno - base) # +14


def recursive_mid_replay(n, depth):
"""Direct recursion, every level with its own hot loop, so every level is
replayed and every level shares ONE code object with its caller -- the shape
where a per-level frame mix-up survives a code-object check. A level
answering for another one shows up as a shifted offset."""
tb = None
k = 0
while k < n:
try:
raise ValueError(k)
except ValueError as e:
tb = e.__traceback__
k += 1
if depth > 0:
inner = recursive_mid_replay(n, depth - 1)
else:
inner = ()
return ((caller_offset(), caller_offset()),) + inner # +17


def main():
rounds = 8
each = N // rounds
failures = []

def check(label, got, want):
if got != want:
failures.append(f"{label}: got {got!r}, want {want!r}")

check("plain", sorted({plain(each) for _ in range(rounds)}), [4])
check(
"getframe",
sorted({mid_replay_getframe(each) for _ in range(rounds)}),
[9],
)
check(
"handler",
sorted({mid_replay_handler(each) for _ in range(rounds)}),
[(10, 14)],
)
check(
"recursive",
recursive_mid_replay(N // 2, 3),
((17, 17), (17, 17), (17, 17), (17, 17)),
)

if failures:
for f in failures:
print("FAIL", f)
return 1
print("PASS mid-replay coordinates")
return 0


sys.exit(main())
Loading
Loading