diff --git a/majit/majit-translate/src/model.rs b/majit/majit-translate/src/model.rs index 80ea67220b9..6b911c22eca 100644 --- a/majit/majit-translate/src/model.rs +++ b/majit/majit-translate/src/model.rs @@ -7283,6 +7283,202 @@ mod tests { } } + #[test] + fn fuse_boxing_alloc_resolves_a_split_cluster_only_when_the_links_agree() { + // A boxing cluster's header stores sit before its `malloc_typed`, and + // each call ends a block, so `ob_type` / `w_class` reach the ctor as + // `Block.inputargs` while the `ConstRefAddr` producing them stays in a + // predecessor. `resolve_addr` steps through the links to read it, and + // takes an answer only when every predecessor agrees: a phi merging two + // type pointers is not a constant, and stamping either one onto the + // allocation would name the wrong type. Every other cluster here is + // built in one block, so neither half of that walk is otherwise reached. + type Var = crate::flowspace::model::Variable; + const FLOAT_TYPE_ADDR: i64 = 4357049520; + const OTHER_TYPE_ADDR: i64 = 4357049600; + + fn call(graph: &mut FunctionGraph, blk: BlockId, path: &[&str], args: Vec) -> Var { + graph + .push_op_var( + blk, + OpKind::Call { + target: CallTarget::FunctionPath { + segments: path.iter().map(|s| (*s).to_string()).collect(), + }, + args, + result_ty: ValueType::Ref(Some("object".into())), + }, + true, + ) + .unwrap() + } + /// The `(ob_type, w_class)` pair a `w_float_new` header stores, both + /// read off a `&FLOAT_TYPE`-shaped constant at `addr`. + fn header_pair(graph: &mut FunctionGraph, blk: BlockId, addr: i64) -> Vec { + let cast = |graph: &mut FunctionGraph| { + let ty = graph + .push_op_var(blk, OpKind::ConstRefAddr(addr), true) + .unwrap(); + call(graph, blk, &["__pyre_cast_instance", "PyType"], vec![ty]) + }; + let ob_type = cast(graph); + let w_class_cast = cast(graph); + let w_class = call( + graph, + blk, + &["pyre_object", "pyobject", "get_instantiate"], + vec![w_class_cast], + ); + vec![ob_type, w_class] + } + /// The rest of the cluster, in `blk`: the nested `PyObject` header + /// taking the two values `blk` was handed, the `W_FloatObject` ctor and + /// its payload store, and the `malloc_typed` the fusion rewrites. + fn cluster_in(graph: &mut FunctionGraph, blk: BlockId, ob_type: &Var, w_class: &Var) { + let field = |base: &Var, name: &str, owner: &str, value: &Var| OpKind::FieldWrite { + base: base.clone(), + field: FieldDescriptor { + name: name.into(), + owner_root: Some(owner.into()), + owner_id: None, + base_is_deref: None, + taken_by_address: false, + }, + value: LinkArg::Value(value.clone()), + ty: ValueType::Ref(None), + }; + let ctor = |graph: &mut FunctionGraph, name: &str| { + graph + .push_op_var( + blk, + OpKind::Call { + target: CallTarget::synthetic_transparent_ctor(name), + args: vec![], + result_ty: ValueType::Ref(Some(name.into())), + }, + true, + ) + .unwrap() + }; + let payload = graph + .push_op_var(blk, OpKind::ConstFloat(0.0f64.to_bits()), true) + .unwrap(); + let header = ctor(graph, "PyObject"); + graph.push_op_var(blk, field(&header, "ob_type", "PyObject", ob_type), false); + graph.push_op_var(blk, field(&header, "w_class", "PyObject", w_class), false); + let agg = ctor(graph, "W_FloatObject"); + graph.push_op_var( + blk, + field(&agg, "ob_header", "W_FloatObject", &header), + false, + ); + graph.push_op_var( + blk, + field(&agg, "floatval", "W_FloatObject", &payload), + false, + ); + let ret = call( + graph, + blk, + &["pyre_object", "lltype", "malloc_typed"], + vec![agg], + ); + graph.set_return(blk, Some(ret)); + } + + /// One producer of the header pair, `crossings` blocks of pure relay, + /// then the cluster — the shape a run of calls before the allocation + /// leaves behind. + fn chain(crossings: usize) -> FunctionGraph { + let mut graph = FunctionGraph::new("test"); + let entry = graph.startblock; + let mut carried = header_pair(&mut graph, entry, FLOAT_TYPE_ADDR); + let mut from = entry; + for _ in 0..crossings { + let (next, args) = graph.create_block_with_arg_vars(2); + graph.set_goto(from, next, carried); + carried = args; + from = next; + } + cluster_in(&mut graph, from, &carried[0], &carried[1]); + graph + } + /// Two predecessors of one merge block, each building its own header + /// pair off the address it is given, and the cluster in the merge. + fn merge(left_addr: i64, right_addr: i64) -> FunctionGraph { + let mut graph = FunctionGraph::new("test"); + let entry = graph.startblock; + let cond = graph.push_op_var(entry, OpKind::ConstInt(0), true).unwrap(); + let (join, carried) = graph.create_block_with_arg_vars(2); + let arms: Vec = [(true, left_addr), (false, right_addr)] + .into_iter() + .map(|(case, addr)| { + let arm = graph.create_block(); + let pair = header_pair(&mut graph, arm, addr); + graph.set_goto(arm, join, pair); + Link::from_variables(&graph, vec![], arm, Some(ExitCase::Bool(case))) + }) + .collect(); + graph.block_mut(entry).exitswitch = Some(ExitSwitch::Value(cond)); + graph.closeblock(entry, arms); + cluster_in(&mut graph, join, &carried[0], &carried[1]); + graph + } + + let rows: [(&str, &dyn Fn() -> FunctionGraph, usize); 4] = [ + ("one link crossing", &|| chain(1), 1), + ("two link crossings", &|| chain(2), 1), + ( + "predecessors naming one type", + &|| merge(FLOAT_TYPE_ADDR, FLOAT_TYPE_ADDR), + 1, + ), + ( + "predecessors naming two types", + &|| merge(FLOAT_TYPE_ADDR, OTHER_TYPE_ADDR), + 0, + ), + ]; + for (shape, build, expected) in rows { + let mut graph = build(); + assert_eq!( + fuse_boxing_alloc(&mut graph, &numeric_boxing_attrs()), + expected, + "{shape}: wrong number of fused clusters" + ); + let vtables: Vec = graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .filter_map(|op| match &op.kind { + OpKind::NewWithVtable { vtable, .. } => Some(*vtable), + _ => None, + }) + .collect(); + // Naming the address rather than counting the op is what separates + // a walk that read the predecessor from one that read some other + // constant in the graph. + let expected_vtables = if expected == 0 { + Vec::new() + } else { + vec![FLOAT_TYPE_ADDR] + }; + assert_eq!(vtables, expected_vtables, "{shape}: wrong vtable stamped"); + let residual = graph.blocks.iter().flat_map(|b| &b.operations).any(|op| { + matches!( + &op.kind, + OpKind::Call { target: CallTarget::FunctionPath { segments }, .. } + if segments.last().map(String::as_str) == Some("malloc_typed") + ) + }); + assert_eq!( + residual, + expected == 0, + "{shape}: malloc_typed residual must survive exactly when the cluster declines" + ); + } + } + #[test] fn fuse_boxing_alloc_sweeps_nested_header_chain() { // Faithful `w_float_new` shape: the boxing struct's header is a nested diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index f7c98faf884..46854eb2f0b 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -961,19 +961,19 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { "pyre_object::try_gc_alloc_stable_raw", pyre_object::gc_hook::try_gc_alloc_stable_raw as *const (), ); - // `w_int_box_slow` is the allocating tail of `w_int_new`, reached from + // `w_int_gc_alloc` is the collector-heap arm of `w_int_new`, reached from // inside a descended body whenever a fold boxes an int. Bind the // macro-emitted trampoline rather than the raw fn, for the reason // `prepare_list_ref_store` documents: the raw `(i64) -> *mut PyObject` is // `(i64) -> i32` on wasm32, while the wasm backend types the residual's // `call_indirect` `(i64) -> i64` from the descr alone. - let w_int_box_slow: extern "C" fn(i64) -> i64 = - pyre_object::intobject::__majit_call_target_w_int_box_slow; + let w_int_gc_alloc: extern "C" fn(i64) -> i64 = + pyre_object::intobject::__majit_call_target_w_int_gc_alloc; push_alias_pair( &mut entries, - "pyre_object::intobject::w_int_box_slow", - "pyre_object::w_int_box_slow", - w_int_box_slow as *const (), + "pyre_object::intobject::w_int_gc_alloc", + "pyre_object::w_int_gc_alloc", + w_int_gc_alloc as *const (), ); // `w_type_set_abstract` stores the runtime-mutable `flag_abstract` atomic — a // side effect on per-type state, not a build-time constant, so it carries diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index d05df1de0b8..71c02c2d06b 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -9699,7 +9699,10 @@ pub(crate) fn try_walker_orthodox_list_append( ); match commit_result { Ok(()) => {} - Err(DispatchError::OrthodoxSubWalkTraceUnsupported { .. }) => { + Err(DispatchError::OrthodoxSubWalkTraceUnsupported { pc }) => { + if fbw_debug_abort_enabled() { + eprintln!("[decline-why] LIST-APPEND-SUBWALK pc={pc}"); + } ctx.trace_ctx.cut_trace(pre_fold_pos); ctx.trace_ctx.heap_cache_mut().reset(); bool_box_truth_reset(); @@ -10236,7 +10239,10 @@ pub(crate) fn try_walker_orthodox_list_pop( ctx, op, sym, &sub_body, self_ref, inner_self, len_before, raw_item, dst, ) { Ok(()) => Ok(Some(())), - Err(DispatchError::OrthodoxSubWalkTraceUnsupported { .. }) => { + Err(DispatchError::OrthodoxSubWalkTraceUnsupported { pc }) => { + if fbw_debug_abort_enabled() { + eprintln!("[decline-why] LIST-POP-SUBWALK pc={pc}"); + } ctx.trace_ctx.cut_trace(pre_fold_pos); ctx.trace_ctx.heap_cache_mut().reset(); bool_box_truth_reset(); @@ -10486,7 +10492,10 @@ pub(crate) fn try_walker_orthodox_list_append_opcode( ); match commit_result { Ok(()) => {} - Err(DispatchError::OrthodoxSubWalkTraceUnsupported { .. }) => { + Err(DispatchError::OrthodoxSubWalkTraceUnsupported { pc }) => { + if fbw_debug_abort_enabled() { + eprintln!("[decline-why] LIST-APPEND-SUBWALK pc={pc}"); + } ctx.trace_ctx.cut_trace(pre_fold_pos); ctx.trace_ctx.heap_cache_mut().reset(); bool_box_truth_reset(); diff --git a/pyre/pyre-object/src/intobject.rs b/pyre/pyre-object/src/intobject.rs index 781cf05d0e6..25ac1d49bbf 100644 --- a/pyre/pyre-object/src/intobject.rs +++ b/pyre/pyre-object/src/intobject.rs @@ -101,7 +101,7 @@ static SMALL_INTS: LazyLock = LazyLock::new(|| { ) }); -/// `pypy/objspace/std/intobject.py:883-897 wrapint` parity. +/// `pypy/objspace/std/intobject.py:903-921 wrapint` parity. /// /// `withprebuiltint=False` (PyPy default) → always allocate fresh, /// matching upstream `return W_IntObject(x)`. With the flag enabled @@ -109,6 +109,27 @@ static SMALL_INTS: LazyLock = LazyLock::new(|| { /// returns the pre-allocated entry; outside the range we allocate /// (`instantiate(W_IntObject)` upstream). /// +/// Traced, not residualised, and `#[inline]` — `wrapint` carries no +/// `@dont_look_inside` and its own comment reads "this whole function is +/// getting inlined into every caller so keeping the branching to a minimum +/// is a good idea" (intobject.py:908-910). The allocation upstream is +/// `instantiate(W_IntObject)` followed by `w_res.intval = x` +/// (intobject.py:913-920): alloc-then-init, which the rtyper lowers to the +/// `new_with_vtable` + payload `setfield_gc` pair the optimizer can +/// virtualize where the box does not escape. The stack-built +/// `malloc_typed(W_IntObject { .. })` spelling below is the Rust form of the +/// same thing: `fuse_boxing_alloc` (`majit-translate` `model.rs`) rewrites +/// that cluster into exactly that pair, and it does fire here — its walk +/// resolves the header pointers across the block boundary each call ends. +/// +/// `bench/synth/list_pop_append` reads the same either way, and its negative +/// control (boundary removed with the fusion reverted) failed to reproduce +/// the regression that motivated the boundary, so that bench does not +/// discriminate this mechanism in either direction. What decides it is the +/// shape: a residual call can never be virtualized and `new_with_vtable` +/// can, and the boundary's own stated blocker — the fusion not resolving a +/// vtable here — is gone. +/// /// The allocation path goes through [`crate::lltype::malloc_typed`] /// which carries `W_INT_GC_TYPE_ID` + /// `W_INT_OBJECT_SIZE` via the [`crate::lltype::GcType`] impl above — @@ -133,61 +154,10 @@ pub fn w_int_new(value: i64) -> PyObjectRef { let idx = (value - PREBUILTINTFROM) as usize; return (&SMALL_INTS.0[idx] as *const W_IntObject).cast_mut() as PyObjectRef; } - w_int_box_slow(value) -} - -/// The allocating tail of [`w_int_new`], behind a residualisation boundary -/// (`rlib/jit.py:139 @dont_look_inside`). The collector arm is the -/// `gct_fv_gc_malloc` bracket (`rpython/memory/gctransform/framework.py`) in -/// its alloc-then-init form — take the block, then write the header and -/// payload into it; it falls through to `malloc_typed` when no collector owns -/// the heap. -/// -/// Both arms have a shape no trace can carry, which is why the boundary sits -/// around the pair rather than around either one. A stack-built `W_IntObject` -/// lowers to a `SyntheticTransparentCtor` for its `PyObject` header, whose -/// funcptr constant degrades to a `symbolic_fnaddr` hash; a descending -/// sub-jitcode walk cannot record such a call, so it declines the entire -/// descent — that is what takes `list.pop()`'s fold off the compiled loop. -/// Writing the fields individually instead lands the header's own `ob_type` -/// slot at offset 0, which the wasm backend does not lower faithfully. -/// -/// The boundary is a deviation from `wrapint` -/// (`objspace/std/intobject.py:908-910`), which keeps its allocation inline — -/// its own comment there notes the function is inlined into every caller. The -/// orthodox lowering is `new_with_vtable`, which stays in the trace and can be -/// optimised away where the box does not escape. `fuse_boxing_alloc` -/// (`majit-translate` `model.rs`) rewrites exactly this ctor-plus-`FieldWrite` -/// shape into `NewWithVtable`, and it does now fire here: the static address -/// table is populated in the build-script pipeline, and the pass reported -/// every site unresolved only because its walk stopped at a block boundary — -/// each call ends a block, so a header store sitting before one crosses as a -/// link argument while its `ConstRefAddr` producer stays in the predecessor. -/// -/// So the vtable now resolves here, but removing the boundary was measured to -/// change nothing: `bench/synth/list_pop_append` reads the same on all three -/// backends with the boundary present, with it removed, and with it removed -/// while the fusion is reverted. That last arm is a negative control which was -/// expected to reproduce the pre-boundary regression and did not, so the bench -/// no longer discriminates this mechanism and its null says nothing either -/// way. The boundary stays until an instrument that can tell the two apart -/// says otherwise. -/// -/// Spelled `*mut PyObject` rather than `PyObjectRef` so the attribute emits -/// its `extern "C"` call trampoline: the macro recognises raw pointers -/// syntactically and declines to emit one for an aliased return type. -#[majit_macros::dont_look_inside] -pub fn w_int_box_slow(value: i64) -> *mut PyObject { if crate::gc_interp::enabled() { - let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_INT_GC_TYPE_ID, W_INT_OBJECT_SIZE); - if !raw.is_null() { - unsafe { - let p = raw as *mut W_IntObject; - (*p).ob_header.ob_type = &INT_TYPE as *const PyType; - (*p).ob_header.w_class = get_instantiate(&INT_TYPE); - (*p).intval = value; - } - return raw as PyObjectRef; + let boxed = w_int_gc_alloc(value); + if !boxed.is_null() { + return boxed; } } crate::lltype::malloc_typed(W_IntObject { @@ -199,6 +169,45 @@ pub fn w_int_box_slow(value: i64) -> *mut PyObject { }) as PyObjectRef } +/// The collector-heap arm of [`w_int_new`]: the `gct_fv_gc_malloc` bracket +/// (`rpython/memory/gctransform/framework.py`) in its alloc-then-init form — +/// take the block, then write the header and payload into it, rather than +/// copying a stack-built struct over it. Returns null when no collector owns +/// the heap, which is the caller's signal to take the `malloc_typed` arm. +/// +/// Residualised (`rlib/jit.py:139 @dont_look_inside`) rather than traced, +/// which [`w_int_new`] is not — a deviation this arm alone carries, because +/// neither spelling of it reaches the trace intact. Writing the fields into +/// the collector's block, as below, lands the header's own `ob_type` slot at +/// offset 0, which the wasm backend does not lower faithfully. Building the +/// struct on the stack and copying it in instead is not the +/// `malloc_typed(%agg)` route `fuse_boxing_alloc` rewrites, so its +/// `SyntheticTransparentCtor` for the `PyObject` header survives lowering as +/// a call to a `symbolic_fnaddr` hash, which a descending sub-jitcode walk +/// cannot record and declines on. The boundary keeps both shapes out of the +/// trace, and costs nothing where the arm is unreachable: +/// `gc_interp::enabled()` is false on the native backends, whose +/// `malloc_typed` arm is fused into a `NewWithVtable`. Drop it once the wasm +/// backend lowers an offset-0 field store faithfully. +/// +/// Spelled `*mut PyObject` rather than `PyObjectRef` so the attribute emits +/// its `extern "C"` call trampoline: the macro recognises raw pointers +/// syntactically and declines to emit one for an aliased return type. +#[majit_macros::dont_look_inside] +pub fn w_int_gc_alloc(value: i64) -> *mut PyObject { + let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_INT_GC_TYPE_ID, W_INT_OBJECT_SIZE); + if raw.is_null() { + return crate::PY_NULL; + } + unsafe { + let p = raw as *mut W_IntObject; + (*p).ob_header.ob_type = &INT_TYPE as *const PyType; + (*p).ob_header.w_class = get_instantiate(&INT_TYPE); + (*p).intval = value; + } + raw as PyObjectRef +} + /// Create a W_IntObject bypassing the small-int cache. /// /// Used for int subclass instances that need unique object identity