From 01c106a35a19c960d4be29ca013add1193d4190e Mon Sep 17 00:00:00 2001 From: darmie Date: Sun, 27 Sep 2026 15:45:38 +0000 Subject: [PATCH] LICM and load CSE: two roots are two objects only when their provenance says so A parameter, a phi, a loaded pointer or a call's result may name the same object as any other such root, in any function, not only in an outlined region. Only allocations the function makes, globals, and objects of two different exact struct types are kept apart. git-bug: 96fe0b9665e4a9476eaf1ff4421ab1f54b1417ca6323e3ba3a820ee6dcdf7bcc --- crates/compiler/src/licm.rs | 167 ++++++++++++++---- .../tests/cranelift_first_call.rs | 8 + .../one_object_passed_twice.expected | 2 + .../one_object_passed_twice.py | 18 ++ 4 files changed, 165 insertions(+), 30 deletions(-) create mode 100644 crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.expected create mode 100644 crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.py diff --git a/crates/compiler/src/licm.rs b/crates/compiler/src/licm.rs index d8003175..e056fe9d 100644 --- a/crates/compiler/src/licm.rs +++ b/crates/compiler/src/licm.rs @@ -666,44 +666,52 @@ pub(crate) struct MemLoc { root: Option, offset: Option, size: u32, - /// The root is a parameter of an outlined loop region: a pointer - /// the region's function had live at the header, which may name - /// the same object as any other root. - shared_root: bool, - /// The root is a module global: storage the compiler owns, which - /// no object pointer handed to a region points into. - global_root: bool, + /// Where the root comes from, which decides what it may alias. + root_kind: RootKind, /// The root's pointee type when it is an exact struct type: the /// root points at an object of that type. exact_type: Option, } +/// Where a location's root pointer comes from. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RootKind { + /// An allocation this function made: no other root names it until + /// it is stored and read back, and a read back is `Unknown`. + Fresh, + /// A parameter: an object that existed before the function began, + /// so never one it allocates. + Param, + /// A module global: storage the compiler owns, which no object + /// pointer points into. + Global, + /// A phi, a loaded pointer, a call's result or anything else: it + /// may name the same object as any root but a global. + Unknown, +} + impl MemLoc { /// May the regions described by `self` and `other` overlap? /// /// Returns `true` conservatively when we can't prove they don't. - /// The fast win: when both sides have a *known root* and the - /// roots differ, we rely on the SSA no-aliasing assumption - /// (distinct pointer SSA values refer to distinct memory) — that - /// case returns `false` even if one side has an unknown offset - /// or unknown size, because the roots already establish disjoint - /// allocations. Only when roots match (or one is unknown) do we - /// fall back to byte-range comparison. + /// Two different roots are disjoint when they point at objects of + /// two different exact struct types, or when their [`RootKind`]s + /// prove two objects; then the offsets do not matter. One root is + /// compared by byte range. pub(crate) fn may_alias(&self, other: &MemLoc) -> bool { if self.distinct_exact_objects(other) { return false; } match (self.root, other.root) { - (Some(r1), Some(r2)) if r1 != r2 && (self.shared_root || other.shared_root) => { - !(self.global_root || other.global_root) - } (Some(r1), Some(r2)) if r1 != r2 => { - // Different SSA roots: rely on the SSA-level - // no-aliasing assumption. Holds for the patterns ZynML - // emits — each malloc / alloca produces a fresh root, - // table reads through different indices produce - // different SSA values, etc. - false + use RootKind::*; + // Two different roots are two objects only when their + // provenance says so; two parameters, or a phi and a + // parameter, may be one object passed twice. + !matches!( + (self.root_kind, other.root_kind), + (Global, _) | (_, Global) | (Fresh, Fresh) | (Fresh, Param) | (Param, Fresh) + ) } (None, _) | (_, None) => { // At least one side's root is opaque; can't reason. @@ -763,6 +771,8 @@ pub(crate) enum AddrLink { }, /// A pointer read out of a struct field — chase to the aggregate. Field { aggregate: HirId, index: u32 }, + /// An allocation this function makes: a root, and a fresh object. + Fresh, } /// Build a `HirId → AddrLink` lookup over every `Cast` / `GEP` @@ -779,6 +789,14 @@ pub(crate) fn build_addr_index( for block in func.blocks.values() { for inst in &block.instructions { match inst { + HirInstruction::Alloca { result, .. } + | HirInstruction::Call { + result: Some(result), + callee: HirCallable::Intrinsic(crate::hir::Intrinsic::Malloc), + .. + } => { + idx.insert(*result, AddrLink::Fresh); + } HirInstruction::Cast { result, operand, .. } => { @@ -908,14 +926,16 @@ pub(crate) fn extract_mem_loc( } current = identity_subst.get(base).copied().unwrap_or(*base); } - None => break, + Some(AddrLink::Fresh) | None => break, } } let root_value = func.values.get(¤t); - let root_kind = root_value.map(|v| &v.kind); - let shared_root = func.attributes.osr_region - && matches!(root_kind, Some(crate::hir::HirValueKind::Parameter(_))); - let global_root = matches!(root_kind, Some(crate::hir::HirValueKind::Global(_))); + let root_kind = match root_value.map(|v| &v.kind) { + Some(crate::hir::HirValueKind::Parameter(_)) => RootKind::Param, + Some(crate::hir::HirValueKind::Global(_)) => RootKind::Global, + _ if matches!(addr_index.get(¤t), Some(AddrLink::Fresh)) => RootKind::Fresh, + _ => RootKind::Unknown, + }; let exact_type = match root_value.map(|v| &v.ty) { Some(HirType::Ptr(inner)) => match &**inner { HirType::Struct(s) => s.name.filter(|n| exact.contains(n)), @@ -931,8 +951,7 @@ pub(crate) fn extract_mem_loc( None }, size, - shared_root, - global_root, + root_kind, exact_type, } } @@ -1557,6 +1576,94 @@ mod tests { ); } + /// A loop storing through `stored` and loading through `loaded`, + /// both `*i64`. Whether the load left the loop. + fn hoists_past_store( + make: impl FnOnce(&mut HirFunction, HirId, HirType) -> (HirId, HirId), + ) -> bool { + let (mut f, entry, _header, body, _exit) = mk_func(); + let ptr_ty = HirType::Ptr(Box::new(HirType::I64)); + let (stored, loaded) = make(&mut f, entry, ptr_ty); + let v = add_param(&mut f, HirType::I64, 9); + let r = add_inst(&mut f, HirType::I64); + let b = f.blocks.get_mut(&body).unwrap(); + b.instructions.push(HirInstruction::Store { + value: v, + ptr: stored, + align: 8, + volatile: false, + }); + b.instructions.push(HirInstruction::Load { + result: r, + ty: HirType::I64, + ptr: loaded, + align: 8, + volatile: false, + }); + run(&mut f); + !f.blocks[&body] + .instructions + .iter() + .any(|inst| matches!(inst, HirInstruction::Load { result, .. } if *result == r)) + } + + /// Two pointer parameters of any function may name one object, as + /// a call passing it twice does: a read through one is not free of + /// a store through the other. + #[test] + fn parameters_may_alias() { + assert!(!hoists_past_store(|f, _, ty| { + (add_param(f, ty.clone(), 0), add_param(f, ty, 1)) + })); + } + + /// A pointer read from memory may name what a parameter names. + #[test] + fn a_loaded_pointer_may_alias_a_parameter() { + assert!(!hoists_past_store(|f, entry, ty| { + let param = add_param(f, ty.clone(), 0); + let slot = add_param(f, HirType::Ptr(Box::new(ty.clone())), 1); + let loaded = add_inst(f, ty); + f.blocks + .get_mut(&entry) + .unwrap() + .instructions + .push(HirInstruction::Load { + result: loaded, + ty: HirType::Ptr(Box::new(HirType::I64)), + ptr: slot, + align: 8, + volatile: false, + }); + (param, loaded) + })); + } + + /// An allocation the function makes is not an object its caller + /// passed in: a read through a parameter leaves the loop past a + /// store to it. + #[test] + fn a_parameter_is_not_an_allocation_the_function_makes() { + assert!(hoists_past_store(|f, entry, ty| { + let param = add_param(f, ty.clone(), 0); + let size = add_param(f, HirType::I64, 1); + let fresh = add_inst(f, ty); + f.blocks + .get_mut(&entry) + .unwrap() + .instructions + .push(HirInstruction::Call { + result: Some(fresh), + callee: HirCallable::Intrinsic(crate::hir::Intrinsic::Malloc), + args: vec![size], + type_args: vec![], + const_args: vec![], + is_tail: false, + }); + (fresh, param) + })); + } + /// A global's storage is the compiler's own: a store through a /// region's pointer parameter cannot reach it, so a read of the /// global leaves the loop. diff --git a/crates/zyntax_python/tests/cranelift_first_call.rs b/crates/zyntax_python/tests/cranelift_first_call.rs index f944834c..9743f947 100644 --- a/crates/zyntax_python/tests/cranelift_first_call.rs +++ b/crates/zyntax_python/tests/cranelift_first_call.rs @@ -80,3 +80,11 @@ fn scimark_lu_factor_on_boxed_rows() { run("lu_scimark_dynamic", false); run("lu_scimark_dynamic", true); } + +/// A loop writing a field through one parameter and reading it through +/// another, called with one object for both. +#[test] +fn one_object_passed_as_two_parameters() { + run("one_object_passed_twice", false); + run("one_object_passed_twice", true); +} diff --git a/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.expected b/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.expected new file mode 100644 index 00000000..737a0eaa --- /dev/null +++ b/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.expected @@ -0,0 +1,2 @@ +503500 +10030 diff --git a/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.py b/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.py new file mode 100644 index 00000000..5c6fd553 --- /dev/null +++ b/crates/zyntax_python/tests/fixtures/cranelift_first_call/one_object_passed_twice.py @@ -0,0 +1,18 @@ +# One object passed as two parameters: a loop reading a field through +# one and writing it through the other sees every write. +class A: + def __init__(self, x): + self.x = x + + +def f(a, b, n): + s = 0 + for i in range(n): + b.x = b.x + 1 + s += a.x + return s + + +a = A(3) +print(f(a, a, 1000)) +print(f(a, A(0), 10))