Skip to content

jit-trace: name and remove builtin-inline blockers; x86 call/nursery results in the result register; w_class and __float__ exactness #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 9 commits into from
Aug 22, 2026
Merged

jit-trace: name and remove builtin-inline blockers; x86 call/nursery results in the result register; w_class and __float__ exactness #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb5fb10
jit-trace: carry the un-lowered helper's symbolic funcbox to the decl…
youknowone Aug 21, 2026
89881ee
jit-trace: route the raise path through published exception helpers
youknowone Aug 21, 2026
9f99d75
majit dynasm x86: keep call and nursery-allocation results in the reg…
youknowone Aug 21, 2026
41aee11
jit-trace: pin `w_class` on the operands five folds only unboxed
youknowone Aug 22, 2026
155ec09
jit-trace: rustfmt the `w_class` guard call sites
youknowone Aug 22, 2026
f8c7ca6
jit-trace: pin `w_class` on the `truth_int` operand
youknowone Aug 22, 2026
834b79f
interp: honor `__float__` on an int subclass at the float coercions, …
youknowone Aug 22, 2026
46d160a
interp: give `int.__float__` a payload-only handler
youknowone Aug 22, 2026
68013a9
jit-translate, interp: review follow-ups on the raise-path fusion
youknowone Aug 22, 2026
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
153 changes: 99 additions & 54 deletions majit/majit-backend-dynasm/src/x86/assembler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,46 @@ mod tests {

use crate::runner::DynasmBackend;

/// x86/regalloc.py `consider_call_malloc_nursery` binds the result with
/// `force_allocate_reg(op, selected_reg=ecx)`; only FrameManager may spill
/// it later. Storing it into a slot here as well grew every recursive
/// loop's JitFrame by one slot per allocation, which changes how much the
/// process allocates and so shifts the minor-collection schedule the
/// jitcounter decay is driven by.
#[test]
fn malloc_nursery_result_does_not_grow_frame_depth() {
let mut backend = DynasmBackend::new();
backend.attach_default_test_descrs();

let malloc = Rc::new(Op::new(
OpCode::CallMallocNursery,
&[Operand::from_opref(OpRef::const_int(32))],
));
malloc.pos.set(OpRef::ref_op(0));

let finish = Op::new(OpCode::Finish, &[Operand::from_bound_op(&malloc)]);
finish.pos.set(OpRef::void_op(1));
finish.set_fail_arg_types(vec![Type::Ref]);
finish.setfailargs(vec![].into());

let token = JitCellToken::new(517);
backend
.compile_loop(&[], &[malloc, Rc::new(finish)], &token)
.expect("compile register-resident nursery result trace");

let compiled = token
.compiled
.get()
.expect("compiled code")
.downcast_ref::<super::CompiledCode>()
.expect("dynasm compiled code");
assert_eq!(
compiled.frame_depth.load(Ordering::Acquire),
super::JITFRAME_FIXED_SIZE,
"a register-resident nursery result must not allocate a shadow frame slot",
);
}

fn compile_eval_breaker_poll_trace(
trace_id: u64,
word_addr: usize,
Expand Down Expand Up @@ -4190,7 +4230,7 @@ impl<'a> Assembler386<'a> {
// store next GUARD_NOT_FORCED's descr ptr to jf_force_descr
// BEFORE the call, so forcing code knows which guard to resume.
self._store_force_index_if_next_guard(ops, op_index, fail_index);
self.genop_call_assembler(op, arglocs);
self.genop_call_assembler(op, arglocs, result_loc);
}
OpCode::CondCallN => self.genop_discard_cond_call(op, arglocs),
OpCode::CondCallValueI | OpCode::CondCallValueR => {
Expand Down Expand Up @@ -4315,20 +4355,10 @@ impl<'a> Assembler386<'a> {
dynasm!(self.mc ; .arch x64 ; mov Rq(rv), rcx);
}
dynasm!(self.mc ; .arch x64 ; =>done);
// Spill the result to the regalloc-assigned jitframe slot
// directly from `result_reg`. The malloc-nursery clobber
// set is only ECX/EDX (SAVE_DEFAULT_REGS), so RAX may hold
// a live Box the regalloc bound to it across this op;
// routing the spill through RAX (`mov rax, result_reg`)
// silently destroyed that value — this mirrors the
// fixed-size `genop_call_malloc_nursery` final block, which
// was already corrected for the same reason.
let pos = op.pos.get();
if !pos.is_none() {
let slot = self.allocate_slot(pos);
let offset = Self::slot_offset(slot);
dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv));
}
// The payload already sits in `result_reg` on both paths, and
// that register is the delivery contract (see
// `genop_call_malloc_nursery`); an extra store grew
// `frame_depth` by one slot per allocation.
}
// x86/assembler.py malloc_cond_varsize parity
// arglocs = [lengthloc, imm(itemsize), imm(kind)]
Expand Down Expand Up @@ -4392,8 +4422,20 @@ impl<'a> Assembler386<'a> {
// result slot.
self.emit_propagate_exception_if_zero(0);
dynasm!(self.mc ; .arch x64 ; mov QWORD [rbp + gcmap_ofs], 0);
if !op.pos.get().is_none() {
self.store_rax_to_result(op.pos.get());
// Unlike the other three nursery paths, this one leaves the
// helper's return in RAX rather than in the regalloc result
// register, so the move is emitted here instead of the store
// that used to grow `frame_depth` by a slot per allocation.
// `consider_call_malloc_nursery_varsize` forces the result to
// `MALLOC_NURSERY_RESULT`, so this is never a no-op.
let Some(Loc::Reg(r)) = result_loc else {
panic!(
"CallMallocNurseryVarsize result_loc must be a register; got {result_loc:?}"
);
};
if r.value != crate::regloc::EAX.value {
let rv = r.value;
dynasm!(self.mc ; .arch x64 ; mov Rq(rv), rax);
}
}
// x86/assembler.py `genop_discard_check_memory_error`
Expand Down Expand Up @@ -6999,7 +7041,7 @@ impl<'a> Assembler386<'a> {
/// `reload_frame_if_necessary` because a minor GC during the callee
/// may have moved the caller jitframe; the popped rbp is the
/// pre-GC address while the shadow stack carries the updated one.
fn genop_call_assembler(&mut self, op: &Op, arglocs: &[Loc]) {
fn genop_call_assembler(&mut self, op: &Op, arglocs: &[Loc], result_loc: Option<&Loc>) {
// handle_call_assembler (rewrite.py) always pre-builds the
// callee jitframe — storing every inputarg, and for a virtualizable
// passing the forced vable object as the second arg — so the backend
Expand Down Expand Up @@ -7042,9 +7084,7 @@ impl<'a> Assembler386<'a> {
} else {
dynasm!(self.mc ; .arch x64 ; xor eax, eax);
}
if !op.pos.get().is_none() {
self.store_rax_to_result(op.pos.get());
}
self.move_call_assembler_result(result_type, result_loc);
return;
}

Expand Down Expand Up @@ -7119,8 +7159,36 @@ impl<'a> Assembler386<'a> {
; =>merge
);
}
if !op.pos.get().is_none() {
self.store_rax_to_result(op.pos.get());
self.move_call_assembler_result(result_type, result_loc);

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 22, 2026

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 Keep CALL_ASSEMBLER results visible to legacy consumers

When an x86 dynasm trace uses a CALL_ASSEMBLER result as the predicate of a following COND_CALL_N or COND_CALL_VALUE_*, this now leaves the value only in result_loc. Both conditional-call emitters still ignore that predicate's regalloc location and call load_arg_to_rax, whose resolve_opref only recognizes constants and frame slots; because the removed result spill also supplied the slot mapping, compiling this valid trace can now panic with “unmapped non-constant OpRef.” Pass the predicate argloc into those emitters, or retain materialization until every legacy consumer uses regalloc locations.

AGENTS.md reference: AGENTS.md:L184-L185

Useful? React with 👍 / 👎.

@youknowone youknowone Aug 22, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Confirmed and fixed in d790ddcb3d7.

The defect is real, and it is slightly broader than described — it is not
confined to a CALL_ASSEMBLER predicate. resolve_opref maps only constants
and frame slots, so any predicate the regalloc leaves register-resident has
no mapping there. Three sites read it that way, and all three now read
arglocs[0]:

  • x86::genop_discard_cond_call
  • x86::genop_cond_call_value
  • aarch64::genop_cond_call_value (same spelling, same defect)

aarch64::genop_discard_cond_call was already correct — it reads arglocs[0]
via emit_load_loc_to_ip0. The x86 twin's own comment claims to mirror it, but
mirrored it only for the callee and arguments, not for the predicate.

One thing worth recording, because it is why the x86 fix is not a literal
transcription of the aarch64 one: on x86 the predicate could not simply move to
arglocs[0] and stay in rax. ALL_CORE_REGS contains EAX, and
consider_discard_nargs_j2 emits no before_call, so for COND_CALL_N the
same op's other arglocs may themselves be caller-saved registers including rax
— loading the predicate there would clobber a call argument. The test therefore
moves to R11 (X86_64_SCRATCH_REG), which is outside the allocation pool; this
is the same reasoning that put the aarch64 test in ip0/x16 rather than x0. A
new emit_load_loc_to_scratch is the counterpart of emit_load_loc_to_ip0.

genop_cond_call_value keeps rax deliberately: on the not-taken path the
predicate is the result, and store_rax_to_result reads it from rax. That
load is safe there because consider_raw_call_like_j2 runs before_call
before computing arglocs, so no argloc is a caller-saved register.

Scope note: I did not change the callee/argument resolution inside
genop_cond_call_value (it still uses emit_call, not
emit_call_from_arglocs). That path has the same resolve_opref exposure, but
it is pre-existing rather than introduced here, and the two helpers differ in
their arg_types fallback (emit_call infers per-argument types;
emit_call_from_arglocs assumes all-Int), so swapping it is a separate change
with its own ABI risk. Flagging it rather than folding it in silently.

Landed in #1433 (ebc0710fcb3) — this PR was squash-merged before the fix was pushed.

— commented by Claude

}

/// Materialize a CALL_ASSEMBLER result from the raw bits both paths leave
/// in RAX into the regalloc-assigned location.
///
/// The previous shape spilled RAX to a fresh JitFrame slot, which grew
/// `frame_depth` by one slot per call and left a `Float` result taking the
/// helper path in RAX while the regalloc expected XMM0. Only the fast path
/// happened to leave it in XMM0 as a side effect of its `movq rax, xmm0`
/// normalisation.
///
/// x86/regalloc.py `_consider_call_assembler` binds the result through
/// `after_call`, so it is `eax` for an int or ref and `xmm0` for a float;
/// the move is elided when the value already sits there.
fn move_call_assembler_result(&mut self, result_type: Type, result_loc: Option<&Loc>) {
match (result_type, result_loc) {
(Type::Void, None) => {}
(Type::Float, Some(Loc::Reg(r))) if r.is_xmm => {
dynasm!(self.mc ; .arch x64 ; movq Rx(r.value), rax);
}
(_, Some(Loc::Reg(r))) if !r.is_xmm => {
if r.value != crate::regloc::EAX.value {
dynasm!(self.mc ; .arch x64 ; mov Rq(r.value), rax);
}
}
_ => panic!(
"CALL_ASSEMBLER result must use its regalloc result register: \
type={result_type:?} loc={result_loc:?}"
),
}
}

Expand Down Expand Up @@ -7495,25 +7563,11 @@ impl<'a> Assembler386<'a> {
let _ = gcmap_ofs;

dynasm!(self.mc ; .arch x64 ; =>done);
// Spill the result to the regalloc-assigned jitframe slot. Stage
// it directly from `result_reg`; routing through RAX (the previous
// shape) silently clobbered any live Box the regalloc bound to
// RAX across this op, since the malloc-nursery clobber set is
// only ECX/EDX.
let pos = op.pos.get();
if !pos.is_none() {
let slot = self.allocate_slot(pos);
let offset = Self::slot_offset(slot);
// malloc_cond / malloc_cond_varsize (assembler.py:2556,2604) keep
// the allocated pointer in ecx and route it through the regalloc-
// assigned `result_reg`. Anything else here would spill a stale
// RAX (now caller-live) instead of the object pointer.
let Some(Loc::Reg(r)) = result_loc else {
panic!("CallMallocNursery result_loc must be a register; got {result_loc:?}");
};
let rv = r.value;
dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv));
}
// x86/regalloc.py `consider_call_malloc_nursery` binds the result with
// `force_allocate_reg(op, selected_reg=ecx)`, so the register IS the
// delivery contract; FrameManager emits a spill only where a later
// lifetime boundary needs one. Storing it here as well grew
// `frame_depth` by one slot for every allocation.
}

