diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index 45c7a1bf921..b3ce39da480 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -3122,7 +3122,7 @@ impl<'a> AssemblerARM64<'a> { } OpCode::CondCallN => self.genop_discard_cond_call(op, arglocs), OpCode::CondCallValueI | OpCode::CondCallValueR => { - self.genop_cond_call_value(op); + self.genop_cond_call_value(op, arglocs); } // ── Allocation (raw, when GC rewriter is not active) ── OpCode::New => self.genop_new(op), @@ -6651,8 +6651,14 @@ impl<'a> AssemblerARM64<'a> { } /// COND_CALL_VALUE_I/R: if arg(0) == 0, call function; else result = arg(0). - fn genop_cond_call_value(&mut self, op: &Op) { - self.load_arg_to_rax(op.arg(0).to_opref()); + /// + /// The predicate comes from its regalloc location, not `resolve_opref`, + /// which only recognises constants and frame slots — a predicate left + /// register-resident has no slot mapping. It is loaded into x0 rather + /// than the ip0 scratch because on the not-taken path the predicate IS the + /// result, and `store_rax_to_result` reads it from there. + fn genop_cond_call_value(&mut self, op: &Op, arglocs: &[Loc]) { + self.emit_load_to_rax(arglocs[0]); let skip_label = self.mc.new_dynamic_label(); dynasm!(self.mc ; .arch aarch64 ; cbnz x0, =>skip_label); diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index 3c6d035296e..26d786eaa1c 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -1427,6 +1427,19 @@ impl<'a> Assembler386<'a> { } } + /// Emit: load a regalloc Loc into the dedicated scratch (R11), which is + /// outside `ALL_CORE_REGS`, so the load cannot clobber a value the + /// regalloc still has live in an allocatable register. Counterpart of + /// the AArch64 `emit_load_loc_to_ip0`. + fn emit_load_loc_to_scratch(&mut self, loc: Loc) { + let scratch = crate::regloc::X86_64_SCRATCH_REG; + match loc { + // already there + Loc::Reg(r) if !r.is_xmm && r.value == scratch.value => {} + _ => self.regalloc_mov(&loc, &Loc::Reg(scratch)), + } + } + /// Emit: load the value of `opref` into RCX (x64) / X1 (aarch64). fn load_arg_to_rcx(&mut self, opref: OpRef) { match self.resolve_opref(opref) { @@ -4234,7 +4247,7 @@ impl<'a> Assembler386<'a> { } OpCode::CondCallN => self.genop_discard_cond_call(op, arglocs), OpCode::CondCallValueI | OpCode::CondCallValueR => { - self.genop_cond_call_value(op); + self.genop_cond_call_value(op, arglocs); } // ── Allocation (raw, when GC rewriter is not active) ── OpCode::New => self.genop_new(op), @@ -7969,8 +7982,18 @@ impl<'a> Assembler386<'a> { if let Some(cc) = self.guard_success_cc.take() { self.emit_jcc_to_label(invert_cc(cc), skip_label); } else { - self.load_arg_to_rax(op.arg(0).to_opref()); - dynasm!(self.mc ; .arch x64 ; test rax, rax ; jz =>skip_label); + // Read the predicate from its regalloc location, not via + // `resolve_opref`: `consider_discard_nargs_j2` emits no + // `before_call`, so a predicate the regalloc left register-resident + // has no slot mapping and would panic there. Test it in the + // scratch (R11) rather than rax, which IS allocatable here and may + // still hold one of the call's own arglocs. + self.emit_load_loc_to_scratch(arglocs[0]); + let scratch = crate::regloc::X86_64_SCRATCH_REG.value; + dynasm!(self.mc ; .arch x64 + ; test Rq(scratch), Rq(scratch) + ; jz =>skip_label + ); } // `consider_discard_nargs` emits no `before_call`, so the regalloc @@ -7996,8 +8019,15 @@ impl<'a> Assembler386<'a> { } /// COND_CALL_VALUE_I/R: if arg(0) == 0, call function; else result = arg(0). - fn genop_cond_call_value(&mut self, op: &Op) { - self.load_arg_to_rax(op.arg(0).to_opref()); + /// + /// The predicate comes from its regalloc location for the same reason as + /// `genop_discard_cond_call`. It is loaded into rax rather than the + /// scratch because on the not-taken path the predicate IS the result, and + /// `store_rax_to_result` reads it from there. `consider_raw_call_like_j2` + /// runs `before_call` before computing arglocs, so no argloc is a + /// caller-saved register and this load cannot clobber one. + fn genop_cond_call_value(&mut self, op: &Op, arglocs: &[Loc]) { + self.emit_load_to_rax(arglocs[0]); let skip_label = self.mc.new_dynamic_label(); dynasm!(self.mc ; .arch x64 ; test rax, rax ; jnz =>skip_label); diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats index 9cb62b5b198..93ab6cc6b4d 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.cranelift.jitstats @@ -8,8 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=17 +guard_failures=24 internal_compile_panics=0 loops_aborted=0 -loops_compiled=17 +loops_compiled=24 retraces_compiled=0 diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats index 9cb62b5b198..93ab6cc6b4d 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.dynasm.jitstats @@ -8,8 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=17 +guard_failures=24 internal_compile_panics=0 loops_aborted=0 -loops_compiled=17 +loops_compiled=24 retraces_compiled=0 diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.py b/pyre/bench/synth/float_subclass_binop_dispatch.py index eda73f5d90e..ace711d3c20 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.py +++ b/pyre/bench/synth/float_subclass_binop_dispatch.py @@ -196,6 +196,56 @@ def warm_then_swap_store_attr(n): return type(holder.x).__name__ +# The same three storage paths on the float side. The value's exact class must +# survive the store: a `LiarFloat` written through a warmed-up float store must +# read back as `LiarFloat`, not as `float`. +def warm_then_swap_store_subscr_float(n): + lst = [0.0] + for a in [0.0] * n + [LiarFloat(7.0)]: + lst[0] = a + return type(lst[0]).__name__ + + +def warm_then_swap_newlist_float(n): + out = None + for a in [0.0] * n + [LiarFloat(7.0)]: + out = [a] + return type(out[0]).__name__ + + +def warm_then_swap_store_attr_float(n): + holder = Slotted() + for a in [0.0] * n + [LiarFloat(7.0)]: + holder.x = a + return type(holder.x).__name__ + + +# Cold start on the storage side: the subclass is the value from the first +# iteration, so the unboxed-strategy gates must decline on the recorded value +# rather than rely on the `w_class` pin to reject a later arrival. An unboxed +# int/float slot stores the raw payload, so a fold here reads back as `int` / +# `float` instead of the subclass. +def store_subscr_cold_subclass(n): + lst = [0] + for _ in range(n): + lst[0] = LiarInt(7) + return type(lst[0]).__name__ + + +def store_attr_cold_subclass(n): + holder = Slotted() + for _ in range(n): + holder.x = LiarInt(7) + return type(holder.x).__name__ + + +def store_attr_cold_subclass_float(n): + holder = Slotted() + for _ in range(n): + holder.x = LiarFloat(7.0) + return type(holder.x).__name__ + + # `truth_int` reaches the same hole from the branch side rather than the value # side: `POP_JUMP_IF_*` and the short-circuit operators read the truth of a # payload the `GUARD_CLASS INT` admits, so a `__bool__` override on a zero-payload @@ -216,6 +266,20 @@ def warm_then_swap_truth_and(n): return out +# Cold start: the trace records on the subclass from the very first iteration +# rather than meeting it after warming up on exact ints. The fold has to decline +# on the *recorded* operand — pinning `w_class` only rejects a subclass that +# arrives later, and the walk is the authoritative executor, so a payload-folded +# truth here is the answer the program returns. `LiarBool(0)` is falsy by payload +# and true by `__bool__`, so the two answers differ. +def truth_cold_subclass(n): + hits = 0 + for a in [LiarBool(0)] * n: + if a: + hits += 1 + return hits + + def truth_bool_call_control(n): out = None for a in [0] * n + [LiarBool(0)]: @@ -228,6 +292,13 @@ def truth_bool_call_control(n): print(warm_then_swap_store_subscr(N)) print(warm_then_swap_newlist(N)) print(warm_then_swap_store_attr(N)) +print(warm_then_swap_store_subscr_float(N)) +print(warm_then_swap_newlist_float(N)) +print(warm_then_swap_store_attr_float(N)) print(warm_then_swap_truth_if(N)) print(warm_then_swap_truth_and(N)) +print(store_subscr_cold_subclass(N)) +print(store_attr_cold_subclass(N)) +print(store_attr_cold_subclass_float(N)) +print(truth_cold_subclass(N)) print(truth_bool_call_control(N)) diff --git a/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats b/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats index 9cb62b5b198..93ab6cc6b4d 100644 --- a/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats +++ b/pyre/bench/synth/float_subclass_binop_dispatch.wasm.jitstats @@ -8,8 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=17 +guard_failures=24 internal_compile_panics=0 loops_aborted=0 -loops_compiled=17 +loops_compiled=24 retraces_compiled=0 diff --git a/pyre/pyre-interpreter/src/module/math/interp_math.rs b/pyre/pyre-interpreter/src/module/math/interp_math.rs index 0d1e17bb81d..516851e2475 100644 --- a/pyre/pyre-interpreter/src/module/math/interp_math.rs +++ b/pyre/pyre-interpreter/src/module/math/interp_math.rs @@ -30,24 +30,10 @@ pub fn try_get_double(obj: PyObjectRef) -> Result { // subclass falls to the ladder below; an inherited `int.__float__` // reproduces the payload. The `float` arm stays ungated because the // conversion short-circuits on the layout and ignores an override. - if pyre_object::is_exact_builtin_instance(obj) { - if is_int(obj) { - return Ok(w_int_get_value(obj) as f64); - } - if is_long(obj) { - // A Python int is always finite, so a non-finite conversion means - // the magnitude exceeds f64 range — PyFloat_AsDouble raises here. - let v = jit_bigint_to_f64_or_nan(w_long_get_value(obj)); - if !v.is_finite() { - return Err(crate::PyError::overflow_error( - "int too large to convert to float", - )); - } - return Ok(v); - } - if is_bool(obj) { - return Ok(if w_bool_get_value(obj) { 1.0 } else { 0.0 }); - } + if pyre_object::is_exact_builtin_instance(obj) + && let Some(value) = crate::builtins::int_payload_as_f64(obj) + { + return value; } } // `__float__` is a type-only special-method lookup (`space.lookup`); an diff --git a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs index 23f4f72bf66..37b095f2914 100644 --- a/pyre/pyre-interpreter/src/objspace/std/mapdict.rs +++ b/pyre/pyre-interpreter/src/objspace/std/mapdict.rs @@ -3404,7 +3404,7 @@ fn box_value(typ: UnboxType, val: i64) -> PyObjectRef { /// /// # Safety /// `w_value` must point to a live object. -unsafe fn is_unboxable_int(w_value: PyObjectRef) -> bool { +pub unsafe fn is_unboxable_int(w_value: PyObjectRef) -> bool { if unsafe { pyre_object::is_bool(w_value) } || !unsafe { pyre_object::is_int(w_value) } { return false; } diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index 1c713c0ee4b..ba485329b1a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -181,7 +181,19 @@ pub(crate) fn try_walker_specialize_truth_int( return Ok(None); }; let val = unsafe { - if !pyre_object::is_int(obj) || pyre_object::is_bool(obj) { + // `is_int` reads `ob_type`, which an `int` subclass shares, so it alone + // admits one here. Two things then go wrong at once: the walk folds the + // truth straight off the payload instead of running the subclass's + // `__bool__`, and `walker_numeric_builtin_class` answers with the + // canonical `int` — a `w_class` the recorded operand does not carry, so + // the pin below becomes a guard that fails on the very value that + // recorded it. Decline before unboxing, as `walker_unary_int_operand` + // does; `walker_numeric_builtin_class` documents this gate as its + // precondition. + if !pyre_object::is_int(obj) + || pyre_object::is_bool(obj) + || !pyre_object::is_exact_builtin_instance(obj) + { return Ok(None); } pyre_object::w_int_get_value(obj) @@ -3898,11 +3910,12 @@ pub(crate) fn try_walker_specialize_store_attr( } { match unbox_type { pyre_interpreter::objspace::std::mapdict::UnboxType::Int => { - // `type(w_value) is space.IntObjectCls` (mapdict.py): reject bool - // and every type-changing value before emitting any guards. - if unsafe { - pyre_object::pyobject::is_bool(concrete_value) - || !pyre_object::pyobject::is_int(concrete_value) + // Match mapdict.py `_direct_write` exactly, through the same + // predicate the interpreter's own store uses: `is_int` reads + // `ob_type`, which an `int` subclass shares, so unboxing on it + // would take the raw payload and lose `w_class`. + if !unsafe { + pyre_interpreter::objspace::std::mapdict::is_unboxable_int(concrete_value) } { return Ok(None); } @@ -14234,8 +14247,16 @@ pub(crate) fn try_walker_specialize_store_subscr( if index as usize >= concrete_len { return Ok(None); } + // Object storage keeps the value boxed, so a subclass survives it; the + // unboxed strategies write the raw payload and would drop the subclass + // identity the read-back must return. `is_int`/`is_float` read + // `ob_type`, which a subclass shares, so they alone do not establish + // that -- and `walker_numeric_builtin_class` below answers with the + // canonical class, which such a value does not carry. let sid = if pyre_object::w_list_uses_object_storage(list_obj) { 0i64 + } else if !pyre_object::is_exact_builtin_instance(value_obj) { + return Ok(None); } else if pyre_object::w_list_uses_int_storage(list_obj) && pyre_object::is_int(value_obj) && !pyre_object::is_bool(value_obj)