diff --git a/majit/majit-translate/src/model.rs b/majit/majit-translate/src/model.rs index ad7958dbc7c..f38f1456fdc 100644 --- a/majit/majit-translate/src/model.rs +++ b/majit/majit-translate/src/model.rs @@ -2972,6 +2972,60 @@ pub fn fuse_boxing_alloc( _ => None, }) } + /// The op-result variables `var` can be, following the links when `var` is + /// a `Block.inputargs` phi rather than an op result. + /// + /// A field store is recorded against the variable the producing operation + /// wrote, so a header value that crosses a block boundary — each preceding + /// call ends a block — leaves its `ob_type` / `w_class` stores behind on + /// the predecessor's variable, and a bare `base == var` lookup on the phi + /// finds nothing. Collecting the roots first puts the lookup back on the + /// variable the stores actually name. Every root is then required to agree, + /// exactly as `resolve_addr` requires of a merged pointer. `depth` bounds + /// the walk so a loop-carried phi whose own link arg is itself terminates. + fn store_roots( + graph: &FunctionGraph, + var: &Variable, + depth: u32, + out: &mut Vec, + ) -> bool { + if depth == 0 { + return false; + } + if graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .any(|o| o.result.as_ref() == Some(var)) + { + out.push(var.clone()); + return true; + } + let Some((target_id, slot)) = graph.blocks.iter().find_map(|b| { + b.inputargs + .iter() + .position(|arg| arg == var) + .map(|slot| (b.id, slot)) + }) else { + return false; + }; + for block in &graph.blocks { + for link in &block.exits { + if link.target != target_id { + continue; + } + // A link whose arity does not cover the slot is a malformed + // edge; refuse rather than read past it. + let Some(arg) = link.args.get(slot).and_then(LinkArg::as_variable) else { + return false; + }; + if !store_roots(graph, arg, depth - 1, out) { + return false; + } + } + } + !out.is_empty() + } /// Resolve `var` to the constant address `terminal` reads off its producer. /// /// The walk itself is shared by every header spelling: it steps through @@ -3086,19 +3140,31 @@ pub fn fuse_boxing_alloc( let Some(header) = store_value(graph, agg, "ob_header") else { return 0; }; - let Some(vtable) = store_value(graph, &header, "ob_type") - .and_then(|obtype| const_ref_addr(graph, &obtype, 8)) - else { - return 0; - }; - let w_class_value = store_value(graph, &header, "w_class"); - let w_class = w_class_value - .as_ref() - .and_then(|value| get_instantiate_arg_addr(graph, value, 8)); - if w_class != Some(vtable) { + let mut roots = Vec::new(); + if !store_roots(graph, &header, 8, &mut roots) { return 0; } - vtable + let mut resolved: Option = None; + for root in &roots { + let Some(vtable) = store_value(graph, root, "ob_type") + .and_then(|obtype| const_ref_addr(graph, &obtype, 8)) + else { + return 0; + }; + let w_class = store_value(graph, root, "w_class") + .and_then(|value| get_instantiate_arg_addr(graph, &value, 8)); + if w_class != Some(vtable) { + return 0; + } + match resolved { + None => resolved = Some(vtable), + Some(seen) if seen == vtable => {} + // Predecessors building headers for different types merge into + // one malloc: no single vtable stands for the whole cluster. + Some(_) => return 0, + } + } + resolved.unwrap_or(0) }; struct Payload { @@ -3126,7 +3192,27 @@ pub fn fuse_boxing_alloc( continue; } let Some(result) = &op.result else { continue }; - let agg = &args[0]; + // The aggregate itself can reach the malloc as a `Block.inputargs` + // phi, not only its header: a constructor that builds the struct up + // front and then branches — `w_float_new` builds the `W_FloatObject` + // before `gc_interp::enabled()` and `try_gc_alloc_stable_raw`, each + // of which ends a block — leaves the ctor in a dominator. Every + // lookup below keys on `%agg` by exact variable, so resolve the phi + // to the ctor it stands for before asking any of them. A merge of + // two different aggregates has no single owner or payload set, and + // declines. Using the root also keeps `Site.aggregate` naming the + // variable a `core::ptr::write` arm stores, which is what + // `sink_fused_boxing_aggregates_at_raw_writes` matches on. + let mut agg_roots = Vec::new(); + if !store_roots(graph, &args[0], 8, &mut agg_roots) { + continue; + } + let Some((agg, other_roots)) = agg_roots.split_first() else { + continue; + }; + if other_roots.iter().any(|root| root != agg) { + continue; + } // `%agg` must be a `SyntheticTransparentCtor` for a known boxing // struct. Search graph-wide: the ctor and the malloc call land in // the same block, but the field stores feeding the aggregate may sit @@ -7280,6 +7366,472 @@ mod tests { } } + #[test] + fn fuse_boxing_alloc_resolves_an_aggregate_that_crosses_a_link() { + // The *aggregate* crosses the boundary, not only its header. + // `w_float_new` builds the whole `W_FloatObject` up front and then + // branches: `gc_interp::enabled()` and `try_gc_alloc_stable_raw` each + // end a block, so the ctor stays in a dominator while the malloc sees + // a `Block.inputargs` phi. Owner, payload stores and `ob_header` are + // all looked up by exact `%agg`, so without resolving the phi first + // the cluster declines for want of a ctor -- which is why the + // canonical float constructor never fused in production. + // + // The other arm stores the same aggregate through `core::ptr::write`. + // Fusing the malloc must not damage it: the write has to keep reading + // a fully constructed object. (Sinking that materialization to the + // write is an optimisation `sink_fused_boxing_aggregates_at_raw_writes` + // performs when it can match the variable; it is not required for + // correctness, so this asserts the object, not the sinking.) + type Var = crate::flowspace::model::Variable; + const FLOAT_TYPE_ADDR: i64 = 4357049520; + + 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() + } + fn field(base: &Var, name: &str, owner: &str, value: &Var) -> OpKind { + 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), + } + } + fn ctor(graph: &mut FunctionGraph, blk: BlockId, name: &str) -> Var { + 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 mut graph = FunctionGraph::new("test"); + let entry = graph.startblock; + + // The whole cluster, built in the dominator. + let cast = |graph: &mut FunctionGraph| { + let ty = graph + .push_op_var(entry, OpKind::ConstRefAddr(FLOAT_TYPE_ADDR), true) + .unwrap(); + call(graph, entry, &["__pyre_cast_instance", "PyType"], vec![ty]) + }; + let ob_type = cast(&mut graph); + let w_class_cast = cast(&mut graph); + let w_class = call( + &mut graph, + entry, + &["pyre_object", "pyobject", "get_instantiate"], + vec![w_class_cast], + ); + let header = ctor(&mut graph, entry, "PyObject"); + graph.push_op_var( + entry, + field(&header, "ob_type", "PyObject", &ob_type), + false, + ); + graph.push_op_var( + entry, + field(&header, "w_class", "PyObject", &w_class), + false, + ); + let payload = graph + .push_op_var(entry, OpKind::ConstFloat(0.0f64.to_bits()), true) + .unwrap(); + let agg = ctor(&mut graph, entry, "W_FloatObject"); + graph.push_op_var( + entry, + field(&agg, "ob_header", "W_FloatObject", &header), + false, + ); + graph.push_op_var( + entry, + field(&agg, "floatval", "W_FloatObject", &payload), + false, + ); + + // `gc_interp::enabled()` ends the dominator; the aggregate crosses. + let (probe, probe_args) = graph.create_block_with_arg_vars(1); + graph.set_goto(entry, probe, vec![agg.clone()]); + + // `try_gc_alloc_stable_raw` ends the next block, then the branch. + let raw = call( + &mut graph, + probe, + &["pyre_object", "gc_hook", "try_gc_alloc_stable_raw"], + vec![], + ); + let (gc_arm, gc_args) = graph.create_block_with_arg_vars(2); + let (plain_arm, plain_args) = graph.create_block_with_arg_vars(1); + graph.block_mut(probe).exitswitch = Some(ExitSwitch::Value(raw.clone())); + graph.closeblock( + probe, + vec![ + Link::from_variables( + &graph, + vec![probe_args[0].clone(), raw.clone()], + gc_arm, + Some(ExitCase::Bool(true)), + ), + Link::from_variables( + &graph, + vec![probe_args[0].clone()], + plain_arm, + Some(ExitCase::Bool(false)), + ), + ], + ); + + // GC arm: write the aggregate into the raw allocation. + graph.push_op_var( + gc_arm, + OpKind::Call { + target: CallTarget::FunctionPath { + segments: ["core", "ptr", "write"] + .iter() + .map(|s| s.to_string()) + .collect(), + }, + args: vec![gc_args[1].clone(), gc_args[0].clone()], + result_ty: ValueType::Void, + }, + false, + ); + graph.set_return(gc_arm, Some(gc_args[1].clone())); + + // Ordinary arm: the `malloc_typed` the fusion rewrites. + let ret = call( + &mut graph, + plain_arm, + &["pyre_object", "lltype", "malloc_typed"], + vec![plain_args[0].clone()], + ); + graph.set_return(plain_arm, Some(ret)); + + assert_eq!( + fuse_boxing_alloc(&mut graph, &numeric_boxing_attrs()), + 1, + "the aggregate's phi must resolve to its ctor" + ); + + let vtables: Vec = graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .filter_map(|op| match &op.kind { + OpKind::NewWithVtable { vtable, .. } => Some(*vtable), + _ => None, + }) + .collect(); + assert_eq!(vtables, vec![FLOAT_TYPE_ADDR], "wrong vtable stamped"); + assert!( + !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") + ) + }), + "the fused cluster must leave no residual malloc_typed" + ); + + // The GC arm must still store a fully constructed object: whatever the + // write reads has to reach a `W_FloatObject` ctor carrying its header. + let written = graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .find_map(|op| match &op.kind { + OpKind::Call { + target: CallTarget::FunctionPath { segments }, + args, + .. + } if segments + .iter() + .map(String::as_str) + .eq(["core", "ptr", "write"]) => + { + Some(args[1].clone()) + } + _ => None, + }) + .expect("the raw write must survive the rewrite"); + // Follow the links the way the pass does, then confirm the ctor and + // its header store are both still present for that root. + let mut roots: Vec = Vec::new(); + let mut frontier = vec![written]; + while let Some(var) = frontier.pop() { + if graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .any(|o| o.result.as_ref() == Some(&var)) + { + roots.push(var); + continue; + } + let Some((target, slot)) = graph.blocks.iter().find_map(|b| { + b.inputargs + .iter() + .position(|arg| arg == &var) + .map(|slot| (b.id, slot)) + }) else { + panic!("the written value is neither an op result nor an inputarg"); + }; + for b in &graph.blocks { + for link in &b.exits { + if link.target == target { + frontier + .extend(link.args.get(slot).and_then(LinkArg::as_variable).cloned()); + } + } + } + } + assert!(!roots.is_empty(), "the written value resolves to nothing"); + for root in &roots { + assert!( + graph.blocks.iter().flat_map(|b| &b.operations).any(|o| { + o.result.as_ref() == Some(root) + && matches!( + &o.kind, + OpKind::Call { target: CallTarget::SyntheticTransparentCtor { name, .. }, .. } + if name == "W_FloatObject" + ) + }), + "the raw write must still read a W_FloatObject ctor" + ); + assert!( + graph.blocks.iter().flat_map(|b| &b.operations).any(|o| { + matches!(&o.kind, OpKind::FieldWrite { base, field, .. } + if base == root && field.name.as_str() == "ob_header") + }), + "the written aggregate must keep its ob_header store" + ); + } + } + + #[test] + fn fuse_boxing_alloc_resolves_a_header_that_crosses_a_link() { + // Here the *header itself* crosses the boundary, not just the two + // values it stores. That is the shape every constructor with a + // preceding call leaves behind — the `PyObject` ctor and its + // `ob_type` / `w_class` stores stay in the predecessor while the + // header arrives at the `malloc_typed` as a `Block.inputargs` phi. A + // field store is recorded against the variable the producing + // operation wrote, so asking the phi for `ob_type` finds nothing and + // the cluster declines however constant its type pointer is; the + // lookup has to resolve the phi back to its roots first. Roots + // reached through a merge must agree, for the reason `resolve_addr` + // requires it of a merged pointer: no single vtable stands for a + // header that is one of two types. + 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() + } + fn field(base: &Var, name: &str, owner: &str, value: &Var) -> OpKind { + 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), + } + } + fn ctor(graph: &mut FunctionGraph, blk: BlockId, name: &str) -> Var { + graph + .push_op_var( + blk, + OpKind::Call { + target: CallTarget::synthetic_transparent_ctor(name), + args: vec![], + result_ty: ValueType::Ref(Some(name.into())), + }, + true, + ) + .unwrap() + } + /// The nested `PyObject` header, built entirely in `blk`. + fn header_in(graph: &mut FunctionGraph, blk: BlockId, addr: i64) -> Var { + 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], + ); + let header = ctor(graph, blk, "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); + header + } + /// The outer object and its `malloc_typed`, taking `header` from + /// wherever it was built. + fn outer_in(graph: &mut FunctionGraph, blk: BlockId, header: &Var) { + let payload = graph + .push_op_var(blk, OpKind::ConstFloat(0.0f64.to_bits()), true) + .unwrap(); + let agg = ctor(graph, blk, "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)); + } + + /// The header built once, then relayed across `crossings` blocks + /// before the object that stores it — a run of calls before the + /// allocation leaves exactly this. + fn chain(crossings: usize) -> FunctionGraph { + let mut graph = FunctionGraph::new("test"); + let entry = graph.startblock; + let mut carried = vec![header_in(&mut graph, entry, FLOAT_TYPE_ADDR)]; + let mut from = entry; + for _ in 0..crossings { + let (next, args) = graph.create_block_with_arg_vars(1); + graph.set_goto(from, next, carried); + carried = args; + from = next; + } + outer_in(&mut graph, from, &carried[0]); + graph + } + /// Two predecessors each building their own header, and the object + /// storing whichever one the merge carried. + 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(1); + let arms: Vec = [(true, left_addr), (false, right_addr)] + .into_iter() + .map(|(case, addr)| { + let arm = graph.create_block(); + let header = header_in(&mut graph, arm, addr); + graph.set_goto(arm, join, vec![header]); + Link::from_variables(&graph, vec![], arm, Some(ExitCase::Bool(case))) + }) + .collect(); + graph.block_mut(entry).exitswitch = Some(ExitSwitch::Value(cond)); + graph.closeblock(entry, arms); + outer_in(&mut graph, join, &carried[0]); + graph + } + + let rows: [(&str, &dyn Fn() -> FunctionGraph, usize); 4] = [ + ("header crosses one link", &|| chain(1), 1), + ("header crosses two links", &|| chain(2), 1), + ( + "merged headers naming one type", + &|| merge(FLOAT_TYPE_ADDR, FLOAT_TYPE_ADDR), + 1, + ), + ( + "merged headers 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 separates a walk that reached the header's + // own roots 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/majit/majit-translate/src/translator/rtyper/flowspace_adapter.rs b/majit/majit-translate/src/translator/rtyper/flowspace_adapter.rs index 47f3b31d7b3..e2a705a21e1 100644 --- a/majit/majit-translate/src/translator/rtyper/flowspace_adapter.rs +++ b/majit/majit-translate/src/translator/rtyper/flowspace_adapter.rs @@ -2046,12 +2046,16 @@ pub fn translate_op( return Ok(vec![FlowspaceOp::new(opname, arg_hls, result)]); } } - // Fail-closed on an UNFUSED `lltype::malloc_typed`. Only the - // numeric boxing structs recognised by `fuse_boxing_alloc` - // (`model.rs payload_fields` — `W_FloatObject` / `W_IntObject` - // / `W_ComplexObject` / `W_LongObject`) get a `NewWithVtable` - // lowering before the rtyper runs; every other mallocable GC - // struct's `malloc_typed` survives to here with no ported + // Fail-closed on an UNFUSED `lltype::malloc_typed`. A boxing + // struct gets its `NewWithVtable` lowering before the rtyper + // runs only where `fuse_boxing_alloc` (`model.rs`) could + // resolve the cluster's header: a constant `ob_type` + // type-pointer whose `w_class` is `get_instantiate` of that + // same type. A cluster it declined — no `ob_header` field at + // all, a type-pointer that is not a constant, or a `w_class` + // naming a different type, which is a subclass construction + // (`w_long_from_raw` pairs `&LONG_TYPE` with `&INT_TYPE`) — + // survives to here as a call, with no ported // `jtransform.rewrite_op_malloc` general path // (`jtransform.py:1012`). Layer-1 misses it (cutover skips // registering `malloc_typed`), so it would resolve to the @@ -2071,9 +2075,9 @@ pub fn translate_op( { return Err(TyperError::message( "`lltype::malloc_typed[_managed]` survived fuse_boxing_alloc unfused; \ - only the numeric boxing structs fuse_boxing_alloc rewrites \ - (W_Float/W_Int/W_Complex/W_Long) have a NewWithVtable \ - lowering; no general malloc->new lowering ported" + the cluster's header did not resolve to a constant type \ + pointer standing for its own `w_class`, and no general \ + malloc->new lowering is ported" .to_string(), )); }