/// Headerless fixed-size nursery allocation. `size` excludes any GC
Expand Down Expand Up @@ -7599,18 +7653,9 @@ impl<'a> Assembler386<'a> {
let _ = gcmap_ofs;

dynasm!(self.mc ; .arch x64 ; =>done);
let pos = op.pos.get();
if !pos.is_none() {
let slot = self.allocate_slot(pos);
let offset = Self::slot_offset(slot);
let Some(Loc::Reg(r)) = result_loc else {
panic!(
"CallMallocNurseryHeaderless result_loc must be a register; got {result_loc:?}"
);
};
let rv = r.value;
dynasm!(self.mc ; .arch x64 ; mov [rbp + offset], Rq(rv));
}
// The result register is the delivery contract (see
// `genop_call_malloc_nursery`); an extra store here grew `frame_depth`
// by one slot per allocation.
}

/// NEW: allocate a fixed-size object. Requires GC runtime.
Expand Down
29 changes: 17 additions & 12 deletions majit/majit-translate/src/codewriter/jitcode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,11 @@ pub struct JitCode {
/// so an answer already computed for the original holds for it too.
#[derive(Debug, Default, Clone)]
pub struct DerivedBodyFacts {
/// Whether descending into this body can reach a residual call whose
/// funcbox is an un-lowered helper's symbolic hash. Read through
/// [`JitCode::descent_reaches_unlowered_helper_call`].
descent_reaches_unlowered_helper_call: OnceLock<bool>,
/// The un-lowered helper a descent into this body can reach, named by
/// the symbolic hash standing in for its funcbox, or `None` when the
/// descent reaches no such call. Read through
/// [`JitCode::descent_unlowered_helper_blocker`].
descent_unlowered_helper_blocker: OnceLock<Option<i64>>,
}

mod oncelock_usize_serde {
Expand Down Expand Up @@ -312,16 +313,20 @@ impl JitCode {
.expect("JitCode body not yet set — call set_body() before body_mut()")
}

/// Whether descending into this body can reach a residual call whose
/// funcbox is an un-lowered helper's symbolic hash, computing the answer
/// with `compute` the first time it is asked. The property is fixed by the
/// assembled body, so the first answer is the only one this instance gives:
/// `body_mut` needs `&mut self`, which `runtime_fnaddr_patch` can only take
/// before the jitcode is published behind an `Arc`.
pub fn descent_reaches_unlowered_helper_call(&self, compute: impl FnOnce() -> bool) -> bool {
/// The un-lowered helper a descent into this body can reach, named by
/// the symbolic hash standing in for its funcbox, computing the answer
/// with `compute` the first time it is asked. `None` means the descent
/// reaches no such call. The property is fixed by the assembled body, so
/// the first answer is the only one this instance gives: `body_mut` needs
/// `&mut self`, which `runtime_fnaddr_patch` can only take before the
/// jitcode is published behind an `Arc`.
pub fn descent_unlowered_helper_blocker(
&self,
compute: impl FnOnce() -> Option<i64>,
) -> Option<i64> {
*self
.derived
.descent_reaches_unlowered_helper_call
.descent_unlowered_helper_blocker
.get_or_init(compute)
}

Expand Down
4 changes: 4 additions & 0 deletions majit/majit-translate/src/front/mir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,10 @@ fn lower_unstructured_with_static_addrs_and_attrs(
tail_forwarded_returns,
)
.map_err(LowerError::Unsupported)?;
// Fold each raise site's `PyError` constructor into its
// materialisation call, so the transparent constructor — which has
// no host symbol and therefore no address — leaves this graph.
crate::front::result_exc::fuse_kind_ctor_raise(&mut lo.graph);

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 21, 2026

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 Move raise fusion out of the translator special case

Remove this bespoke front-end fusion and express the opaque raise path in the interpreter source, or fix constructor lowering generically. This call makes the generated JIT recognize one exact PyError::type_error/literal-message CFG and substitute a helper that the interpreter never calls; consequently formatted messages, additional constructors, or harmless CFG reshaping silently bypass the fix and remain inline blockers. That is precisely the source/JIT divergence the repository requires generation fixes to avoid.

AGENTS.md reference: AGENTS.md:L12-L15

Useful? React with 👍 / 👎.

@youknowone youknowone Aug 22, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Taking this one seriously rather than waving it off, because one half of it is
literally accurate and I want to separate that half from the part I think does
not hold.

Accurate: pyerror_type_error_to_exc_object has no interpreter-side
caller. Its only references are the fnaddr registration, the fusion table in
front/result_exc.rs, and a test. If the standard for "the JIT is generated
from the interpreter source" is "every residual the JIT calls is also on a live
interpreter path", this helper fails it, and I am not going to argue otherwise.

Where I think the divergence claim does not hold: the helper is interpreter
source, and its body is the exact sequential composition of the pair it
replaces — not a re-implementation of it:

pub unsafe fn pyerror_type_error_to_exc_object(w_msg: *mut PyObject) -> *mut PyObject {
    let msg = unsafe { w_str_get_wtf8(w_msg) }.to_owned();
    PyError::type_error(msg).to_exc_object()
}

There is no behaviour the generated JIT can observe here that the unfused pair
would not produce. The previous spelling already passed the same receiver
address to the same body via CallTarget::Method{to_exc_object, receiver PyError}, so this does not introduce a new ABI or a new GC exposure either.

On "formatted messages and other constructors silently bypass the fix":
that is correct, and for formatted messages it is required, not incidental. The
helper does an unchecked w_str_get_wtf8 deref, and a runtime-formatted
message need not be a box_str_constant object — so the declining path
(producer alloc::fmt::format) is the safe direction. Declining falls back to
exactly today's lowering; nothing regresses, it simply is not improved.

On "additional constructors": I measured this before writing the table rather
than assuming it. type_error is the only PyError constructor that reaches a
raise site in the __pyre_wrap_* family — the rule fires at 582 of 681
constructors across 301 distinct wrapper graphs. A nine-constructor table would
have been speculative; one entry is the corpus.

On "fix constructor lowering generically" — this is the real point, and I
concede it is the better fix. I could not reach it. The generic fix is making
PyError::new lower, and it is blocked on two independent things: it is generic
over impl Into<Wtf8Buf> and returns an aggregate, and its struct construction
falls back to a symbolic transparent ctor because of the layout —
message: Wtf8Buf is 24 bytes against one 8-byte descr row, with kind at
offset 54 of 56. Closing that means changing PyError's layout, which an
adversarial review already refused for the trace-New variant.

So my honest summary: this is a mitigation with a measured ceiling (union
blocker census: CLEAR 0 → 73 of 561; the PyError-ctor bucket 560 → 376), not
the generic generation fix, and it does not pretend to be one. Whether a
measured mitigation is worth carrying while the generic fix is blocked is a
call about this repository's policy, not something I should decide unilaterally
— @youknowone, if you would rather this come out until constructor lowering
can be fixed properly, say so and I will drop the front/result_exc.rs rule
and its table entry; the rest of the commit (the published helper and its
fnaddr test) stands on its own.

— commented by Claude

if result_exc_ok_is_unit {
// Stamp `FUNC.RESULT = void`. The exception-link lowering
// already returns the unit `()` (the callee no longer
Expand Down
Loading
Loading