From 948ad3e38393b010a2bde9bdc46f8ae438864c93 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 16:49:26 +0300 Subject: [PATCH 01/19] fix(codegen): prune spills by reachability, not dominance The spills analysis deliberately places spills per path, e.g. one along each arm of a join, so a reload placed after the join is covered by the set of arm spills, none of which individually dominates it. The spill pruning in rewrite_spill_pseudo_instructions kept a spill only when it dominated a live reload, so it erased every covering spill of such a reload while the reload itself survived, and reload materialization then panicked looking up a procedure local that was never allocated. Had the value also owned a second, dominated reload, one spill would survive and uncovered paths would silently read a stale local instead. Replace the dominance predicate with reachability: a spill is elided only when no control-flow path from it can reach a live reload of its value. Operations are first normalized to their ancestors in the innermost common region, so spills nested in structured control flow (which have no CFG successors) are still handled, and every case that cannot be reasoned about conservatively keeps the spill. This preserves dead edge-spill elimination, since a consistency spill whose only reload sits in a sibling arm reaches nothing and is still erased, and it makes the previously unpruned single-block path use the same criterion. The now-unused dominfo parameter is dropped, and convert_reload_to_load reports a descriptive error instead of panicking if a live reload ever loses all covering spills again. Add an end-to-end regression test in which both arms of a diamond spill the same value and the sole reload lands at the join, plus unit tests pinning spill_reaches_reload (now exported) across diamond, loop back-edge, and nested scf.if shapes. Existing spill fixtures are unchanged. --- dialects/hir/src/transforms/spill.rs | 9 +- ...lizes_spills_join_covered_reload_after.hir | 31 ++ ...izes_spills_join_covered_reload_before.hir | 28 ++ dialects/hir/src/transforms/spill/tests.rs | 305 +++++++++++++++++- hir-transform/src/lib.rs | 4 +- hir-transform/src/spill.rs | 86 +++-- 6 files changed, 437 insertions(+), 26 deletions(-) create mode 100644 dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir create mode 100644 dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir diff --git a/dialects/hir/src/transforms/spill.rs b/dialects/hir/src/transforms/spill.rs index b65c9cac1..c2196f00d 100644 --- a/dialects/hir/src/transforms/spill.rs +++ b/dialects/hir/src/transforms/spill.rs @@ -1,4 +1,4 @@ -use alloc::rc::Rc; +use alloc::{format, rc::Rc}; use midenc_hir::{ BlockRef, BuilderExt, EntityMut, Op, OpBuilder, OperationName, OperationRef, Report, Rewriter, @@ -154,7 +154,12 @@ impl TransformSpillsInterface for TransformSpillsImpl { use crate::HirOpBuilder; let spilled = reload.borrow().as_trait::().unwrap().spilled_value(); - let local = self.locals[&spilled]; + let Some(local) = self.locals.get(&spilled).copied() else { + return Err(Report::msg(format!( + "live reload of {spilled} has no corresponding spill: every kept reload must be \ + covered by at least one spill of the same value" + ))); + }; let reloaded = rewriter.load_local(local, reload.span())?; rewriter.replace_op_with_values(reload, &[Some(reloaded)]); diff --git a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir new file mode 100644 index 000000000..7b1167848 --- /dev/null +++ b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir @@ -0,0 +1,31 @@ +builtin.function public extern("C") @materializes_spills_join_covered_reload(%0: ptr) -> u32 { + %1 = hir.ptr_to_int %0 <{ ty = #builtin.type }>; + %2 = arith.constant 32 : u32; + %3 = arith.add %1, %2 <{ overflow = #builtin.overflow }>; + %4 = hir.int_to_ptr %3 <{ ty = #builtin.type> }>; + %5 = hir.load %4; + %6 = arith.constant 64 : u32; + %7 = arith.add %1, %6 <{ overflow = #builtin.overflow }>; + %8 = hir.int_to_ptr %7 <{ ty = #builtin.type> }>; + %9 = hir.load %8; + %10 = arith.constant 0 : u32; + %11 = arith.eq %1, %10; + cf.cond_br %11 ^block2, ^block3 : (i1); +^block2: + %12 = arith.constant 1 : u64; + hir.store_local %3 <{ local = #builtin.local_variable<0, u32> }> : (u32); + %13 = hir.exec ::@test::@example(%8, %5, %9, %9, %12) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^block4(%13 : u32); +^block3: + %14 = arith.constant 1 : u64; + hir.store_local %3 <{ local = #builtin.local_variable<0, u32> }> : (u32); + %15 = hir.exec ::@test::@example(%8, %5, %9, %9, %14) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^block4(%15 : u32); +^block4(%16: u32): + %17 = arith.constant 5 : u32; + %18 = arith.add %1, %17 <{ overflow = #builtin.overflow }>; + %19 = arith.add %18, %16 <{ overflow = #builtin.overflow }>; + %22 = hir.load_local <{ local = #builtin.local_variable<0, u32> }>; + %20 = arith.add %19, %22 <{ overflow = #builtin.overflow }>; + builtin.ret %20 : (u32); +}; \ No newline at end of file diff --git a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir new file mode 100644 index 000000000..4bfe18825 --- /dev/null +++ b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir @@ -0,0 +1,28 @@ +builtin.function public extern("C") @materializes_spills_join_covered_reload(%0: ptr) -> u32 { + %1 = hir.ptr_to_int %0 <{ ty = #builtin.type }>; + %2 = arith.constant 32 : u32; + %3 = arith.add %1, %2 <{ overflow = #builtin.overflow }>; + %4 = hir.int_to_ptr %3 <{ ty = #builtin.type> }>; + %5 = hir.load %4; + %6 = arith.constant 64 : u32; + %7 = arith.add %1, %6 <{ overflow = #builtin.overflow }>; + %8 = hir.int_to_ptr %7 <{ ty = #builtin.type> }>; + %9 = hir.load %8; + %10 = arith.constant 0 : u32; + %11 = arith.eq %1, %10; + cf.cond_br %11 ^block2, ^block3 : (i1); +^block2: + %12 = arith.constant 1 : u64; + %13 = hir.exec ::@test::@example(%8, %5, %9, %9, %12) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^block4(%13 : u32); +^block3: + %14 = arith.constant 1 : u64; + %15 = hir.exec ::@test::@example(%8, %5, %9, %9, %14) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^block4(%15 : u32); +^block4(%16: u32): + %17 = arith.constant 5 : u32; + %18 = arith.add %1, %17 <{ overflow = #builtin.overflow }>; + %19 = arith.add %18, %16 <{ overflow = #builtin.overflow }>; + %20 = arith.add %19, %3 <{ overflow = #builtin.overflow }>; + builtin.ret %20 : (u32); +}; \ No newline at end of file diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index a775fd564..6c3797190 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -6,14 +6,20 @@ use midenc_dialect_cf::ControlFlowOpBuilder as Cf; use midenc_dialect_scf::StructuredControlFlowOpBuilder; use midenc_expect_test::expect_file; use midenc_hir::{ - AddressSpace, Builder, Op, PointerType, ProgramPoint, Report, SourceSpan, Type, ValueRef, - dialects::builtin::BuiltinOpBuilder, testing::Test, + AddressSpace, Builder, Op, OperationRef, PointerType, ProgramPoint, Report, SourceSpan, Type, + ValueRef, dialects::builtin::BuiltinOpBuilder, testing::Test, }; +use midenc_hir_transform::spill_reaches_reload; use crate::{HirOpBuilder, transforms::TransformSpills}; type TestResult = Result; +/// Returns the defining operation of `value`. +fn defining_op(value: ValueRef) -> OperationRef { + value.borrow().get_defining_op().expect("expected value to have a defining op") +} + /// Build a simple single-block function which triggers spills and reloads, /// then run the `TransformSpills` pass and check that spills/reloads are /// materialized as `hir.store_local`/`hir.load_local`. @@ -255,6 +261,134 @@ fn materializes_spills_branching_cfg() -> TestResult<()> { Ok(()) } +/// Build a branching CFG in which *both* arms spill the same value (each contains a copy of the +/// high-pressure call), while the only use of that value after the spills lies *after* the join. +/// +/// The spills analysis places one spill per arm, and the single reload lands at the join, so the +/// reload is covered by the *set* of spills, none of which individually dominates it. This is a +/// regression test for spill pruning: pruning spills that do not dominate a live reload erased +/// both arm spills while keeping the reload, and reload materialization then panicked because no +/// procedure local was ever allocated for the spilled value. +/// +/// Operand stack pressure at each call (the analysis models pressure in felts with `K = 16`): +/// the call arguments require 15 felts (`v6: ptr` = 1, `v4: u128` = 4, `v7: u128` passed twice +/// = 8, `u64` = 2), and `v1: u32` and `v2: u32` are live across both calls (used only in the +/// join block), for a total of `15 + 2 = 17 > K`, so exactly one of `{v1, v2}` is spilled in +/// each arm. +#[test] +fn materializes_spills_join_covered_reload() -> TestResult<()> { + let mut test = Test::named("materializes_spills_join_covered_reload").in_module("test"); + let span = SourceSpan::UNKNOWN; + + test.with_function( + test.name(), + &[Type::Ptr(Arc::new(PointerType::new_with_address_space( + Type::U8, + AddressSpace::Element, + )))], + &[Type::U32], + ); + let func = test.function(); + + let callee = test.define_function( + "example", + &[ + Type::Ptr(Arc::new(PointerType::new_with_address_space( + Type::U128, + AddressSpace::Element, + ))), + Type::U128, + Type::U128, + Type::U128, + Type::U64, + ], + &[Type::U32], + ); + + { + let mut b = test.function_builder(); + let entry = b.current_block(); + let v0 = entry.borrow().arguments()[0] as ValueRef; + let v1 = b.ptrtoint(v0, Type::U32, span)?; + let k32 = b.u32(32, span); + let v2 = b.add_unchecked(v1, k32, span)?; + let v3 = b.inttoptr( + v2, + Type::Ptr(Arc::new(PointerType::new_with_address_space( + Type::U128, + AddressSpace::Element, + ))), + span, + )?; + let v4 = b.load(v3, span)?; + let k64 = b.u32(64, span); + let v5 = b.add_unchecked(v1, k64, span)?; + let v6 = b.inttoptr( + v5, + Type::Ptr(Arc::new(PointerType::new_with_address_space( + Type::U128, + AddressSpace::Element, + ))), + span, + )?; + let v7 = b.load(v6, span)?; + let zero = b.u32(0, span); + let v8 = b.eq(v1, zero, span)?; + let t = b.create_block(); + let f = b.create_block(); + let join = b.create_block(); + Cf::cond_br(&mut b, v8, t, [], f, [], span)?; + + let callee_sig = callee.borrow().get_signature().clone(); + + // then: the high-pressure call forces a spill on this path + b.switch_to_block(t); + let v9 = b.u64(1, span); + let call = b.exec(callee, callee_sig.clone(), [v6, v4, v7, v7, v9], span)?; + let v10 = call.borrow().results()[0] as ValueRef; + b.br(join, [v10], span)?; + + // else: the same call, so the same value is spilled on this path as well + b.switch_to_block(f); + let v11 = b.u64(1, span); + let call2 = b.exec(callee, callee_sig, [v6, v4, v7, v7, v11], span)?; + let v12 = call2.borrow().results()[0] as ValueRef; + b.br(join, [v12], span)?; + + // join: the only uses of the values live across the calls + let v13 = b.append_block_param(join, Type::U32, span); + b.switch_to_block(join); + let k5 = b.u32(5, span); + let v14 = b.add_unchecked(v1, k5, span)?; + let v15 = b.add_unchecked(v14, v13, span)?; + let v16 = b.add_unchecked(v15, v2, span)?; + b.ret(Some(v16), span)?; + } + + let before = func.as_operation_ref().borrow().to_string(); + let before_file = format!("expected/{}_before.hir", test.name()); + expect_file![&before_file].assert_eq(&before); + + test.apply_pass::(false)?; + + let after = func.as_operation_ref().borrow().to_string(); + let after_file = format!("expected/{}_after.hir", test.name()); + expect_file![&after_file].assert_eq(&after); + + // The spilled value must be stored once per arm, and reloaded once at the join + let stores = after.lines().filter(|l| l.trim_start().starts_with("hir.store_local ")).count(); + let loads = after + .lines() + .filter(|l| { + l.trim_start().contains("= hir.load_local ") + || l.trim_start().starts_with("hir.load_local ") + }) + .count(); + assert!(stores == 2, "expected one store_local op in each arm\n{after}"); + assert!(loads == 1, "expected one load_local op at the join\n{after}"); + Ok(()) +} + /// Build a small multi-block CFG containing a `scf.if`, where spilled values are only used inside /// the nested regions of the `scf.if`. /// @@ -573,3 +707,170 @@ fn materializes_spills_nested_scf_while_after_region() -> TestResult<()> { Ok(()) } + +/// Positional reachability over a diamond CFG. +/// +/// This pins the join-covered shape that dominance-based spill pruning got wrong: a reload after +/// the join is covered by the set of per-arm spills, none of which dominates it, so each arm must +/// count as reaching the join. Sibling arms must stay mutually unreachable so the dead-edge-spill +/// elimination keeps working. +#[test] +fn spill_reachability_in_branching_cfg() -> TestResult<()> { + let mut test = Test::named("spill_reachability_in_branching_cfg").in_module("test"); + let span = SourceSpan::UNKNOWN; + test.with_function(test.name(), &[Type::U32], &[Type::U32]); + + let mut b = test.function_builder(); + let entry = b.current_block(); + let v0 = entry.borrow().arguments()[0] as ValueRef; + let k1 = b.u32(1, span); + let entry_first = b.add_unchecked(v0, k1, span)?; + let cond = b.eq(entry_first, k1, span)?; + let left = b.create_block(); + let right = b.create_block(); + let join = b.create_block(); + Cf::cond_br(&mut b, cond, left, [], right, [], span)?; + + b.switch_to_block(left); + let left_value = b.add_unchecked(entry_first, k1, span)?; + b.br(join, [left_value], span)?; + + b.switch_to_block(right); + let right_value = b.add_unchecked(entry_first, entry_first, span)?; + b.br(join, [right_value], span)?; + + let join_arg = b.append_block_param(join, Type::U32, span); + b.switch_to_block(join); + let join_value = b.add_unchecked(join_arg, k1, span)?; + b.ret(Some(join_value), span)?; + + let entry_first = defining_op(entry_first); + let entry_second = defining_op(cond); + let left_op = defining_op(left_value); + let right_op = defining_op(right_value); + let join_op = defining_op(join_value); + + assert!(spill_reaches_reload(left_op, join_op), "arm must reach the join"); + assert!(spill_reaches_reload(right_op, join_op), "arm must reach the join"); + assert!( + !spill_reaches_reload(left_op, right_op), + "sibling arms must not reach each other" + ); + assert!( + spill_reaches_reload(entry_first, entry_second), + "earlier op must reach a later op in the same block" + ); + assert!( + !spill_reaches_reload(entry_second, entry_first), + "later op must not reach an earlier op without a cycle" + ); + assert!(!spill_reaches_reload(join_op, left_op), "join must not reach an arm"); + Ok(()) +} + +/// Positional reachability through a loop back-edge. +#[test] +fn spill_reachability_through_loop_back_edge() -> TestResult<()> { + let mut test = Test::named("spill_reachability_through_loop_back_edge").in_module("test"); + let span = SourceSpan::UNKNOWN; + test.with_function(test.name(), &[Type::U32], &[Type::U32]); + + let mut b = test.function_builder(); + let entry = b.current_block(); + let v0 = entry.borrow().arguments()[0] as ValueRef; + let header = b.create_block(); + let body = b.create_block(); + let exit = b.create_block(); + b.br(header, [v0], span)?; + + let header_arg = b.append_block_param(header, Type::U32, span); + b.switch_to_block(header); + let k1 = b.u32(1, span); + let header_value = b.add_unchecked(header_arg, k1, span)?; + let cond = b.eq(header_value, k1, span)?; + Cf::cond_br(&mut b, cond, body, [], exit, [], span)?; + + b.switch_to_block(body); + let body_value = b.add_unchecked(header_value, k1, span)?; + b.br(header, [body_value], span)?; + + b.switch_to_block(exit); + let exit_value = b.add_unchecked(header_value, header_value, span)?; + b.ret(Some(exit_value), span)?; + + let header_op = defining_op(header_value); + let header_second = defining_op(cond); + let body_op = defining_op(body_value); + let exit_op = defining_op(exit_value); + + assert!(spill_reaches_reload(body_op, header_op), "back edge must reach the header"); + assert!( + spill_reaches_reload(header_second, header_op), + "later op must reach an earlier op in the same block through the loop" + ); + assert!(!spill_reaches_reload(exit_op, body_op), "exit must not reach the loop body"); + Ok(()) +} + +/// Positional reachability with operations nested in `scf.if` regions. +/// +/// Nested operations are normalized to their ancestor in the common region; operations nested +/// under the same region-branch op conservatively reach each other (e.g. across loop iterations). +#[test] +fn spill_reachability_across_nested_regions() -> TestResult<()> { + let mut test = Test::named("spill_reachability_across_nested_regions").in_module("test"); + let span = SourceSpan::UNKNOWN; + test.with_function(test.name(), &[Type::U32], &[Type::U32]); + + let mut b = test.function_builder(); + let entry = b.current_block(); + let v0 = entry.borrow().arguments()[0] as ValueRef; + let k1 = b.u32(1, span); + let pre_value = b.add_unchecked(v0, k1, span)?; + let cond = b.eq(pre_value, k1, span)?; + + let mut if_op = b.r#if(cond, &[Type::U32], span)?; + let context = b.builder().context_rc(); + let (then_block, else_block) = (context.create_block(), context.create_block()); + { + let mut if_op = if_op.borrow_mut(); + if_op.then_body_mut().push_back(then_block); + if_op.else_body_mut().push_back(else_block); + } + + b.switch_to_block(then_block); + let then_value = b.add_unchecked(pre_value, k1, span)?; + b.r#yield([then_value], span)?; + + b.switch_to_block(else_block); + let else_value = b.add_unchecked(pre_value, pre_value, span)?; + b.r#yield([else_value], span)?; + + b.switch_to_block(entry); + let if_result = if_op.as_operation_ref().borrow().results()[0] as ValueRef; + let post_value = b.add_unchecked(if_result, k1, span)?; + b.ret(Some(post_value), span)?; + + let pre_op = defining_op(pre_value); + let then_op = defining_op(then_value); + let else_op = defining_op(else_value); + let post_op = defining_op(post_value); + + assert!( + spill_reaches_reload(pre_op, then_op), + "op before the region op must reach into it" + ); + assert!( + spill_reaches_reload(then_op, post_op), + "nested op must reach past the region op" + ); + assert!( + !spill_reaches_reload(post_op, then_op), + "op after the region op must not reach back into it" + ); + assert!( + spill_reaches_reload(then_op, else_op), + "sibling regions of one op conservatively reach each other" + ); + Ok(()) +} diff --git a/hir-transform/src/lib.rs b/hir-transform/src/lib.rs index 1394fd948..d7343f6cf 100644 --- a/hir-transform/src/lib.rs +++ b/hir-transform/src/lib.rs @@ -22,5 +22,7 @@ pub use self::{ cse::CommonSubexpressionElimination, sccp::SparseConditionalConstantPropagation, sink::{ControlFlowSink, SinkOperandDefs}, - spill::{ReloadLike, SpillLike, TransformSpillsInterface, transform_spills}, + spill::{ + ReloadLike, SpillLike, TransformSpillsInterface, spill_reaches_reload, transform_spills, + }, }; diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 74cad1ab0..63e1a8e98 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -454,7 +454,7 @@ fn rewrite_single_block_spills( } let context = { op.borrow().context_rc() }; - rewrite_spill_pseudo_instructions(context, analysis, interface, None, trace_target) + rewrite_spill_pseudo_instructions(context, analysis, interface, trace_target) } fn rewrite_cfg_spills( @@ -570,7 +570,7 @@ fn rewrite_cfg_spills( used_sets.insert(block_ref, used); } - rewrite_spill_pseudo_instructions(context, analysis, interface, Some(dominfo), trace_target) + rewrite_spill_pseudo_instructions(context, analysis, interface, trace_target) } /// Rewrite uses of spilled values in `op` and any nested regions of `op`. @@ -838,19 +838,15 @@ fn rewrite_inserted_phi_uses( /// /// However, this produces dead spills on some paths through the function, which are not /// needed once rewrites have been performed. So we eliminate dead spills by identifying -/// those spills which do not dominate any reloads - if a store to a spill slot can never -/// be read, then the store can be elided. +/// those spills which cannot reach any live reload of their value - if a store to a spill +/// slot can never be read, then the store can be elided. fn rewrite_spill_pseudo_instructions( context: Rc, analysis: &mut SpillAnalysis, interface: &mut dyn TransformSpillsInterface, - dominfo: Option<&DominanceInfo>, trace_target: &TraceTarget, ) -> Result<(), Report> { - use midenc_hir::{ - dominance::Dominates, - patterns::{RewriterImpl, TracingRewriterListener}, - }; + use midenc_hir::patterns::{RewriterImpl, TracingRewriterListener}; let mut builder = RewriterImpl::::new(context) .with_listener(TracingRewriterListener); @@ -863,7 +859,7 @@ fn rewrite_spill_pseudo_instructions( .expect("expected materialized spill operation to implement SpillLike"); spill_like.spilled_value() }; - // Only keep spills that feed a live reload dominated by this spill + // Only keep spills that can reach a live reload of their value let mut is_used = false; for rinfo in analysis.reloads() { if rinfo.value != spilled { @@ -872,22 +868,14 @@ fn rewrite_spill_pseudo_instructions( let Some(reload_op) = rinfo.inst else { continue; }; - let (reload_used, dom_ok) = { + let reload_used = { let rop = reload_op.borrow(); let rl = rop .as_trait::() .expect("expected materialized reload op to implement ReloadLike"); - let used = rl.reloaded().borrow().is_used(); - let dom_ok = match dominfo { - None => true, - Some(dominfo) => { - let sop = operation.borrow(); - sop.dominates(&rop, dominfo) - } - }; - (used, dom_ok) + rl.reloaded().borrow().is_used() }; - if reload_used && dom_ok { + if reload_used && spill_reaches_reload(operation, reload_op) { is_used = true; break; } @@ -935,3 +923,59 @@ fn rewrite_spill_pseudo_instructions( Ok(()) } + +/// Returns true if some control-flow path from `spill` reaches `reload`. +/// +/// Reachability, not dominance, is the criterion for keeping a spill: the spills analysis +/// conservatively places spills per path (e.g. along each edge of a join), so a reload after the +/// join is covered by a set of spills, none of which individually dominates it. Eliding a spill +/// is only sound when it provably cannot reach any live reload of its value, otherwise some path +/// would reload from a local that was never written. +pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { + // Normalize both operations to the innermost region containing them both: an operation + // nested in a sub-region (e.g. structured control flow) is represented by its ancestor + // operation in the common region. + let Some(common_region) = Region::find_common_ancestor(&[spill, reload]) else { + // Conservatively keep spills whose placement cannot be reasoned about. + return true; + }; + let common_region = common_region.borrow(); + let (Some(spill_ancestor), Some(reload_ancestor)) = + (common_region.find_ancestor_op(spill), common_region.find_ancestor_op(reload)) + else { + return true; + }; + + // Both are nested under the same operation, in different sub-regions. Whether control can + // transfer between those regions depends on that operation's semantics (e.g. it can across + // loop iterations), so conservatively assume it can. + if spill_ancestor == reload_ancestor { + return true; + } + + let (Some(spill_block), Some(reload_block)) = + (spill_ancestor.borrow().parent(), reload_ancestor.borrow().parent()) + else { + return true; + }; + + // Within one block the spill directly reaches every later operation; earlier operations are + // only reachable through a cycle back into the block, which the successor walk finds. + if spill_block == reload_block && spill_ancestor.borrow().is_before_in_block(&reload_ancestor) { + return true; + } + + let mut visited = SmallSet::::default(); + let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(spill_block)); + while let Some(block) = worklist.pop() { + if block == reload_block { + return true; + } + if !visited.insert(block) { + continue; + } + worklist.extend(BlockRef::children(block)); + } + + false +} From 8440f20bac551b25ec19614546a3eef615437eb1 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 17:33:56 +0300 Subject: [PATCH 02/19] test(codegen): rewrite spill pruning tests on parsed IR and lit The spill-pruning regression tests built their fixtures with IR builders and pinned results via expect-files and line-count assertions, which made the CFG shapes hard to read and coupled the tests to builder APIs. Rewrite the spill_reaches_reload unit tests to parse textual IR fixtures (via parse_function_fixpoint, locating operations positionally), and move the join-covered-reload regression to tests/lit/hir-opt, where hir-opt runs a transform-spills pass pipeline over the parsed module and filecheck pins the materialized output: one store_local per arm to the same procedure local, and a single load_local at the join. Register TransformSpills in the global pass registry so pass pipelines can refer to it by name; previously it was not invocable from hir-opt at all. Both layers independently guard the fix: against dominance-based pruning the lit test fails with the live-reload-without-spill diagnostic, and each reachability unit test fails on the assertion encoding the corresponding dominance blind spot (arm-to-join, back-edge-to-header, nested-region-to-parent-successor). --- dialects/hir/src/transforms/spill.rs | 5 + ...lizes_spills_join_covered_reload_after.hir | 31 -- ...izes_spills_join_covered_reload_before.hir | 28 -- dialects/hir/src/transforms/spill/tests.rs | 344 ++++++------------ .../transform-spills-join-covered-reload.hir | 68 ++++ 5 files changed, 182 insertions(+), 294 deletions(-) delete mode 100644 dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir delete mode 100644 dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir create mode 100644 tests/lit/hir-opt/transform-spills-join-covered-reload.hir diff --git a/dialects/hir/src/transforms/spill.rs b/dialects/hir/src/transforms/spill.rs index c2196f00d..0105532b7 100644 --- a/dialects/hir/src/transforms/spill.rs +++ b/dialects/hir/src/transforms/spill.rs @@ -13,6 +13,11 @@ use midenc_hir_transform::{self as transforms, ReloadLike, SpillLike, TransformS #[derive(Default)] pub struct TransformSpills; +midenc_hir::inventory::submit!(::midenc_hir::pass::registry::PassInfo::new::( + "transform-spills", + "materialize operand stack spills as stores/loads of procedure locals" +)); + impl Pass for TransformSpills { type Target = Function; diff --git a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir deleted file mode 100644 index 7b1167848..000000000 --- a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_after.hir +++ /dev/null @@ -1,31 +0,0 @@ -builtin.function public extern("C") @materializes_spills_join_covered_reload(%0: ptr) -> u32 { - %1 = hir.ptr_to_int %0 <{ ty = #builtin.type }>; - %2 = arith.constant 32 : u32; - %3 = arith.add %1, %2 <{ overflow = #builtin.overflow }>; - %4 = hir.int_to_ptr %3 <{ ty = #builtin.type> }>; - %5 = hir.load %4; - %6 = arith.constant 64 : u32; - %7 = arith.add %1, %6 <{ overflow = #builtin.overflow }>; - %8 = hir.int_to_ptr %7 <{ ty = #builtin.type> }>; - %9 = hir.load %8; - %10 = arith.constant 0 : u32; - %11 = arith.eq %1, %10; - cf.cond_br %11 ^block2, ^block3 : (i1); -^block2: - %12 = arith.constant 1 : u64; - hir.store_local %3 <{ local = #builtin.local_variable<0, u32> }> : (u32); - %13 = hir.exec ::@test::@example(%8, %5, %9, %9, %12) : extern("C") (ptr, u128, u128, u128, u64) -> u32; - cf.br ^block4(%13 : u32); -^block3: - %14 = arith.constant 1 : u64; - hir.store_local %3 <{ local = #builtin.local_variable<0, u32> }> : (u32); - %15 = hir.exec ::@test::@example(%8, %5, %9, %9, %14) : extern("C") (ptr, u128, u128, u128, u64) -> u32; - cf.br ^block4(%15 : u32); -^block4(%16: u32): - %17 = arith.constant 5 : u32; - %18 = arith.add %1, %17 <{ overflow = #builtin.overflow }>; - %19 = arith.add %18, %16 <{ overflow = #builtin.overflow }>; - %22 = hir.load_local <{ local = #builtin.local_variable<0, u32> }>; - %20 = arith.add %19, %22 <{ overflow = #builtin.overflow }>; - builtin.ret %20 : (u32); -}; \ No newline at end of file diff --git a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir b/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir deleted file mode 100644 index 4bfe18825..000000000 --- a/dialects/hir/src/transforms/spill/expected/materializes_spills_join_covered_reload_before.hir +++ /dev/null @@ -1,28 +0,0 @@ -builtin.function public extern("C") @materializes_spills_join_covered_reload(%0: ptr) -> u32 { - %1 = hir.ptr_to_int %0 <{ ty = #builtin.type }>; - %2 = arith.constant 32 : u32; - %3 = arith.add %1, %2 <{ overflow = #builtin.overflow }>; - %4 = hir.int_to_ptr %3 <{ ty = #builtin.type> }>; - %5 = hir.load %4; - %6 = arith.constant 64 : u32; - %7 = arith.add %1, %6 <{ overflow = #builtin.overflow }>; - %8 = hir.int_to_ptr %7 <{ ty = #builtin.type> }>; - %9 = hir.load %8; - %10 = arith.constant 0 : u32; - %11 = arith.eq %1, %10; - cf.cond_br %11 ^block2, ^block3 : (i1); -^block2: - %12 = arith.constant 1 : u64; - %13 = hir.exec ::@test::@example(%8, %5, %9, %9, %12) : extern("C") (ptr, u128, u128, u128, u64) -> u32; - cf.br ^block4(%13 : u32); -^block3: - %14 = arith.constant 1 : u64; - %15 = hir.exec ::@test::@example(%8, %5, %9, %9, %14) : extern("C") (ptr, u128, u128, u128, u64) -> u32; - cf.br ^block4(%15 : u32); -^block4(%16: u32): - %17 = arith.constant 5 : u32; - %18 = arith.add %1, %17 <{ overflow = #builtin.overflow }>; - %19 = arith.add %18, %16 <{ overflow = #builtin.overflow }>; - %20 = arith.add %19, %3 <{ overflow = #builtin.overflow }>; - builtin.ret %20 : (u32); -}; \ No newline at end of file diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 6c3797190..ce2c0ef5c 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -1,4 +1,4 @@ -use alloc::{format, string::ToString, sync::Arc}; +use alloc::{format, rc::Rc, string::ToString, sync::Arc}; use litcheck_filecheck::{filecheck, litcheck}; use midenc_dialect_arith::ArithOpBuilder; @@ -6,8 +6,10 @@ use midenc_dialect_cf::ControlFlowOpBuilder as Cf; use midenc_dialect_scf::StructuredControlFlowOpBuilder; use midenc_expect_test::expect_file; use midenc_hir::{ - AddressSpace, Builder, Op, OperationRef, PointerType, ProgramPoint, Report, SourceSpan, Type, - ValueRef, dialects::builtin::BuiltinOpBuilder, testing::Test, + AddressSpace, BlockRef, Builder, Context, Op, OperationRef, PointerType, ProgramPoint, Report, + SourceSpan, Type, ValueRef, + dialects::builtin::{BuiltinOpBuilder, FunctionRef}, + testing::{Test, parse_function_fixpoint}, }; use midenc_hir_transform::spill_reaches_reload; @@ -15,9 +17,35 @@ use crate::{HirOpBuilder, transforms::TransformSpills}; type TestResult = Result; -/// Returns the defining operation of `value`. -fn defining_op(value: ValueRef) -> OperationRef { - value.borrow().get_defining_op().expect("expected value to have a defining op") +/// Returns the `index`-th block in the body of `function`. +fn block_at(function: FunctionRef, index: usize) -> BlockRef { + let function = function.borrow(); + function + .body() + .body() + .iter() + .nth(index) + .map(|block| block.as_block_ref()) + .expect("block index out of bounds") +} + +/// Returns the `index`-th operation in `block`. +fn op_at(block: BlockRef, index: usize) -> OperationRef { + let block = block.borrow(); + block + .body() + .iter() + .nth(index) + .map(|op| op.as_operation_ref()) + .expect("operation index out of bounds") +} + +/// Returns the `op_index`-th operation in the entry block of `op`'s `region_index`-th region. +fn op_in_region(op: OperationRef, region_index: usize, op_index: usize) -> OperationRef { + let op = op.borrow(); + let region = op.regions().iter().nth(region_index).expect("region index out of bounds"); + let entry = region.entry_block_ref().expect("expected region to have an entry block"); + op_at(entry, op_index) } /// Build a simple single-block function which triggers spills and reloads, @@ -261,134 +289,6 @@ fn materializes_spills_branching_cfg() -> TestResult<()> { Ok(()) } -/// Build a branching CFG in which *both* arms spill the same value (each contains a copy of the -/// high-pressure call), while the only use of that value after the spills lies *after* the join. -/// -/// The spills analysis places one spill per arm, and the single reload lands at the join, so the -/// reload is covered by the *set* of spills, none of which individually dominates it. This is a -/// regression test for spill pruning: pruning spills that do not dominate a live reload erased -/// both arm spills while keeping the reload, and reload materialization then panicked because no -/// procedure local was ever allocated for the spilled value. -/// -/// Operand stack pressure at each call (the analysis models pressure in felts with `K = 16`): -/// the call arguments require 15 felts (`v6: ptr` = 1, `v4: u128` = 4, `v7: u128` passed twice -/// = 8, `u64` = 2), and `v1: u32` and `v2: u32` are live across both calls (used only in the -/// join block), for a total of `15 + 2 = 17 > K`, so exactly one of `{v1, v2}` is spilled in -/// each arm. -#[test] -fn materializes_spills_join_covered_reload() -> TestResult<()> { - let mut test = Test::named("materializes_spills_join_covered_reload").in_module("test"); - let span = SourceSpan::UNKNOWN; - - test.with_function( - test.name(), - &[Type::Ptr(Arc::new(PointerType::new_with_address_space( - Type::U8, - AddressSpace::Element, - )))], - &[Type::U32], - ); - let func = test.function(); - - let callee = test.define_function( - "example", - &[ - Type::Ptr(Arc::new(PointerType::new_with_address_space( - Type::U128, - AddressSpace::Element, - ))), - Type::U128, - Type::U128, - Type::U128, - Type::U64, - ], - &[Type::U32], - ); - - { - let mut b = test.function_builder(); - let entry = b.current_block(); - let v0 = entry.borrow().arguments()[0] as ValueRef; - let v1 = b.ptrtoint(v0, Type::U32, span)?; - let k32 = b.u32(32, span); - let v2 = b.add_unchecked(v1, k32, span)?; - let v3 = b.inttoptr( - v2, - Type::Ptr(Arc::new(PointerType::new_with_address_space( - Type::U128, - AddressSpace::Element, - ))), - span, - )?; - let v4 = b.load(v3, span)?; - let k64 = b.u32(64, span); - let v5 = b.add_unchecked(v1, k64, span)?; - let v6 = b.inttoptr( - v5, - Type::Ptr(Arc::new(PointerType::new_with_address_space( - Type::U128, - AddressSpace::Element, - ))), - span, - )?; - let v7 = b.load(v6, span)?; - let zero = b.u32(0, span); - let v8 = b.eq(v1, zero, span)?; - let t = b.create_block(); - let f = b.create_block(); - let join = b.create_block(); - Cf::cond_br(&mut b, v8, t, [], f, [], span)?; - - let callee_sig = callee.borrow().get_signature().clone(); - - // then: the high-pressure call forces a spill on this path - b.switch_to_block(t); - let v9 = b.u64(1, span); - let call = b.exec(callee, callee_sig.clone(), [v6, v4, v7, v7, v9], span)?; - let v10 = call.borrow().results()[0] as ValueRef; - b.br(join, [v10], span)?; - - // else: the same call, so the same value is spilled on this path as well - b.switch_to_block(f); - let v11 = b.u64(1, span); - let call2 = b.exec(callee, callee_sig, [v6, v4, v7, v7, v11], span)?; - let v12 = call2.borrow().results()[0] as ValueRef; - b.br(join, [v12], span)?; - - // join: the only uses of the values live across the calls - let v13 = b.append_block_param(join, Type::U32, span); - b.switch_to_block(join); - let k5 = b.u32(5, span); - let v14 = b.add_unchecked(v1, k5, span)?; - let v15 = b.add_unchecked(v14, v13, span)?; - let v16 = b.add_unchecked(v15, v2, span)?; - b.ret(Some(v16), span)?; - } - - let before = func.as_operation_ref().borrow().to_string(); - let before_file = format!("expected/{}_before.hir", test.name()); - expect_file![&before_file].assert_eq(&before); - - test.apply_pass::(false)?; - - let after = func.as_operation_ref().borrow().to_string(); - let after_file = format!("expected/{}_after.hir", test.name()); - expect_file![&after_file].assert_eq(&after); - - // The spilled value must be stored once per arm, and reloaded once at the join - let stores = after.lines().filter(|l| l.trim_start().starts_with("hir.store_local ")).count(); - let loads = after - .lines() - .filter(|l| { - l.trim_start().contains("= hir.load_local ") - || l.trim_start().starts_with("hir.load_local ") - }) - .count(); - assert!(stores == 2, "expected one store_local op in each arm\n{after}"); - assert!(loads == 1, "expected one load_local op at the join\n{after}"); - Ok(()) -} - /// Build a small multi-block CFG containing a `scf.if`, where spilled values are only used inside /// the nested regions of the `scf.if`. /// @@ -716,39 +616,32 @@ fn materializes_spills_nested_scf_while_after_region() -> TestResult<()> { /// elimination keeps working. #[test] fn spill_reachability_in_branching_cfg() -> TestResult<()> { - let mut test = Test::named("spill_reachability_in_branching_cfg").in_module("test"); - let span = SourceSpan::UNKNOWN; - test.with_function(test.name(), &[Type::U32], &[Type::U32]); - - let mut b = test.function_builder(); - let entry = b.current_block(); - let v0 = entry.borrow().arguments()[0] as ValueRef; - let k1 = b.u32(1, span); - let entry_first = b.add_unchecked(v0, k1, span)?; - let cond = b.eq(entry_first, k1, span)?; - let left = b.create_block(); - let right = b.create_block(); - let join = b.create_block(); - Cf::cond_br(&mut b, cond, left, [], right, [], span)?; - - b.switch_to_block(left); - let left_value = b.add_unchecked(entry_first, k1, span)?; - b.br(join, [left_value], span)?; - - b.switch_to_block(right); - let right_value = b.add_unchecked(entry_first, entry_first, span)?; - b.br(join, [right_value], span)?; - - let join_arg = b.append_block_param(join, Type::U32, span); - b.switch_to_block(join); - let join_value = b.add_unchecked(join_arg, k1, span)?; - b.ret(Some(join_value), span)?; - - let entry_first = defining_op(entry_first); - let entry_second = defining_op(cond); - let left_op = defining_op(left_value); - let right_op = defining_op(right_value); - let join_op = defining_op(join_value); + let source = r#"builtin.function public extern("C") @spill_reachability_in_branching_cfg(%a: u32) -> u32 { + %one = arith.constant 1 : u32; + %first = arith.add %a, %one <{ overflow = #builtin.overflow }>; + %cond = arith.eq %first, %one; + cf.cond_br %cond ^left, ^right : (i1); +^left: + %lv = arith.add %first, %one <{ overflow = #builtin.overflow }>; + cf.br ^join(%lv : u32); +^right: + %rv = arith.add %first, %first <{ overflow = #builtin.overflow }>; + cf.br ^join(%rv : u32); +^join(%j: u32): + %jv = arith.add %j, %one <{ overflow = #builtin.overflow }>; + builtin.ret %jv : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = + parse_function_fixpoint(&context, "spill_reachability_in_branching_cfg.hir", source)?; + + let entry = block_at(function, 0); + let entry_first = op_at(entry, 1); + let entry_second = op_at(entry, 2); + let left_op = op_at(block_at(function, 1), 0); + let right_op = op_at(block_at(function, 2), 0); + let join_op = op_at(block_at(function, 3), 0); assert!(spill_reaches_reload(left_op, join_op), "arm must reach the join"); assert!(spill_reaches_reload(right_op, join_op), "arm must reach the join"); @@ -771,37 +664,30 @@ fn spill_reachability_in_branching_cfg() -> TestResult<()> { /// Positional reachability through a loop back-edge. #[test] fn spill_reachability_through_loop_back_edge() -> TestResult<()> { - let mut test = Test::named("spill_reachability_through_loop_back_edge").in_module("test"); - let span = SourceSpan::UNKNOWN; - test.with_function(test.name(), &[Type::U32], &[Type::U32]); - - let mut b = test.function_builder(); - let entry = b.current_block(); - let v0 = entry.borrow().arguments()[0] as ValueRef; - let header = b.create_block(); - let body = b.create_block(); - let exit = b.create_block(); - b.br(header, [v0], span)?; - - let header_arg = b.append_block_param(header, Type::U32, span); - b.switch_to_block(header); - let k1 = b.u32(1, span); - let header_value = b.add_unchecked(header_arg, k1, span)?; - let cond = b.eq(header_value, k1, span)?; - Cf::cond_br(&mut b, cond, body, [], exit, [], span)?; - - b.switch_to_block(body); - let body_value = b.add_unchecked(header_value, k1, span)?; - b.br(header, [body_value], span)?; - - b.switch_to_block(exit); - let exit_value = b.add_unchecked(header_value, header_value, span)?; - b.ret(Some(exit_value), span)?; - - let header_op = defining_op(header_value); - let header_second = defining_op(cond); - let body_op = defining_op(body_value); - let exit_op = defining_op(exit_value); + let source = r#"builtin.function public extern("C") @spill_reachability_through_loop_back_edge(%n: u32) -> u32 { + cf.br ^header(%n : u32); +^header(%i: u32): + %one = arith.constant 1 : u32; + %next = arith.add %i, %one <{ overflow = #builtin.overflow }>; + %cond = arith.eq %next, %one; + cf.cond_br %cond ^body, ^exit : (i1); +^body: + %step = arith.add %next, %one <{ overflow = #builtin.overflow }>; + cf.br ^header(%step : u32); +^exit: + %out = arith.add %next, %next <{ overflow = #builtin.overflow }>; + builtin.ret %out : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = + parse_function_fixpoint(&context, "spill_reachability_through_loop_back_edge.hir", source)?; + + let header = block_at(function, 1); + let header_op = op_at(header, 1); + let header_second = op_at(header, 2); + let body_op = op_at(block_at(function, 2), 0); + let exit_op = op_at(block_at(function, 3), 0); assert!(spill_reaches_reload(body_op, header_op), "back edge must reach the header"); assert!( @@ -818,43 +704,31 @@ fn spill_reachability_through_loop_back_edge() -> TestResult<()> { /// under the same region-branch op conservatively reach each other (e.g. across loop iterations). #[test] fn spill_reachability_across_nested_regions() -> TestResult<()> { - let mut test = Test::named("spill_reachability_across_nested_regions").in_module("test"); - let span = SourceSpan::UNKNOWN; - test.with_function(test.name(), &[Type::U32], &[Type::U32]); - - let mut b = test.function_builder(); - let entry = b.current_block(); - let v0 = entry.borrow().arguments()[0] as ValueRef; - let k1 = b.u32(1, span); - let pre_value = b.add_unchecked(v0, k1, span)?; - let cond = b.eq(pre_value, k1, span)?; - - let mut if_op = b.r#if(cond, &[Type::U32], span)?; - let context = b.builder().context_rc(); - let (then_block, else_block) = (context.create_block(), context.create_block()); - { - let mut if_op = if_op.borrow_mut(); - if_op.then_body_mut().push_back(then_block); - if_op.else_body_mut().push_back(else_block); - } - - b.switch_to_block(then_block); - let then_value = b.add_unchecked(pre_value, k1, span)?; - b.r#yield([then_value], span)?; - - b.switch_to_block(else_block); - let else_value = b.add_unchecked(pre_value, pre_value, span)?; - b.r#yield([else_value], span)?; - - b.switch_to_block(entry); - let if_result = if_op.as_operation_ref().borrow().results()[0] as ValueRef; - let post_value = b.add_unchecked(if_result, k1, span)?; - b.ret(Some(post_value), span)?; - - let pre_op = defining_op(pre_value); - let then_op = defining_op(then_value); - let else_op = defining_op(else_value); - let post_op = defining_op(post_value); + let source = r#"builtin.function public extern("C") @spill_reachability_across_nested_regions(%a: u32) -> u32 { + %one = arith.constant 1 : u32; + %pre = arith.add %a, %one <{ overflow = #builtin.overflow }>; + %cond = arith.eq %pre, %one; + %r = scf.if %cond then { + %t = arith.add %pre, %one <{ overflow = #builtin.overflow }>; + scf.yield %t : (u32); + } else { + %e = arith.add %pre, %pre <{ overflow = #builtin.overflow }>; + scf.yield %e : (u32); + } : (i1) -> (u32); + %post = arith.add %r, %one <{ overflow = #builtin.overflow }>; + builtin.ret %post : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = + parse_function_fixpoint(&context, "spill_reachability_across_nested_regions.hir", source)?; + + let entry = block_at(function, 0); + let pre_op = op_at(entry, 1); + let if_op = op_at(entry, 3); + let post_op = op_at(entry, 4); + let then_op = op_in_region(if_op, 0, 0); + let else_op = op_in_region(if_op, 1, 0); assert!( spill_reaches_reload(pre_op, then_op), diff --git a/tests/lit/hir-opt/transform-spills-join-covered-reload.hir b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir new file mode 100644 index 000000000..e2b15e7bc --- /dev/null +++ b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir @@ -0,0 +1,68 @@ +// RUN: hir-opt %s --pass-pipeline='builtin.module(builtin.function(transform-spills))' | filecheck %s + +// Both arms of the diamond below spill the same value (each contains a copy of the +// high-pressure call), while the only use of that value after the spills lies after the join. +// The spills analysis places one spill per arm, and the single reload lands at the join, so the +// reload is covered by the *set* of spills, none of which individually dominates it. This is a +// regression test for spill pruning: pruning spills that do not dominate a live reload erased +// both arm spills while keeping the reload, and reload materialization then failed because no +// procedure local was ever allocated for the spilled value. +// +// Operand stack pressure at each call (the analysis models pressure in felts with K = 16): the +// call arguments require 15 felts (%8: ptr = 1, %5: u128 = 4, %9: u128 passed twice = 8, +// u64 = 2), and %1: u32 and %3: u32 are live across both calls (used only in the join block), +// for a total of 15 + 2 = 17 > K, so exactly one of {%1, %3} is spilled in each arm. + +// COM: No spill materializes before the branch +// CHECK-NOT: hir.store_local +// CHECK: cf.cond_br %{{\d+}} ^[[THEN:block\d+]], ^[[ELSE:block\d+]] + +// COM: The then-arm spills the value live across both calls, before its call +// CHECK: ^[[THEN]]: +// CHECK: hir.store_local %[[V:\d+]] <{ local = #builtin.local_variable<[[L:\d+]], u32> }> +// CHECK-NEXT: hir.exec ::@test::@example + +// COM: The else-arm spills the same value to the same local +// CHECK: ^[[ELSE]]: +// CHECK: hir.store_local %[[V]] <{ local = #builtin.local_variable<[[L]], u32> }> +// CHECK-NEXT: hir.exec ::@test::@example +// CHECK-NOT: hir.store_local + +// COM: The join holds the single reload of the spilled value +// CHECK: hir.load_local <{ local = #builtin.local_variable<[[L]], u32> }> +// CHECK-NOT: hir.load_local +// CHECK-NOT: hir.store_local +// CHECK: builtin.ret % +builtin.module public @test { + builtin.function public extern("C") @join_covered_reload(%0: ptr) -> u32 { + %1 = hir.ptr_to_int %0 <{ ty = #builtin.type }>; + %2 = arith.constant 32 : u32; + %3 = arith.add %1, %2 <{ overflow = #builtin.overflow }>; + %4 = hir.int_to_ptr %3 <{ ty = #builtin.type> }>; + %5 = hir.load %4; + %6 = arith.constant 64 : u32; + %7 = arith.add %1, %6 <{ overflow = #builtin.overflow }>; + %8 = hir.int_to_ptr %7 <{ ty = #builtin.type> }>; + %9 = hir.load %8; + %10 = arith.constant 0 : u32; + %11 = arith.eq %1, %10; + cf.cond_br %11 ^then, ^else : (i1); + ^then: + %12 = arith.constant 1 : u64; + %13 = hir.exec ::@test::@example(%8, %5, %9, %9, %12) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^join(%13 : u32); + ^else: + %14 = arith.constant 1 : u64; + %15 = hir.exec ::@test::@example(%8, %5, %9, %9, %14) : extern("C") (ptr, u128, u128, u128, u64) -> u32; + cf.br ^join(%15 : u32); + ^join(%16: u32): + %17 = arith.constant 5 : u32; + %18 = arith.add %1, %17 <{ overflow = #builtin.overflow }>; + %19 = arith.add %18, %16 <{ overflow = #builtin.overflow }>; + %20 = arith.add %19, %3 <{ overflow = #builtin.overflow }>; + builtin.ret %20 : (u32); + }; + builtin.function public extern("C") @example(%p: ptr, %x: u128, %y: u128, %z: u128, %w: u64) -> u32 { + builtin.ret_imm 42 : u32; + }; +}; From ce155f57f144cf21bb8d74e776fb7c013f712aae Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 18:04:59 +0300 Subject: [PATCH 03/19] fix(codegen): treat repetitive-region re-entry as reachable in spill pruning spill_reaches_reload answered false for a reload positioned before a spill in the same region of a loop op (e.g. the before region of an scf.while): the region's block ends in a region terminator with no block successors, so the CFG successor walk finds nothing, while the loop's back edge lives in the region graph of the owning op. That violates the predicate's contract that false means provably unreachable, since the region re-executes and control genuinely flows from the spill back around to the reload. Today the erased spill is always redundant, as every spill of a value stores the same SSA value to one per-value local and the analysis keeps loop-entry paths covered, but any consumer of the exported predicate, or a future change to spill slot allocation, would inherit a stale-read miscompile. When the forward walk fails, climb the region ancestry from the common region and report reachable if any enclosing region is repetitive, stopping at isolated-from-above boundaries (fresh locals per invocation) and conservatively keeping spills under ops with unknown region semantics. Document the directional contract on the predicate: true may be conservative, false must be proof. Pin the repaired behavior with a unit test covering re-entry of an scf.while region, forward reachability within its block, and no-reach-back-in from after the loop. --- dialects/hir/src/transforms/spill/tests.rs | 53 ++++++++++++++++++++++ hir-transform/src/spill.rs | 29 ++++++++++++ 2 files changed, 82 insertions(+) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index ce2c0ef5c..e43c16389 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -748,3 +748,56 @@ fn spill_reachability_across_nested_regions() -> TestResult<()> { ); Ok(()) } + +/// Positional reachability through re-entry of a repetitive region. +/// +/// A repetitive region (e.g. the `before` region of an `scf.while`) has no CFG back edge: its +/// block ends in a region terminator with no block successors, and the loop is expressed in the +/// region graph of the parent op. An op positioned after another op in such a region still +/// reaches it, through the next iteration. +#[test] +fn spill_reachability_through_region_re_entry() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @spill_reachability_through_region_re_entry(%n: u32) -> u32 { + %zero = arith.constant 0 : u32; + %r = scf.while %zero before { + ^head(%i: u32): + %one = arith.constant 1 : u32; + %early = arith.add %i, %one <{ overflow = #builtin.overflow }>; + %late = arith.add %early, %early <{ overflow = #builtin.overflow }>; + %continue = arith.lt %late, %n; + scf.condition %continue, %late : (i1, u32); + } after { + ^body(%j: u32): + %next = arith.incr %j; + scf.yield %next : (u32); + } : (u32) -> u32; + builtin.ret %r : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = parse_function_fixpoint( + &context, + "spill_reachability_through_region_re_entry.hir", + source, + )?; + + let entry = block_at(function, 0); + let while_op = op_at(entry, 1); + let ret_op = op_at(entry, 2); + let early_op = op_in_region(while_op, 0, 1); + let late_op = op_in_region(while_op, 0, 2); + + assert!( + spill_reaches_reload(late_op, early_op), + "a later op must reach an earlier op in the same repetitive region through re-entry" + ); + assert!( + spill_reaches_reload(early_op, late_op), + "earlier op must reach a later op in the same block" + ); + assert!( + !spill_reaches_reload(ret_op, late_op), + "an op after the loop must not reach into it" + ); + Ok(()) +} diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 63e1a8e98..44cb2391b 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -926,6 +926,10 @@ fn rewrite_spill_pseudo_instructions( /// Returns true if some control-flow path from `spill` reaches `reload`. /// +/// The result may be conservatively `true` when reachability cannot be reasoned about precisely; +/// `false` means the reload is provably unreachable from the spill, which is the direction +/// callers rely on when erasing spills. +/// /// Reachability, not dominance, is the criterion for keeping a spill: the spills analysis /// conservatively places spills per path (e.g. along each edge of a join), so a reload after the /// join is covered by a set of spills, none of which individually dominates it. Eliding a spill @@ -977,5 +981,30 @@ pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { worklist.extend(BlockRef::children(block)); } + // No forward path within the common region. Control can still leave the region after the + // spill and come back around to the reload if the common region, or any region enclosing + // it, can execute more than once (e.g. the regions of an `scf.while`): such back edges are + // expressed in the region graph of the owning op, not as block successors. + let mut region = Some(common_region.as_region_ref()); + while let Some(r) = region { + let Some(owner) = r.parent() else { + break; + }; + let owner_op = owner.borrow(); + if owner_op.implements::() { + // Function boundary: every invocation gets fresh locals, so re-entry of the + // function is irrelevant to spill coverage. + break; + } + if !owner_op.implements::() { + // Unknown region semantics: conservatively treat re-entry as possible. + return true; + } + if r.borrow().is_repetitive_region() { + return true; + } + region = owner_op.parent_region(); + } + false } From 46769e3a06187d90efc9f9a107a03357ff021127 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 18:06:03 +0300 Subject: [PATCH 04/19] docs(codegen): describe reload coverage in path terms, not dominance The ReloadLike contract still said a reload requires a dominating SpillLike op, which is exactly the invariant the reachability-based pruning removed: a reload after a join is covered by a set of per-path spills, none of which individually dominates it. Restate the contract in path-coverage terms so the old dominance reasoning is not reintroduced from the docs, and clarify in the transform_spills overview that dominance governs only the SSA use rewrite while spill materialization is decided by reachability. --- hir-transform/src/spill.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 44cb2391b..39915d079 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -79,11 +79,13 @@ pub trait SpillLike { /// An operation trait for operations that implement reload-like behavior for purposes of the /// spills transformation/rewrite. /// -/// A reload-like operation is expected to take a single value, for which a dominating [SpillLike] -/// op exists, and produce a new, unique SSA value corresponding to the reloaded spill value. The -/// spills transformation will handle rewriting any uses of the [SpillLike] and [ReloadLike] ops -/// such that they are not present after the transformation, in conjunction with an implementation -/// of the [TransformSpillsInterface]. +/// A reload-like operation is expected to take a single value, for which at least one [SpillLike] +/// op of that value executes on every control-flow path reaching the reload (a reload after a +/// join may be covered by a set of per-path spills, none of which individually dominates it), and +/// produce a new, unique SSA value corresponding to the reloaded spill value. The spills +/// transformation will handle rewriting any uses of the [SpillLike] and [ReloadLike] ops such +/// that they are not present after the transformation, in conjunction with an implementation of +/// the [TransformSpillsInterface]. pub trait ReloadLike { /// Returns the operand corresponding to the spilled value fn spilled(&self) -> OpOperand; @@ -107,7 +109,9 @@ pub trait ReloadLike { /// * Rewrites `op` such that all uses of a spilled value dominated by a reload, are rewritten to /// use that reload, or in the case of crossing a dominance frontier, a materialized block /// argument/phi representing the closest definition of that value from each predecessor. -/// * Rewrites all spill and reload instructions to their primitive memory store/load ops +/// * Rewrites all spill and reload instructions to their primitive memory store/load ops. +/// Dominance governs only the SSA use rewrite above; whether a spill is materialized or elided +/// is decided by reachability to live reloads (see [rewrite_spill_pseudo_instructions]). pub fn transform_spills( op: OperationRef, analysis: &mut SpillAnalysis, From b0f52093383ee8f9f2d91cc5168396d51e21bfe6 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 18:07:56 +0300 Subject: [PATCH 05/19] refactor(codegen): rename spill_reaches_reload to op_reaches The predicate is plain operation-to-operation reachability: its body never touches SpillLike or ReloadLike, and the unit tests exercise it on arbitrary arith ops, which read oddly through a spill-specific name. Rename it to op_reaches, document the generic contract on the public item (conservative true, false is a proof, scoped to a single execution of the innermost isolated-from-above ancestor), and keep the spill-pruning rationale with the pruning itself in the rewrite_spill_pseudo_instructions docs. --- dialects/hir/src/transforms/spill/tests.rs | 46 +++++++---------- hir-transform/src/lib.rs | 4 +- hir-transform/src/spill.rs | 59 ++++++++++++---------- 3 files changed, 50 insertions(+), 59 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index e43c16389..3cb7eaa7a 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -11,7 +11,7 @@ use midenc_hir::{ dialects::builtin::{BuiltinOpBuilder, FunctionRef}, testing::{Test, parse_function_fixpoint}, }; -use midenc_hir_transform::spill_reaches_reload; +use midenc_hir_transform::op_reaches; use crate::{HirOpBuilder, transforms::TransformSpills}; @@ -643,21 +643,18 @@ fn spill_reachability_in_branching_cfg() -> TestResult<()> { let right_op = op_at(block_at(function, 2), 0); let join_op = op_at(block_at(function, 3), 0); - assert!(spill_reaches_reload(left_op, join_op), "arm must reach the join"); - assert!(spill_reaches_reload(right_op, join_op), "arm must reach the join"); + assert!(op_reaches(left_op, join_op), "arm must reach the join"); + assert!(op_reaches(right_op, join_op), "arm must reach the join"); + assert!(!op_reaches(left_op, right_op), "sibling arms must not reach each other"); assert!( - !spill_reaches_reload(left_op, right_op), - "sibling arms must not reach each other" - ); - assert!( - spill_reaches_reload(entry_first, entry_second), + op_reaches(entry_first, entry_second), "earlier op must reach a later op in the same block" ); assert!( - !spill_reaches_reload(entry_second, entry_first), + !op_reaches(entry_second, entry_first), "later op must not reach an earlier op without a cycle" ); - assert!(!spill_reaches_reload(join_op, left_op), "join must not reach an arm"); + assert!(!op_reaches(join_op, left_op), "join must not reach an arm"); Ok(()) } @@ -689,12 +686,12 @@ fn spill_reachability_through_loop_back_edge() -> TestResult<()> { let body_op = op_at(block_at(function, 2), 0); let exit_op = op_at(block_at(function, 3), 0); - assert!(spill_reaches_reload(body_op, header_op), "back edge must reach the header"); + assert!(op_reaches(body_op, header_op), "back edge must reach the header"); assert!( - spill_reaches_reload(header_second, header_op), + op_reaches(header_second, header_op), "later op must reach an earlier op in the same block through the loop" ); - assert!(!spill_reaches_reload(exit_op, body_op), "exit must not reach the loop body"); + assert!(!op_reaches(exit_op, body_op), "exit must not reach the loop body"); Ok(()) } @@ -730,20 +727,14 @@ fn spill_reachability_across_nested_regions() -> TestResult<()> { let then_op = op_in_region(if_op, 0, 0); let else_op = op_in_region(if_op, 1, 0); + assert!(op_reaches(pre_op, then_op), "op before the region op must reach into it"); + assert!(op_reaches(then_op, post_op), "nested op must reach past the region op"); assert!( - spill_reaches_reload(pre_op, then_op), - "op before the region op must reach into it" - ); - assert!( - spill_reaches_reload(then_op, post_op), - "nested op must reach past the region op" - ); - assert!( - !spill_reaches_reload(post_op, then_op), + !op_reaches(post_op, then_op), "op after the region op must not reach back into it" ); assert!( - spill_reaches_reload(then_op, else_op), + op_reaches(then_op, else_op), "sibling regions of one op conservatively reach each other" ); Ok(()) @@ -788,16 +779,13 @@ fn spill_reachability_through_region_re_entry() -> TestResult<()> { let late_op = op_in_region(while_op, 0, 2); assert!( - spill_reaches_reload(late_op, early_op), + op_reaches(late_op, early_op), "a later op must reach an earlier op in the same repetitive region through re-entry" ); assert!( - spill_reaches_reload(early_op, late_op), + op_reaches(early_op, late_op), "earlier op must reach a later op in the same block" ); - assert!( - !spill_reaches_reload(ret_op, late_op), - "an op after the loop must not reach into it" - ); + assert!(!op_reaches(ret_op, late_op), "an op after the loop must not reach into it"); Ok(()) } diff --git a/hir-transform/src/lib.rs b/hir-transform/src/lib.rs index d7343f6cf..155a5d7e2 100644 --- a/hir-transform/src/lib.rs +++ b/hir-transform/src/lib.rs @@ -22,7 +22,5 @@ pub use self::{ cse::CommonSubexpressionElimination, sccp::SparseConditionalConstantPropagation, sink::{ControlFlowSink, SinkOperandDefs}, - spill::{ - ReloadLike, SpillLike, TransformSpillsInterface, spill_reaches_reload, transform_spills, - }, + spill::{ReloadLike, SpillLike, TransformSpillsInterface, op_reaches, transform_spills}, }; diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 39915d079..e64715838 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -844,6 +844,13 @@ fn rewrite_inserted_phi_uses( /// needed once rewrites have been performed. So we eliminate dead spills by identifying /// those spills which cannot reach any live reload of their value - if a store to a spill /// slot can never be read, then the store can be elided. +/// +/// Reachability, not dominance, is the criterion: the spills analysis places spills per path +/// (e.g. along each edge of a join), so a reload after the join is covered by a set of spills, +/// none of which individually dominates it. Eliding a spill is only sound when it provably +/// cannot reach any live reload of its value, otherwise some path would reload from a local +/// that was never written. Locals are allocated per invocation, so function re-entry is +/// irrelevant to coverage, matching the single-execution semantics of [op_reaches]. fn rewrite_spill_pseudo_instructions( context: Rc, analysis: &mut SpillAnalysis, @@ -879,7 +886,7 @@ fn rewrite_spill_pseudo_instructions( .expect("expected materialized reload op to implement ReloadLike"); rl.reloaded().borrow().is_used() }; - if reload_used && spill_reaches_reload(operation, reload_op) { + if reload_used && op_reaches(operation, reload_op) { is_used = true; break; } @@ -928,28 +935,25 @@ fn rewrite_spill_pseudo_instructions( Ok(()) } -/// Returns true if some control-flow path from `spill` reaches `reload`. +/// Returns true if some control-flow path from `from` may reach `to`. /// /// The result may be conservatively `true` when reachability cannot be reasoned about precisely; -/// `false` means the reload is provably unreachable from the spill, which is the direction -/// callers rely on when erasing spills. +/// `false` means `to` is provably unreachable from `from`. Callers making elision decisions +/// (e.g. spill pruning) rely on the `false` direction being a proof. /// -/// Reachability, not dominance, is the criterion for keeping a spill: the spills analysis -/// conservatively places spills per path (e.g. along each edge of a join), so a reload after the -/// join is covered by a set of spills, none of which individually dominates it. Eliding a spill -/// is only sound when it provably cannot reach any live reload of its value, otherwise some path -/// would reload from a local that was never written. -pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { +/// Reachability is evaluated within a single execution of the innermost isolated-from-above +/// ancestor (e.g. one function invocation). +pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { // Normalize both operations to the innermost region containing them both: an operation // nested in a sub-region (e.g. structured control flow) is represented by its ancestor // operation in the common region. - let Some(common_region) = Region::find_common_ancestor(&[spill, reload]) else { - // Conservatively keep spills whose placement cannot be reasoned about. + let Some(common_region) = Region::find_common_ancestor(&[from, to]) else { + // Operations without a common ancestor region cannot be reasoned about. return true; }; let common_region = common_region.borrow(); - let (Some(spill_ancestor), Some(reload_ancestor)) = - (common_region.find_ancestor_op(spill), common_region.find_ancestor_op(reload)) + let (Some(from_ancestor), Some(to_ancestor)) = + (common_region.find_ancestor_op(from), common_region.find_ancestor_op(to)) else { return true; }; @@ -957,26 +961,27 @@ pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { // Both are nested under the same operation, in different sub-regions. Whether control can // transfer between those regions depends on that operation's semantics (e.g. it can across // loop iterations), so conservatively assume it can. - if spill_ancestor == reload_ancestor { + if from_ancestor == to_ancestor { return true; } - let (Some(spill_block), Some(reload_block)) = - (spill_ancestor.borrow().parent(), reload_ancestor.borrow().parent()) + let (Some(from_block), Some(to_block)) = + (from_ancestor.borrow().parent(), to_ancestor.borrow().parent()) else { return true; }; - // Within one block the spill directly reaches every later operation; earlier operations are - // only reachable through a cycle back into the block, which the successor walk finds. - if spill_block == reload_block && spill_ancestor.borrow().is_before_in_block(&reload_ancestor) { + // Within one block an operation directly reaches every later operation; earlier operations + // are only reachable through a cycle back into the block, which the successor walk and the + // repetitive-region check below find. + if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { return true; } let mut visited = SmallSet::::default(); - let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(spill_block)); + let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from_block)); while let Some(block) = worklist.pop() { - if block == reload_block { + if block == to_block { return true; } if !visited.insert(block) { @@ -985,9 +990,9 @@ pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { worklist.extend(BlockRef::children(block)); } - // No forward path within the common region. Control can still leave the region after the - // spill and come back around to the reload if the common region, or any region enclosing - // it, can execute more than once (e.g. the regions of an `scf.while`): such back edges are + // No forward path within the common region. Control can still leave the region after + // `from` and come back around to `to` if the common region, or any region enclosing it, + // can execute more than once (e.g. the regions of an `scf.while`): such back edges are // expressed in the region graph of the owning op, not as block successors. let mut region = Some(common_region.as_region_ref()); while let Some(r) = region { @@ -996,8 +1001,8 @@ pub fn spill_reaches_reload(spill: OperationRef, reload: OperationRef) -> bool { }; let owner_op = owner.borrow(); if owner_op.implements::() { - // Function boundary: every invocation gets fresh locals, so re-entry of the - // function is irrelevant to spill coverage. + // An isolated-from-above boundary (e.g. a function): reachability is scoped to a + // single execution of its body. break; } if !owner_op.implements::() { From 9d49292ecec177ed4fc8967d580c2baa2e136a2d Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Sun, 5 Jul 2026 18:08:21 +0300 Subject: [PATCH 06/19] test(codegen): pin the then-arm store count in the spill pruning lit test The gap between the then-arm's call and the else-arm label had no CHECK-NOT, so an extra unpruned store after the first call would have passed unnoticed. Add the symmetric CHECK-NOT so the test pins exactly one store per arm on both arms. --- tests/lit/hir-opt/transform-spills-join-covered-reload.hir | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lit/hir-opt/transform-spills-join-covered-reload.hir b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir index e2b15e7bc..7df5b387c 100644 --- a/tests/lit/hir-opt/transform-spills-join-covered-reload.hir +++ b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir @@ -21,6 +21,7 @@ // CHECK: ^[[THEN]]: // CHECK: hir.store_local %[[V:\d+]] <{ local = #builtin.local_variable<[[L:\d+]], u32> }> // CHECK-NEXT: hir.exec ::@test::@example +// CHECK-NOT: hir.store_local // COM: The else-arm spills the same value to the same local // CHECK: ^[[ELSE]]: From 9419c55853aee7acc0e283927bd3d3e6047f99a1 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 08:26:26 +0300 Subject: [PATCH 07/19] fix(codegen): answer op_reaches conservatively outside a single isolation scope op_reaches documents false as a proof of unreachability, but a query crossing an isolation boundary broke that contract: for operations in two sibling functions, normalization silently climbed out into the module body and the answer degenerated to their textual order there, returning true one way and false the other. The false direction is meaningless for such queries, since the module body is a graph region whose block order is not control-flow order, and cross-function control transfer is not a walkable path. Guard the query up front by comparing the operations' innermost isolated-from-above ancestors and answering the conservative true when they differ, treat normalized ancestors that are themselves isolated (sibling functions in one module) the same way, and mirror the dominance precedent for same-block queries in regions without SSA dominance. Document the precondition, mark the unreachable find_ancestor_op fallback as defensive, and reword the same-ancestor comment to cover the enclosing-op case where true is exact rather than conservative. Pin the guards with a unit test querying across sibling functions and their module, in both directions. The only current caller passes two positions of one function body, so no miscompile was reachable; this hardens the exported contract. --- dialects/hir/src/transforms/spill/tests.rs | 52 +++++++++++++++++++ hir-transform/src/spill.rs | 59 ++++++++++++++++++---- 2 files changed, 102 insertions(+), 9 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 3cb7eaa7a..2f34882ae 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -8,7 +8,9 @@ use midenc_expect_test::expect_file; use midenc_hir::{ AddressSpace, BlockRef, Builder, Context, Op, OperationRef, PointerType, ProgramPoint, Report, SourceSpan, Type, ValueRef, + diagnostics::Uri, dialects::builtin::{BuiltinOpBuilder, FunctionRef}, + parse::{self, ParserConfig}, testing::{Test, parse_function_fixpoint}, }; use midenc_hir_transform::op_reaches; @@ -789,3 +791,53 @@ fn spill_reachability_through_region_re_entry() -> TestResult<()> { assert!(!op_reaches(ret_op, late_op), "an op after the loop must not reach into it"); Ok(()) } + +/// Queries that cross an isolation boundary are not control-flow questions. +/// +/// Operations in two different functions have no shared control flow to walk, and for the +/// function operations themselves the module body is a graph region where block order proves +/// nothing. All such queries must answer with the conservative `true` in both directions, so +/// that `false` remains a proof of unreachability. +#[test] +fn op_reaches_conservative_across_isolation_boundaries() -> TestResult<()> { + let source = r#"builtin.module public @test { + builtin.function public extern("C") @first(%a: u32) -> u32 { + %r = arith.add %a, %a <{ overflow = #builtin.overflow }>; + builtin.ret %r : (u32); + }; + builtin.function public extern("C") @second(%b: u32) -> u32 { + %r = arith.add %b, %b <{ overflow = #builtin.overflow }>; + builtin.ret %r : (u32); + }; +};"#; + + let context = Rc::new(Context::default()); + let module_op = parse::parse_any( + ParserConfig::new(context.clone()), + Uri::new("op_reaches_conservative_across_isolation_boundaries.hir"), + source, + )?; + + let first_fn = op_in_region(module_op, 0, 0); + let second_fn = op_in_region(module_op, 0, 1); + let in_first = op_in_region(first_fn, 0, 0); + let in_second = op_in_region(second_fn, 0, 0); + + assert!( + op_reaches(in_second, in_first), + "ops in different functions must be conservatively reachable regardless of module order" + ); + assert!( + op_reaches(in_first, in_second), + "ops in different functions must be conservatively reachable" + ); + assert!( + op_reaches(second_fn, first_fn), + "sibling isolated ops must be conservatively reachable regardless of module order" + ); + assert!( + op_reaches(first_fn, second_fn), + "sibling isolated ops must be conservatively reachable" + ); + Ok(()) +} diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index e64715838..a874bd1ec 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -942,8 +942,16 @@ fn rewrite_spill_pseudo_instructions( /// (e.g. spill pruning) rely on the `false` direction being a proof. /// /// Reachability is evaluated within a single execution of the innermost isolated-from-above -/// ancestor (e.g. one function invocation). +/// ancestor (e.g. one function invocation). Queries that cross an isolation boundary, or whose +/// positions cannot otherwise be related (graph regions, no common ancestor region), are +/// conservatively reachable. pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { + // Operations in different isolation scopes (e.g. two functions) share no control flow to + // walk, so the query cannot be answered; report the conservative `true`. + if isolation_scope(from) != isolation_scope(to) { + return true; + } + // Normalize both operations to the innermost region containing them both: an operation // nested in a sub-region (e.g. structured control flow) is represented by its ancestor // operation in the common region. @@ -955,27 +963,47 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { let (Some(from_ancestor), Some(to_ancestor)) = (common_region.find_ancestor_op(from), common_region.find_ancestor_op(to)) else { + // Unreachable per find_common_ancestor's postcondition (the returned region contains + // every queried op); kept as a defensive fallback. return true; }; - // Both are nested under the same operation, in different sub-regions. Whether control can - // transfer between those regions depends on that operation's semantics (e.g. it can across - // loop iterations), so conservatively assume it can. + // Both operations normalize to the same ancestor op: either one op encloses the other (and + // trivially reaches it), or they sit in different sub-regions of that op, where transfer + // between the regions depends on the op's semantics (e.g. it can happen across loop + // iterations), so conservatively assume it does. if from_ancestor == to_ancestor { return true; } + // Ancestors that are themselves isolated from above (e.g. two functions in one module) are + // symbol-container members, not control-flow positions; their block order proves nothing, + // so conservatively report reachable. + if from_ancestor.borrow().implements::() + || to_ancestor.borrow().implements::() + { + return true; + } + let (Some(from_block), Some(to_block)) = (from_ancestor.borrow().parent(), to_ancestor.borrow().parent()) else { return true; }; - // Within one block an operation directly reaches every later operation; earlier operations - // are only reachable through a cycle back into the block, which the successor walk and the - // repetitive-region check below find. - if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { - return true; + if from_block == to_block { + // In a region without SSA dominance, block order does not imply control-flow order + // (mirroring how dominance treats same-block queries in such regions), so the + // operations are conservatively mutually reachable. + if !from_block.borrow().has_ssa_dominance() { + return true; + } + // Within one block an operation directly reaches every later operation; earlier + // operations are only reachable through a cycle back into the block, which the + // successor walk and the repetitive-region check below find. + if from_ancestor.borrow().is_before_in_block(&to_ancestor) { + return true; + } } let mut visited = SmallSet::::default(); @@ -1017,3 +1045,16 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { false } + +/// Returns the nearest proper ancestor of `op` that is isolated from above, i.e. the operation +/// whose single execution scopes a reachability query involving `op`. +fn isolation_scope(op: OperationRef) -> Option { + let mut current = op.borrow().parent_op(); + while let Some(ancestor) = current { + if ancestor.borrow().implements::() { + return Some(ancestor); + } + current = ancestor.borrow().parent_op(); + } + None +} From f2d0736761df9e5f519fa3c645d5bff6544c0fea Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 08:27:21 +0300 Subject: [PATCH 08/19] chore(codegen): mark the uncovered-reload diagnostic as an internal error The missing-spill branch in convert_reload_to_load can only be reached through a compiler bug (pruning erased every covering spill), yet the message read like a statement about the input program and carried no location. Prefix it as an internal error and name the function being transformed so a future report is self-locating. --- dialects/hir/src/transforms/spill.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dialects/hir/src/transforms/spill.rs b/dialects/hir/src/transforms/spill.rs index 0105532b7..3d38f2564 100644 --- a/dialects/hir/src/transforms/spill.rs +++ b/dialects/hir/src/transforms/spill.rs @@ -160,9 +160,11 @@ impl TransformSpillsInterface for TransformSpillsImpl { let spilled = reload.borrow().as_trait::().unwrap().spilled_value(); let Some(local) = self.locals.get(&spilled).copied() else { + let function = self.function.borrow(); + let function = function.get_name(); return Err(Report::msg(format!( - "live reload of {spilled} has no corresponding spill: every kept reload must be \ - covered by at least one spill of the same value" + "internal error: live reload of {spilled} in {function} has no corresponding \ + spill: every kept reload must be covered by at least one spill of the same value" ))); }; let reloaded = rewriter.load_local(local, reload.span())?; From 6217bb2fd6dbd7e8ca4f64b0d76701ce7e9d035e Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 08:28:47 +0300 Subject: [PATCH 09/19] test(codegen): anchor the reload to the join block in the spill lit test The load_local check was only required to appear somewhere after the else-arm's call, so a regression that sank the reload into an arm would still have passed. Capture the join label from both arm terminators, which also pins that the arms branch to the same join, and require the reload to appear inside that block as its first spill-slot access. --- tests/lit/hir-opt/transform-spills-join-covered-reload.hir | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/lit/hir-opt/transform-spills-join-covered-reload.hir b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir index 7df5b387c..a24ed05de 100644 --- a/tests/lit/hir-opt/transform-spills-join-covered-reload.hir +++ b/tests/lit/hir-opt/transform-spills-join-covered-reload.hir @@ -22,14 +22,19 @@ // CHECK: hir.store_local %[[V:\d+]] <{ local = #builtin.local_variable<[[L:\d+]], u32> }> // CHECK-NEXT: hir.exec ::@test::@example // CHECK-NOT: hir.store_local +// CHECK: cf.br ^[[JOIN:block\d+]]( // COM: The else-arm spills the same value to the same local // CHECK: ^[[ELSE]]: // CHECK: hir.store_local %[[V]] <{ local = #builtin.local_variable<[[L]], u32> }> // CHECK-NEXT: hir.exec ::@test::@example // CHECK-NOT: hir.store_local +// CHECK: cf.br ^[[JOIN]]( -// COM: The join holds the single reload of the spilled value +// COM: The join block holds the single reload of the spilled value +// CHECK: ^[[JOIN]]( +// CHECK-NOT: hir.load_local +// CHECK-NOT: hir.store_local // CHECK: hir.load_local <{ local = #builtin.local_variable<[[L]], u32> }> // CHECK-NOT: hir.load_local // CHECK-NOT: hir.store_local From f7cd84070823ff6340e830caec6ec201906366d9 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 09:18:56 +0300 Subject: [PATCH 10/19] fix(codegen): treat CFG-cycle re-entry of an enclosing block as reachable in op_reaches op_reaches recognized only region-graph cycles as a re-entry mechanism after the forward block walk failed: for two ops inside an scf.if region whose host block lies on a CFG cycle, the region is not repetitive in the op's own region graph, so the query answered false even though every loop iteration re-enters the region. That breaks the contract that false proves unreachability, and would let pruning erase a spill that a reload on the next iteration reads. The shape cannot be produced by the current pass schedule, which runs the transform on pure CFGs before control flow is lifted and on single-block bodies after, but nothing enforces that precondition and op_reaches is an exported API. Check both re-entry mechanisms at every level of the region ancestry walk: a repetitive enclosing region, or an enclosing op whose block can reach itself through block successors. Extract the forward walk as block_leads_to and the ancestry walk as region_can_re_execute, so the tail of op_reaches reads as the two-sentence summary its doc gives. Pin the repaired behavior with a regression test placing an scf.if on a CFG loop. --- dialects/hir/src/transforms/spill/tests.rs | 44 +++++++++++++++++++++ hir-transform/src/spill.rs | 45 ++++++++++++++++------ 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 2f34882ae..58148a759 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -792,6 +792,50 @@ fn spill_reachability_through_region_re_entry() -> TestResult<()> { Ok(()) } +/// Positional reachability through re-execution of a region whose owner sits on a CFG cycle. +/// +/// The `then` region of the `scf.if` is not repetitive in the op's own region graph, but the +/// block hosting the `scf.if` lies on a CFG cycle, so every iteration re-enters the region: an +/// op positioned after another op inside it still reaches it, through the next iteration. +#[test] +fn op_reaches_through_cfg_cycle_re_entry() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @op_reaches_through_cfg_cycle_re_entry(%n: u32) -> u32 { + %zero = arith.constant 0 : u32; + cf.br ^head(%zero : u32); +^head(%i: u32): + %one = arith.constant 1 : u32; + %c = arith.eq %i, %one; + %r = scf.if %c then { + %early = arith.add %i, %one <{ overflow = #builtin.overflow }>; + %late = arith.add %early, %early <{ overflow = #builtin.overflow }>; + scf.yield %late : (u32); + } else { + scf.yield %i : (u32); + } : (i1) -> (u32); + %done = arith.eq %r, %n; + cf.cond_br %done ^exit, ^head(%r : u32) : (i1); +^exit: + builtin.ret %r : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = + parse_function_fixpoint(&context, "op_reaches_through_cfg_cycle_re_entry.hir", source)?; + + let head = block_at(function, 1); + let if_op = op_at(head, 2); + let early_op = op_in_region(if_op, 0, 0); + let late_op = op_in_region(if_op, 0, 1); + let ret_op = op_at(block_at(function, 2), 0); + + assert!( + op_reaches(late_op, early_op), + "re-entry of a region through an outer CFG cycle must count as reachable" + ); + assert!(!op_reaches(ret_op, early_op), "an op after the loop must not reach into it"); + Ok(()) +} + /// Queries that cross an isolation boundary are not control-flow questions. /// /// Operations in two different functions have no shared control flow to walk, and for the diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index a874bd1ec..f3976af96 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -1006,10 +1006,22 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { } } + if block_leads_to(from_block, to_block) { + return true; + } + + // No forward path within the common region. Control can still come back around to `to` if + // the common region can execute more than once. + region_can_re_execute(common_region.as_region_ref()) +} + +/// Returns true if control leaving the end of `from` can reach the start of `to` by following +/// block successors. +fn block_leads_to(from: BlockRef, to: BlockRef) -> bool { let mut visited = SmallSet::::default(); - let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from_block)); + let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); while let Some(block) = worklist.pop() { - if block == to_block { + if block == to { return true; } if !visited.insert(block) { @@ -1017,21 +1029,26 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { } worklist.extend(BlockRef::children(block)); } + false +} - // No forward path within the common region. Control can still leave the region after - // `from` and come back around to `to` if the common region, or any region enclosing it, - // can execute more than once (e.g. the regions of an `scf.while`): such back edges are - // expressed in the region graph of the owning op, not as block successors. - let mut region = Some(common_region.as_region_ref()); - while let Some(r) = region { +/// Returns true if `region` can execute more than once within a single execution of its +/// innermost isolated-from-above ancestor. +/// +/// That is the case when an enclosing region is repetitive (e.g. the regions of an `scf.while`, +/// whose back edges are expressed in the region graph of the owning op rather than as block +/// successors), or when an enclosing op itself sits on a CFG cycle in its parent region. +fn region_can_re_execute(region: RegionRef) -> bool { + let mut current = Some(region); + while let Some(r) = current { let Some(owner) = r.parent() else { - break; + return false; }; let owner_op = owner.borrow(); if owner_op.implements::() { // An isolated-from-above boundary (e.g. a function): reachability is scoped to a // single execution of its body. - break; + return false; } if !owner_op.implements::() { // Unknown region semantics: conservatively treat re-entry as possible. @@ -1040,9 +1057,13 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { if r.borrow().is_repetitive_region() { return true; } - region = owner_op.parent_region(); + if let Some(owner_block) = owner_op.parent() + && block_leads_to(owner_block, owner_block) + { + return true; + } + current = owner_op.parent_region(); } - false } From e543da9b4dee25c024e38daef8e0322782bbf439 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 09:20:52 +0300 Subject: [PATCH 11/19] test(codegen): name the op_reaches unit tests after their subject Four of the six tests exercising op_reaches carried a spill_reachability_ prefix from before the predicate was renamed, while the newer two already used op_reaches_. Rename the four tests and their fixtures so all six name the unit under test and group together in test listings. --- dialects/hir/src/transforms/spill/tests.rs | 29 ++++++++++------------ 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 58148a759..a4d7a87cf 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -617,8 +617,8 @@ fn materializes_spills_nested_scf_while_after_region() -> TestResult<()> { /// count as reaching the join. Sibling arms must stay mutually unreachable so the dead-edge-spill /// elimination keeps working. #[test] -fn spill_reachability_in_branching_cfg() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @spill_reachability_in_branching_cfg(%a: u32) -> u32 { +fn op_reaches_in_branching_cfg() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @op_reaches_in_branching_cfg(%a: u32) -> u32 { %one = arith.constant 1 : u32; %first = arith.add %a, %one <{ overflow = #builtin.overflow }>; %cond = arith.eq %first, %one; @@ -636,7 +636,7 @@ fn spill_reachability_in_branching_cfg() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "spill_reachability_in_branching_cfg.hir", source)?; + parse_function_fixpoint(&context, "op_reaches_in_branching_cfg.hir", source)?; let entry = block_at(function, 0); let entry_first = op_at(entry, 1); @@ -662,8 +662,8 @@ fn spill_reachability_in_branching_cfg() -> TestResult<()> { /// Positional reachability through a loop back-edge. #[test] -fn spill_reachability_through_loop_back_edge() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @spill_reachability_through_loop_back_edge(%n: u32) -> u32 { +fn op_reaches_through_loop_back_edge() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @op_reaches_through_loop_back_edge(%n: u32) -> u32 { cf.br ^header(%n : u32); ^header(%i: u32): %one = arith.constant 1 : u32; @@ -680,7 +680,7 @@ fn spill_reachability_through_loop_back_edge() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "spill_reachability_through_loop_back_edge.hir", source)?; + parse_function_fixpoint(&context, "op_reaches_through_loop_back_edge.hir", source)?; let header = block_at(function, 1); let header_op = op_at(header, 1); @@ -702,8 +702,8 @@ fn spill_reachability_through_loop_back_edge() -> TestResult<()> { /// Nested operations are normalized to their ancestor in the common region; operations nested /// under the same region-branch op conservatively reach each other (e.g. across loop iterations). #[test] -fn spill_reachability_across_nested_regions() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @spill_reachability_across_nested_regions(%a: u32) -> u32 { +fn op_reaches_across_nested_regions() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @op_reaches_across_nested_regions(%a: u32) -> u32 { %one = arith.constant 1 : u32; %pre = arith.add %a, %one <{ overflow = #builtin.overflow }>; %cond = arith.eq %pre, %one; @@ -720,7 +720,7 @@ fn spill_reachability_across_nested_regions() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "spill_reachability_across_nested_regions.hir", source)?; + parse_function_fixpoint(&context, "op_reaches_across_nested_regions.hir", source)?; let entry = block_at(function, 0); let pre_op = op_at(entry, 1); @@ -749,8 +749,8 @@ fn spill_reachability_across_nested_regions() -> TestResult<()> { /// region graph of the parent op. An op positioned after another op in such a region still /// reaches it, through the next iteration. #[test] -fn spill_reachability_through_region_re_entry() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @spill_reachability_through_region_re_entry(%n: u32) -> u32 { +fn op_reaches_through_region_re_entry() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @op_reaches_through_region_re_entry(%n: u32) -> u32 { %zero = arith.constant 0 : u32; %r = scf.while %zero before { ^head(%i: u32): @@ -768,11 +768,8 @@ fn spill_reachability_through_region_re_entry() -> TestResult<()> { };"#; let context = Rc::new(Context::default()); - let (function, _) = parse_function_fixpoint( - &context, - "spill_reachability_through_region_re_entry.hir", - source, - )?; + let (function, _) = + parse_function_fixpoint(&context, "op_reaches_through_region_re_entry.hir", source)?; let entry = block_at(function, 0); let while_op = op_at(entry, 1); From 55ff70d358e1998cdf0e3487582b853de2b3a978 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 10:15:34 +0300 Subject: [PATCH 12/19] refactor(codegen): pair reloads with spills via the analysis value in pruning The pruning loop re-derived the spilled value from the spill op's current operand through a SpillLike cast, then matched it against ReloadInfo::value. SpillInfo::value is the same analysis-domain identity, so the derivation was an indirection with a hidden assumption baked in: SSA reconstruction exempts only reload operands from rewriting, so a spill placed after a reload of the same value on one path would have its operand redirected, match no reloads, and be erased despite covering them. The analysis does not produce that shape today, but the pruning decision should not depend on it. Match the analysis's own value on both sides and drop the derivation. --- hir-transform/src/spill.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index f3976af96..02956da5b 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -863,17 +863,12 @@ fn rewrite_spill_pseudo_instructions( .with_listener(TracingRewriterListener); for spill in analysis.spills() { let operation = spill.inst.expect("expected spill to have been materialized"); - let spilled = { - let op = operation.borrow(); - let spill_like = op - .as_trait::() - .expect("expected materialized spill operation to implement SpillLike"); - spill_like.spilled_value() - }; - // Only keep spills that can reach a live reload of their value + // Only keep spills that can reach a live reload of their value. Spills and reloads are + // paired through the analysis's value bookkeeping rather than the spill op's current + // operand, which SSA reconstruction may rewrite (only reload operands are exempt). let mut is_used = false; for rinfo in analysis.reloads() { - if rinfo.value != spilled { + if rinfo.value != spill.value { continue; } let Some(reload_op) = rinfo.inst else { From f6f37b3ddc490e292a6ab46a07c206467bcb8d9a Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 10:16:15 +0300 Subject: [PATCH 13/19] docs(codegen): document the reachability helpers' subtle contracts Three clarifications that protect invariants a future refactor could otherwise break: block_leads_to is not reflexive (a self-query is a cycle test, which region_can_re_execute relies on), the two isolation guards in op_reaches cover disjoint cases and neither subsumes the other, and op_reaches is exported chiefly for the behavioral tests in midenc-dialect-hir rather than as a stable public surface. --- hir-transform/src/spill.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 02956da5b..35254b079 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -940,6 +940,10 @@ fn rewrite_spill_pseudo_instructions( /// ancestor (e.g. one function invocation). Queries that cross an isolation boundary, or whose /// positions cannot otherwise be related (graph regions, no common ancestor region), are /// conservatively reachable. +/// +/// This query is exported from the crate root chiefly so the behavioral tests in +/// `midenc-dialect-hir` (which have the parser and control-flow dialects needed to build +/// fixtures) can exercise it directly; the in-crate consumer is spill pruning. pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { // Operations in different isolation scopes (e.g. two functions) share no control flow to // walk, so the query cannot be answered; report the conservative `true`. @@ -973,7 +977,10 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { // Ancestors that are themselves isolated from above (e.g. two functions in one module) are // symbol-container members, not control-flow positions; their block order proves nothing, - // so conservatively report reachable. + // so conservatively report reachable. This is not redundant with the isolation-scope guard + // above: sibling isolated ops share one scope and only this check catches them, while an + // isolated op nested under a non-isolated region op normalizes to that non-isolated + // wrapper and is caught only by the scope guard. if from_ancestor.borrow().implements::() || to_ancestor.borrow().implements::() { @@ -1012,6 +1019,9 @@ pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { /// Returns true if control leaving the end of `from` can reach the start of `to` by following /// block successors. +/// +/// The walk is not reflexive: `from == to` returns true only when a cycle leads back into the +/// block, which is what [region_can_re_execute] relies on for cycle detection. fn block_leads_to(from: BlockRef, to: BlockRef) -> bool { let mut visited = SmallSet::::default(); let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); From ca7b4dce2d1ba89de440163e4456bc91550ecb8d Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 6 Jul 2026 11:37:31 +0300 Subject: [PATCH 14/19] fix(codegen): key spill locals by the analysis value, not the spill operand Spill pruning pairs spills with reloads through the analysis's value bookkeeping, but materialization still keyed the per-value procedure local by the spill op's current operand while reload lookup used the reload's operand. SSA reconstruction exempts only reload operands from use rewriting, so a kept spill whose operand was redirected (e.g. to a preceding reload's result or an inserted phi) allocated and wrote a second local that no reload reads; had every spill of a value been rewritten, covered reloads would have failed with the internal error on IR the analysis handled correctly. Thread the analysis value through TransformSpillsInterface and key the locals map by it on both sides, while the store keeps writing the op's current operand, which is the correctly split live range at that point and always carries the same runtime value. This makes the internal error unreachable for covered reloads and drops the reliance on reload operands being exempt from rewriting. Pin the behavior with a synthesized spill-reload-spill-reload chain over one value, where the rewrite phase redirects the second spill's operand to the first reload's result: the transform must materialize a single shared local, storing the reload result into it. --- dialects/hir/src/transforms/spill.rs | 18 ++-- dialects/hir/src/transforms/spill/tests.rs | 96 ++++++++++++++++++++++ hir-transform/src/spill.rs | 13 ++- 3 files changed, 118 insertions(+), 9 deletions(-) diff --git a/dialects/hir/src/transforms/spill.rs b/dialects/hir/src/transforms/spill.rs index 3d38f2564..fad49cff1 100644 --- a/dialects/hir/src/transforms/spill.rs +++ b/dialects/hir/src/transforms/spill.rs @@ -8,7 +8,7 @@ use midenc_hir::{ pass::{Pass, PassExecutionState, PostPassStatus}, }; use midenc_hir_analysis::analyses::SpillAnalysis; -use midenc_hir_transform::{self as transforms, ReloadLike, SpillLike, TransformSpillsInterface}; +use midenc_hir_transform::{self as transforms, SpillLike, TransformSpillsInterface}; #[derive(Default)] pub struct TransformSpills; @@ -134,14 +134,18 @@ impl TransformSpillsInterface for TransformSpillsImpl { &mut self, rewriter: &mut dyn Rewriter, spill: OperationRef, + value: ValueRef, ) -> Result<(), Report> { use crate::HirOpBuilder; + // The local is keyed by the analysis's identity of the spilled value; the stored operand + // is the op's current one, which SSA reconstruction may have rewritten (e.g. to a + // preceding reload's result), but always carries the same runtime value. let spilled = spill.borrow().as_trait::().unwrap().spilled_value(); let mut function = self.function; - let local = *self.locals.entry(spilled).or_insert_with(|| { + let local = *self.locals.entry(value).or_insert_with(|| { let mut function = function.borrow_mut(); - function.alloc_local(spilled.borrow().ty().clone()) + function.alloc_local(value.borrow().ty().clone()) }); let store = rewriter.store_local(local, spilled, spill.span())?; @@ -155,16 +159,16 @@ impl TransformSpillsInterface for TransformSpillsImpl { &mut self, rewriter: &mut dyn Rewriter, reload: OperationRef, + value: ValueRef, ) -> Result<(), Report> { use crate::HirOpBuilder; - let spilled = reload.borrow().as_trait::().unwrap().spilled_value(); - let Some(local) = self.locals.get(&spilled).copied() else { + let Some(local) = self.locals.get(&value).copied() else { let function = self.function.borrow(); let function = function.get_name(); return Err(Report::msg(format!( - "internal error: live reload of {spilled} in {function} has no corresponding \ - spill: every kept reload must be covered by at least one spill of the same value" + "internal error: live reload of {value} in {function} has no corresponding spill: \ + every kept reload must be covered by at least one spill of the same value" ))); }; let reloaded = rewriter.load_local(local, reload.span())?; diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index a4d7a87cf..1a1644995 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -610,6 +610,102 @@ fn materializes_spills_nested_scf_while_after_region() -> TestResult<()> { Ok(()) } +/// All spills of one value must share one procedure local, even when SSA reconstruction has +/// rewritten a spill's operand. +/// +/// We synthesize a `spill; reload; spill; reload` chain over a single value: the rewrite phase +/// redirects the second spill's operand to the first reload's result (spill operands are not +/// exempt from use rewriting), so pairing the local slot by the op operand would allocate a +/// second local that no reload ever reads. Materialization must key locals by the value tracked +/// in the spills analysis, storing the (possibly rewritten) operand into the shared slot. +#[test] +fn materializes_spills_shared_local_for_rewritten_spill() -> TestResult<()> { + let mut test = + Test::named("materializes_spills_shared_local_for_rewritten_spill").in_module("test"); + let span = SourceSpan::UNKNOWN; + + test.with_function(test.name(), &[Type::U32], &[Type::U32]); + let func = test.function(); + + let (spilled_value, spill_points, reload_points) = { + let mut b = test.function_builder(); + let entry = b.current_block(); + let v0 = entry.borrow().arguments()[0] as ValueRef; + let k1 = b.u32(1, span); + let v = b.add_unchecked(v0, k1, span)?; + // Anchor operations to place the spill/reload pseudo-ops in front of + let a1 = b.add_unchecked(v0, v0, span)?; + let a2 = b.add_unchecked(a1, k1, span)?; + let a3 = b.add_unchecked(a2, k1, span)?; + let a4 = b.add_unchecked(a3, k1, span)?; + // The post-reload use of the spilled value + let out = b.add_unchecked(v, a4, span)?; + b.ret(Some(out), span)?; + + let anchor = |value: ValueRef| { + ProgramPoint::before( + value.borrow().get_defining_op().expect("expected anchor to have a defining op"), + ) + }; + (v, [anchor(a1), anchor(a3)], [anchor(a2), anchor(a4)]) + }; + + let mut analysis = midenc_hir_analysis::analyses::SpillAnalysis::default(); + analysis.spilled.insert(spilled_value); + for (index, place) in spill_points.into_iter().enumerate() { + analysis.spills.push(midenc_hir_analysis::analyses::spills::SpillInfo { + id: midenc_hir_analysis::analyses::spills::Spill::new(index), + place: midenc_hir_analysis::analyses::spills::Placement::At(place), + value: spilled_value, + span, + inst: None, + }); + } + for (index, place) in reload_points.into_iter().enumerate() { + analysis.reloads.push(midenc_hir_analysis::analyses::spills::ReloadInfo { + id: midenc_hir_analysis::analyses::spills::Reload::new(index), + place: midenc_hir_analysis::analyses::spills::Placement::At(place), + value: spilled_value, + span, + inst: None, + }); + } + + let mut interface = super::TransformSpillsImpl { + function: func, + locals: Default::default(), + }; + let analysis_manager = midenc_hir::pass::AnalysisManager::new(func.as_operation_ref(), None); + midenc_hir_transform::transform_spills( + func.as_operation_ref(), + &mut analysis, + &mut interface, + analysis_manager, + )?; + + let after = func.as_operation_ref().borrow().to_string(); + std::println!("{after}"); + + assert_eq!( + func.borrow().num_locals(), + 1, + "all spills of one value must share one procedure local\n{after}" + ); + // The second store must write the first reload's result into the same local, proving the + // rewritten operand landed in the shared slot + filecheck!( + &after, + r#" +; CHECK: hir.store_local %{{\d+}} <{ local = #builtin.local_variable<[[L:\d+]], u32> }> +; CHECK: %[[R0:\d+]] = hir.load_local <{ local = #builtin.local_variable<[[L]], u32> }> +; CHECK: hir.store_local %[[R0]] <{ local = #builtin.local_variable<[[L]], u32> }> +; CHECK: hir.load_local <{ local = #builtin.local_variable<[[L]], u32> }> +"# + ); + + Ok(()) +} + /// Positional reachability over a diamond CFG. /// /// This pins the join-covered shape that dominance-based spill pruning got wrong: a reload after diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 35254b079..e0b0aeea0 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -46,18 +46,27 @@ pub trait TransformSpillsInterface { /// Convert `spill`, a [SpillLike] operation, into a primitive memory store of the spilled /// value. + /// + /// `value` is the spilled value as tracked by the spills analysis; implementations must key + /// any per-value storage by it, while storing the operation's current operand (the live SSA + /// name at that point, which SSA reconstruction may have rewritten). fn convert_spill_to_store( &mut self, rewriter: &mut dyn Rewriter, spill: OperationRef, + value: ValueRef, ) -> Result<(), Report>; /// Convert `reload`, a [ReloadLike] operation, into a primitive memory load of the spilled /// value. + /// + /// `value` is the spilled value as tracked by the spills analysis; the lookup must use the + /// same key as [Self::convert_spill_to_store]. fn convert_reload_to_load( &mut self, rewriter: &mut dyn Rewriter, reload: OperationRef, + value: ValueRef, ) -> Result<(), Report>; } @@ -889,7 +898,7 @@ fn rewrite_spill_pseudo_instructions( if is_used { builder.set_insertion_point_after(operation); - interface.convert_spill_to_store(&mut builder, operation)?; + interface.convert_spill_to_store(&mut builder, operation, spill.value)?; } else { builder.erase_op(operation); } @@ -915,7 +924,7 @@ fn rewrite_spill_pseudo_instructions( reload.place ); builder.set_insertion_point_after(operation); - interface.convert_reload_to_load(&mut builder, operation)?; + interface.convert_reload_to_load(&mut builder, operation, reload.value)?; } else { log::trace!( target: trace_target, From 7928657a303dd28b9ba315ce7d14e8662d1ae2cd Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Fri, 10 Jul 2026 11:48:22 +0300 Subject: [PATCH 15/19] refactor(hir): replace op_reaches with Operation::reachability Implements the review feedback on the reachability query used by spill pruning. The boolean op_reaches helper over-claimed its name and folded every unanswerable case into a conservative true, and it reasoned about query boundaries via IsolatedFromAbove, which governs SSA value visibility and implies nothing about control flow. Move the query into midenc-hir as Operation::reachability, a general positional query returning a Reachability classification: Impossible and Guaranteed are proofs, Maybe covers paths whose executability only a proper reachability analysis (in concert with SCCP/DCE) could decide, and MaybeInterprocedurally/Indeterminate report queries that leave a single function's control flow or land in a graph-like region, leaving their interpretation to the caller. All boundary reasoning is now based on region kinds: the intra-procedural scope is bounded by the nearest ancestor residing in a graph-like region (in practice the enclosing function), the same boundary terminates the re-execution walk, and a graph-like common ancestor region is Indeterminate. A missing common ancestor region is now correctly Impossible rather than conservatively kept, since any control-flow path would itself lie in a common region. Spill pruning keeps a spill for Guaranteed/Maybe reloads, treats Impossible as not covering, and fails with an internal error on MaybeInterprocedurally/Indeterminate, both of which are invalid IR for a spill/reload pair. Also replaces the intra-doc link to the private rewrite function with plain code formatting, which previously broke cargo doc under deny(warnings). --- dialects/hir/src/transforms/spill/tests.rs | 203 +++++++++++++------- hir-transform/src/lib.rs | 2 +- hir-transform/src/spill.rs | 197 ++++--------------- hir/src/ir.rs | 2 + hir/src/ir/reachability.rs | 213 +++++++++++++++++++++ 5 files changed, 382 insertions(+), 235 deletions(-) create mode 100644 hir/src/ir/reachability.rs diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 1a1644995..037abb3fe 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -6,19 +6,23 @@ use midenc_dialect_cf::ControlFlowOpBuilder as Cf; use midenc_dialect_scf::StructuredControlFlowOpBuilder; use midenc_expect_test::expect_file; use midenc_hir::{ - AddressSpace, BlockRef, Builder, Context, Op, OperationRef, PointerType, ProgramPoint, Report, - SourceSpan, Type, ValueRef, + AddressSpace, BlockRef, Builder, Context, Op, Operation, OperationRef, PointerType, + ProgramPoint, Reachability, Report, SourceSpan, Type, ValueRef, diagnostics::Uri, dialects::builtin::{BuiltinOpBuilder, FunctionRef}, parse::{self, ParserConfig}, testing::{Test, parse_function_fixpoint}, }; -use midenc_hir_transform::op_reaches; use crate::{HirOpBuilder, transforms::TransformSpills}; type TestResult = Result; +/// Shorthand for [Operation::reachability] in assertions. +fn reach(from: OperationRef, to: OperationRef) -> Reachability { + Operation::reachability(from, to) +} + /// Returns the `index`-th block in the body of `function`. fn block_at(function: FunctionRef, index: usize) -> BlockRef { let function = function.borrow(); @@ -710,11 +714,11 @@ fn materializes_spills_shared_local_for_rewritten_spill() -> TestResult<()> { /// /// This pins the join-covered shape that dominance-based spill pruning got wrong: a reload after /// the join is covered by the set of per-arm spills, none of which dominates it, so each arm must -/// count as reaching the join. Sibling arms must stay mutually unreachable so the dead-edge-spill -/// elimination keeps working. +/// count as (maybe) reaching the join. Sibling arms must stay provably unreachable from each +/// other so the dead-edge-spill elimination keeps working. #[test] -fn op_reaches_in_branching_cfg() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @op_reaches_in_branching_cfg(%a: u32) -> u32 { +fn reachability_in_branching_cfg() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_in_branching_cfg(%a: u32) -> u32 { %one = arith.constant 1 : u32; %first = arith.add %a, %one <{ overflow = #builtin.overflow }>; %cond = arith.eq %first, %one; @@ -732,7 +736,7 @@ fn op_reaches_in_branching_cfg() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "op_reaches_in_branching_cfg.hir", source)?; + parse_function_fixpoint(&context, "reachability_in_branching_cfg.hir", source)?; let entry = block_at(function, 0); let entry_first = op_at(entry, 1); @@ -741,25 +745,31 @@ fn op_reaches_in_branching_cfg() -> TestResult<()> { let right_op = op_at(block_at(function, 2), 0); let join_op = op_at(block_at(function, 3), 0); - assert!(op_reaches(left_op, join_op), "arm must reach the join"); - assert!(op_reaches(right_op, join_op), "arm must reach the join"); - assert!(!op_reaches(left_op, right_op), "sibling arms must not reach each other"); - assert!( - op_reaches(entry_first, entry_second), - "earlier op must reach a later op in the same block" + assert_eq!(reach(left_op, join_op), Reachability::Maybe, "arm must reach the join"); + assert_eq!(reach(right_op, join_op), Reachability::Maybe, "arm must reach the join"); + assert_eq!( + reach(left_op, right_op), + Reachability::Impossible, + "sibling arms must not reach each other" + ); + assert_eq!( + reach(entry_first, entry_second), + Reachability::Guaranteed, + "an earlier op always flows into a later op in the same block" ); - assert!( - !op_reaches(entry_second, entry_first), + assert_eq!( + reach(entry_second, entry_first), + Reachability::Impossible, "later op must not reach an earlier op without a cycle" ); - assert!(!op_reaches(join_op, left_op), "join must not reach an arm"); + assert_eq!(reach(join_op, left_op), Reachability::Impossible, "join must not reach an arm"); Ok(()) } /// Positional reachability through a loop back-edge. #[test] -fn op_reaches_through_loop_back_edge() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @op_reaches_through_loop_back_edge(%n: u32) -> u32 { +fn reachability_through_loop_back_edge() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_through_loop_back_edge(%n: u32) -> u32 { cf.br ^header(%n : u32); ^header(%i: u32): %one = arith.constant 1 : u32; @@ -776,7 +786,7 @@ fn op_reaches_through_loop_back_edge() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "op_reaches_through_loop_back_edge.hir", source)?; + parse_function_fixpoint(&context, "reachability_through_loop_back_edge.hir", source)?; let header = block_at(function, 1); let header_op = op_at(header, 1); @@ -784,22 +794,33 @@ fn op_reaches_through_loop_back_edge() -> TestResult<()> { let body_op = op_at(block_at(function, 2), 0); let exit_op = op_at(block_at(function, 3), 0); - assert!(op_reaches(body_op, header_op), "back edge must reach the header"); - assert!( - op_reaches(header_second, header_op), + assert_eq!( + reach(body_op, header_op), + Reachability::Maybe, + "back edge must reach the header" + ); + assert_eq!( + reach(header_second, header_op), + Reachability::Maybe, "later op must reach an earlier op in the same block through the loop" ); - assert!(!op_reaches(exit_op, body_op), "exit must not reach the loop body"); + assert_eq!( + reach(exit_op, body_op), + Reachability::Impossible, + "exit must not reach the loop body" + ); Ok(()) } /// Positional reachability with operations nested in `scf.if` regions. /// -/// Nested operations are normalized to their ancestor in the common region; operations nested -/// under the same region-branch op conservatively reach each other (e.g. across loop iterations). +/// Nested operations are normalized to their ancestor in the common region; positions involving +/// a normalized side are at most [Reachability::Maybe], since entering a sub-region depends on +/// the region op's semantics, and operations nested under the same region-branch op +/// conservatively reach each other (e.g. across loop iterations). #[test] -fn op_reaches_across_nested_regions() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @op_reaches_across_nested_regions(%a: u32) -> u32 { +fn reachability_across_nested_regions() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_across_nested_regions(%a: u32) -> u32 { %one = arith.constant 1 : u32; %pre = arith.add %a, %one <{ overflow = #builtin.overflow }>; %cond = arith.eq %pre, %one; @@ -816,7 +837,7 @@ fn op_reaches_across_nested_regions() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "op_reaches_across_nested_regions.hir", source)?; + parse_function_fixpoint(&context, "reachability_across_nested_regions.hir", source)?; let entry = block_at(function, 0); let pre_op = op_at(entry, 1); @@ -825,16 +846,36 @@ fn op_reaches_across_nested_regions() -> TestResult<()> { let then_op = op_in_region(if_op, 0, 0); let else_op = op_in_region(if_op, 1, 0); - assert!(op_reaches(pre_op, then_op), "op before the region op must reach into it"); - assert!(op_reaches(then_op, post_op), "nested op must reach past the region op"); - assert!( - !op_reaches(post_op, then_op), + assert_eq!( + reach(pre_op, then_op), + Reachability::Maybe, + "op before the region op must reach into it" + ); + assert_eq!( + reach(then_op, post_op), + Reachability::Maybe, + "nested op must reach past the region op" + ); + assert_eq!( + reach(post_op, then_op), + Reachability::Impossible, "op after the region op must not reach back into it" ); - assert!( - op_reaches(then_op, else_op), + assert_eq!( + reach(then_op, else_op), + Reachability::Maybe, "sibling regions of one op conservatively reach each other" ); + assert_eq!( + reach(if_op, then_op), + Reachability::Maybe, + "a region op may reach the operations nested within it" + ); + assert_eq!( + reach(then_op, if_op), + Reachability::Maybe, + "a nested operation flows back through its region op" + ); Ok(()) } @@ -845,8 +886,8 @@ fn op_reaches_across_nested_regions() -> TestResult<()> { /// region graph of the parent op. An op positioned after another op in such a region still /// reaches it, through the next iteration. #[test] -fn op_reaches_through_region_re_entry() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @op_reaches_through_region_re_entry(%n: u32) -> u32 { +fn reachability_through_region_re_entry() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_through_region_re_entry(%n: u32) -> u32 { %zero = arith.constant 0 : u32; %r = scf.while %zero before { ^head(%i: u32): @@ -865,7 +906,7 @@ fn op_reaches_through_region_re_entry() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "op_reaches_through_region_re_entry.hir", source)?; + parse_function_fixpoint(&context, "reachability_through_region_re_entry.hir", source)?; let entry = block_at(function, 0); let while_op = op_at(entry, 1); @@ -873,15 +914,21 @@ fn op_reaches_through_region_re_entry() -> TestResult<()> { let early_op = op_in_region(while_op, 0, 1); let late_op = op_in_region(while_op, 0, 2); - assert!( - op_reaches(late_op, early_op), + assert_eq!( + reach(late_op, early_op), + Reachability::Maybe, "a later op must reach an earlier op in the same repetitive region through re-entry" ); - assert!( - op_reaches(early_op, late_op), - "earlier op must reach a later op in the same block" + assert_eq!( + reach(early_op, late_op), + Reachability::Guaranteed, + "an earlier op always flows into a later op in the same block" + ); + assert_eq!( + reach(ret_op, late_op), + Reachability::Impossible, + "an op after the loop must not reach into it" ); - assert!(!op_reaches(ret_op, late_op), "an op after the loop must not reach into it"); Ok(()) } @@ -891,8 +938,8 @@ fn op_reaches_through_region_re_entry() -> TestResult<()> { /// block hosting the `scf.if` lies on a CFG cycle, so every iteration re-enters the region: an /// op positioned after another op inside it still reaches it, through the next iteration. #[test] -fn op_reaches_through_cfg_cycle_re_entry() -> TestResult<()> { - let source = r#"builtin.function public extern("C") @op_reaches_through_cfg_cycle_re_entry(%n: u32) -> u32 { +fn reachability_through_cfg_cycle_re_entry() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_through_cfg_cycle_re_entry(%n: u32) -> u32 { %zero = arith.constant 0 : u32; cf.br ^head(%zero : u32); ^head(%i: u32): @@ -913,7 +960,7 @@ fn op_reaches_through_cfg_cycle_re_entry() -> TestResult<()> { let context = Rc::new(Context::default()); let (function, _) = - parse_function_fixpoint(&context, "op_reaches_through_cfg_cycle_re_entry.hir", source)?; + parse_function_fixpoint(&context, "reachability_through_cfg_cycle_re_entry.hir", source)?; let head = block_at(function, 1); let if_op = op_at(head, 2); @@ -921,22 +968,26 @@ fn op_reaches_through_cfg_cycle_re_entry() -> TestResult<()> { let late_op = op_in_region(if_op, 0, 1); let ret_op = op_at(block_at(function, 2), 0); - assert!( - op_reaches(late_op, early_op), + assert_eq!( + reach(late_op, early_op), + Reachability::Maybe, "re-entry of a region through an outer CFG cycle must count as reachable" ); - assert!(!op_reaches(ret_op, early_op), "an op after the loop must not reach into it"); + assert_eq!( + reach(ret_op, early_op), + Reachability::Impossible, + "an op after the loop must not reach into it" + ); Ok(()) } -/// Queries that cross an isolation boundary are not control-flow questions. +/// Queries that leave a single function's control flow have no positional answer. /// -/// Operations in two different functions have no shared control flow to walk, and for the -/// function operations themselves the module body is a graph region where block order proves -/// nothing. All such queries must answer with the conservative `true` in both directions, so -/// that `false` remains a proof of unreachability. +/// Operations in two different functions can only be related interprocedurally, and for the +/// function operations themselves the module body is a graph-like region where operation order +/// does not define control flow. Both classifications are direction-independent. #[test] -fn op_reaches_conservative_across_isolation_boundaries() -> TestResult<()> { +fn reachability_across_functions_and_graph_regions() -> TestResult<()> { let source = r#"builtin.module public @test { builtin.function public extern("C") @first(%a: u32) -> u32 { %r = arith.add %a, %a <{ overflow = #builtin.overflow }>; @@ -951,7 +1002,7 @@ fn op_reaches_conservative_across_isolation_boundaries() -> TestResult<()> { let context = Rc::new(Context::default()); let module_op = parse::parse_any( ParserConfig::new(context.clone()), - Uri::new("op_reaches_conservative_across_isolation_boundaries.hir"), + Uri::new("reachability_across_functions_and_graph_regions.hir"), source, )?; @@ -960,21 +1011,35 @@ fn op_reaches_conservative_across_isolation_boundaries() -> TestResult<()> { let in_first = op_in_region(first_fn, 0, 0); let in_second = op_in_region(second_fn, 0, 0); - assert!( - op_reaches(in_second, in_first), - "ops in different functions must be conservatively reachable regardless of module order" + assert_eq!( + reach(in_second, in_first), + Reachability::MaybeInterprocedurally, + "ops in different functions relate only interprocedurally, regardless of module order" ); - assert!( - op_reaches(in_first, in_second), - "ops in different functions must be conservatively reachable" + assert_eq!( + reach(in_first, in_second), + Reachability::MaybeInterprocedurally, + "ops in different functions relate only interprocedurally" ); - assert!( - op_reaches(second_fn, first_fn), - "sibling isolated ops must be conservatively reachable regardless of module order" + assert_eq!( + reach(second_fn, first_fn), + Reachability::Indeterminate, + "graph-region op order does not define control flow, regardless of module order" ); - assert!( - op_reaches(first_fn, second_fn), - "sibling isolated ops must be conservatively reachable" + assert_eq!( + reach(first_fn, second_fn), + Reachability::Indeterminate, + "graph-region op order does not define control flow" + ); + assert_eq!( + reach(first_fn, in_first), + Reachability::Maybe, + "enclosure is intra-procedural even when the encloser resides in a graph region" + ); + assert_eq!( + reach(in_first, first_fn), + Reachability::Maybe, + "enclosure is intra-procedural in both directions" ); Ok(()) } diff --git a/hir-transform/src/lib.rs b/hir-transform/src/lib.rs index 155a5d7e2..1394fd948 100644 --- a/hir-transform/src/lib.rs +++ b/hir-transform/src/lib.rs @@ -22,5 +22,5 @@ pub use self::{ cse::CommonSubexpressionElimination, sccp::SparseConditionalConstantPropagation, sink::{ControlFlowSink, SinkOperandDefs}, - spill::{ReloadLike, SpillLike, TransformSpillsInterface, op_reaches, transform_spills}, + spill::{ReloadLike, SpillLike, TransformSpillsInterface, transform_spills}, }; diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index e0b0aeea0..41547d169 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -1,9 +1,10 @@ -use alloc::{collections::VecDeque, rc::Rc}; +use alloc::{collections::VecDeque, format, rc::Rc}; use midenc_hir::{ BlockRef, Builder, Context, FxHashMap, OpBuilder, OpOperand, Operation, OperationRef, - ProgramPoint, Region, RegionBranchOpInterface, RegionBranchPoint, RegionRef, Report, Rewriter, - SmallVec, SourceSpan, Spanned, StorableEntity, TraceTarget, Usable, ValueRange, ValueRef, + ProgramPoint, Reachability, Region, RegionBranchOpInterface, RegionBranchPoint, RegionRef, + Report, Rewriter, SmallVec, SourceSpan, Spanned, StorableEntity, TraceTarget, Usable, + ValueRange, ValueRef, adt::{SmallDenseMap, SmallSet}, cfg::Graph, dominance::{DomTreeNode, DominanceFrontier, DominanceInfo}, @@ -120,7 +121,7 @@ pub trait ReloadLike { /// argument/phi representing the closest definition of that value from each predecessor. /// * Rewrites all spill and reload instructions to their primitive memory store/load ops. /// Dominance governs only the SSA use rewrite above; whether a spill is materialized or elided -/// is decided by reachability to live reloads (see [rewrite_spill_pseudo_instructions]). +/// is decided by reachability to live reloads (see `rewrite_spill_pseudo_instructions`). pub fn transform_spills( op: OperationRef, analysis: &mut SpillAnalysis, @@ -857,9 +858,9 @@ fn rewrite_inserted_phi_uses( /// Reachability, not dominance, is the criterion: the spills analysis places spills per path /// (e.g. along each edge of a join), so a reload after the join is covered by a set of spills, /// none of which individually dominates it. Eliding a spill is only sound when it provably -/// cannot reach any live reload of its value, otherwise some path would reload from a local -/// that was never written. Locals are allocated per invocation, so function re-entry is -/// irrelevant to coverage, matching the single-execution semantics of [op_reaches]. +/// cannot reach any live reload of its value ([Reachability::Impossible]), otherwise some path +/// would reload from a local that was never written. A spill/reload pair related across +/// functions, or through a graph-like region, is invalid IR and reported as an error. fn rewrite_spill_pseudo_instructions( context: Rc, analysis: &mut SpillAnalysis, @@ -890,9 +891,30 @@ fn rewrite_spill_pseudo_instructions( .expect("expected materialized reload op to implement ReloadLike"); rl.reloaded().borrow().is_used() }; - if reload_used && op_reaches(operation, reload_op) { - is_used = true; - break; + if !reload_used { + continue; + } + match Operation::reachability(operation, reload_op) { + Reachability::Guaranteed | Reachability::Maybe => { + is_used = true; + break; + } + // This spill cannot cover the reload; other spills of the value may. + Reachability::Impossible => {} + Reachability::MaybeInterprocedurally => { + return Err(Report::msg(format!( + "internal error: a spill of {} and a reload of it are in different \ + functions", + spill.value + ))); + } + Reachability::Indeterminate => { + return Err(Report::msg(format!( + "internal error: control flow between a spill of {} and a reload of it is \ + not well-defined", + spill.value + ))); + } } } @@ -938,158 +960,3 @@ fn rewrite_spill_pseudo_instructions( Ok(()) } - -/// Returns true if some control-flow path from `from` may reach `to`. -/// -/// The result may be conservatively `true` when reachability cannot be reasoned about precisely; -/// `false` means `to` is provably unreachable from `from`. Callers making elision decisions -/// (e.g. spill pruning) rely on the `false` direction being a proof. -/// -/// Reachability is evaluated within a single execution of the innermost isolated-from-above -/// ancestor (e.g. one function invocation). Queries that cross an isolation boundary, or whose -/// positions cannot otherwise be related (graph regions, no common ancestor region), are -/// conservatively reachable. -/// -/// This query is exported from the crate root chiefly so the behavioral tests in -/// `midenc-dialect-hir` (which have the parser and control-flow dialects needed to build -/// fixtures) can exercise it directly; the in-crate consumer is spill pruning. -pub fn op_reaches(from: OperationRef, to: OperationRef) -> bool { - // Operations in different isolation scopes (e.g. two functions) share no control flow to - // walk, so the query cannot be answered; report the conservative `true`. - if isolation_scope(from) != isolation_scope(to) { - return true; - } - - // Normalize both operations to the innermost region containing them both: an operation - // nested in a sub-region (e.g. structured control flow) is represented by its ancestor - // operation in the common region. - let Some(common_region) = Region::find_common_ancestor(&[from, to]) else { - // Operations without a common ancestor region cannot be reasoned about. - return true; - }; - let common_region = common_region.borrow(); - let (Some(from_ancestor), Some(to_ancestor)) = - (common_region.find_ancestor_op(from), common_region.find_ancestor_op(to)) - else { - // Unreachable per find_common_ancestor's postcondition (the returned region contains - // every queried op); kept as a defensive fallback. - return true; - }; - - // Both operations normalize to the same ancestor op: either one op encloses the other (and - // trivially reaches it), or they sit in different sub-regions of that op, where transfer - // between the regions depends on the op's semantics (e.g. it can happen across loop - // iterations), so conservatively assume it does. - if from_ancestor == to_ancestor { - return true; - } - - // Ancestors that are themselves isolated from above (e.g. two functions in one module) are - // symbol-container members, not control-flow positions; their block order proves nothing, - // so conservatively report reachable. This is not redundant with the isolation-scope guard - // above: sibling isolated ops share one scope and only this check catches them, while an - // isolated op nested under a non-isolated region op normalizes to that non-isolated - // wrapper and is caught only by the scope guard. - if from_ancestor.borrow().implements::() - || to_ancestor.borrow().implements::() - { - return true; - } - - let (Some(from_block), Some(to_block)) = - (from_ancestor.borrow().parent(), to_ancestor.borrow().parent()) - else { - return true; - }; - - if from_block == to_block { - // In a region without SSA dominance, block order does not imply control-flow order - // (mirroring how dominance treats same-block queries in such regions), so the - // operations are conservatively mutually reachable. - if !from_block.borrow().has_ssa_dominance() { - return true; - } - // Within one block an operation directly reaches every later operation; earlier - // operations are only reachable through a cycle back into the block, which the - // successor walk and the repetitive-region check below find. - if from_ancestor.borrow().is_before_in_block(&to_ancestor) { - return true; - } - } - - if block_leads_to(from_block, to_block) { - return true; - } - - // No forward path within the common region. Control can still come back around to `to` if - // the common region can execute more than once. - region_can_re_execute(common_region.as_region_ref()) -} - -/// Returns true if control leaving the end of `from` can reach the start of `to` by following -/// block successors. -/// -/// The walk is not reflexive: `from == to` returns true only when a cycle leads back into the -/// block, which is what [region_can_re_execute] relies on for cycle detection. -fn block_leads_to(from: BlockRef, to: BlockRef) -> bool { - let mut visited = SmallSet::::default(); - let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); - while let Some(block) = worklist.pop() { - if block == to { - return true; - } - if !visited.insert(block) { - continue; - } - worklist.extend(BlockRef::children(block)); - } - false -} - -/// Returns true if `region` can execute more than once within a single execution of its -/// innermost isolated-from-above ancestor. -/// -/// That is the case when an enclosing region is repetitive (e.g. the regions of an `scf.while`, -/// whose back edges are expressed in the region graph of the owning op rather than as block -/// successors), or when an enclosing op itself sits on a CFG cycle in its parent region. -fn region_can_re_execute(region: RegionRef) -> bool { - let mut current = Some(region); - while let Some(r) = current { - let Some(owner) = r.parent() else { - return false; - }; - let owner_op = owner.borrow(); - if owner_op.implements::() { - // An isolated-from-above boundary (e.g. a function): reachability is scoped to a - // single execution of its body. - return false; - } - if !owner_op.implements::() { - // Unknown region semantics: conservatively treat re-entry as possible. - return true; - } - if r.borrow().is_repetitive_region() { - return true; - } - if let Some(owner_block) = owner_op.parent() - && block_leads_to(owner_block, owner_block) - { - return true; - } - current = owner_op.parent_region(); - } - false -} - -/// Returns the nearest proper ancestor of `op` that is isolated from above, i.e. the operation -/// whose single execution scopes a reachability query involving `op`. -fn isolation_scope(op: OperationRef) -> Option { - let mut current = op.borrow().parent_op(); - while let Some(ancestor) = current { - if ancestor.borrow().implements::() { - return Some(ancestor); - } - current = ancestor.borrow().parent_op(); - } - None -} diff --git a/hir/src/ir.rs b/hir/src/ir.rs index 6fc32c497..56230c337 100644 --- a/hir/src/ir.rs +++ b/hir/src/ir.rs @@ -15,6 +15,7 @@ mod operands; mod operation; pub mod parse; pub mod print; +mod reachability; mod region; mod successor; pub(crate) mod symbols; @@ -64,6 +65,7 @@ pub use self::{ }, parse::{OpAsmParser, OpParser, ParseResult}, print::{AttrPrinter, OpPrinter, OpPrintingFlags}, + reachability::Reachability, region::{ InvocationBounds, LoopLikeOpInterface, Region, RegionBranchOpInterface, RegionBranchPoint, RegionBranchTerminatorOpInterface, RegionCursor, RegionCursorMut, RegionKind, diff --git a/hir/src/ir/reachability.rs b/hir/src/ir/reachability.rs new file mode 100644 index 000000000..601652cde --- /dev/null +++ b/hir/src/ir/reachability.rs @@ -0,0 +1,213 @@ +use crate::{ + BlockRef, Operation, OperationRef, Region, RegionBranchOpInterface, RegionKindInterface, + RegionRef, SmallVec, adt::SmallSet, cfg::Graph, +}; + +/// The answer to a control-flow reachability query between two operations. +/// +/// See [Operation::reachability]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Reachability { + /// Provably unreachable, i.e. no control flow path exists between `a` and `b` + Impossible, + /// Provably reachable, i.e. there is at least one control flow path guaranteed to reach + /// from `a` to `b` + Guaranteed, + /// Reachability is not proven, but there is at least one control flow path that reaches + /// from `a` to `b`; full reachability analysis is required to prove whether the path(s) are + /// truly executable + Maybe, + /// Cannot be determined without global reachability analysis, because the two ops are in + /// different functions + MaybeInterprocedurally, + /// Cannot be determined because control flow between the two ops is not well-defined (i.e. + /// both belong to a graph-like region, or their common ancestor region is graph-like) + Indeterminate, +} + +/// Queries +impl Operation { + /// Computes whether some control-flow path from `from` can reach `to`. + /// + /// This is a conservative, purely structural query over the CFG and region graph: it does + /// not consider whether paths are actually executable at runtime (that requires a proper + /// reachability analysis performed in concert with SCCP/DCE), so paths that exist but may + /// never execute are reported as [Reachability::Maybe]. The precise answers are reliable in + /// both directions: [Reachability::Impossible] means no control-flow path exists at all, + /// and [Reachability::Guaranteed] means a path exists that control cannot branch away from. + /// + /// Queries that relate positions in different functions, or in regions where operation + /// order does not define control flow, are answered with + /// [Reachability::MaybeInterprocedurally] and [Reachability::Indeterminate] respectively, + /// leaving the interpretation of such queries to the caller. + pub fn reachability(from: OperationRef, to: OperationRef) -> Reachability { + // One operation enclosing the other is an intra-procedural relationship no matter where + // the encloser resides: control entering the enclosing op can reach the nested + // position, and control leaving the nested position flows back through the enclosing + // op. This must precede the scope comparison, which would otherwise misclassify + // enclosure by an op residing in a graph-like region (e.g. a function op and an op in + // its body) as interprocedural. + if from.borrow().is_proper_ancestor_of(&to.borrow()) + || to.borrow().is_proper_ancestor_of(&from.borrow()) + { + return Reachability::Maybe; + } + + // Intra-procedural reasoning is bounded by the nearest ancestor residing in a + // graph-like region (in practice, the enclosing function): positions under different + // such ancestors can only be related interprocedurally. + if control_flow_scope(from) != control_flow_scope(to) { + return Reachability::MaybeInterprocedurally; + } + + // Without a common ancestor region no control-flow path can exist: any path from `from` + // to `to` would itself lie in a region containing both. + let Some(common_region) = Region::find_common_ancestor(&[from, to]) else { + return Reachability::Impossible; + }; + + // In a graph-like region operation order does not define control flow, so positional + // queries within it are meaningless. + if !region_has_ssa_dominance(common_region) { + return Reachability::Indeterminate; + } + + // Normalize both operations to the common region: an operation nested in a sub-region + // (e.g. structured control flow) is represented by its ancestor op in the common region. + let common_region_ref = common_region; + let common_region = common_region.borrow(); + let (Some(from_ancestor), Some(to_ancestor)) = + (common_region.find_ancestor_op(from), common_region.find_ancestor_op(to)) + else { + // Unreachable per find_common_ancestor's postcondition (the returned region + // contains every queried op); kept as a defensive fallback. + return Reachability::Maybe; + }; + + // Both operations normalize to the same ancestor op: either one op encloses the other + // (and control entering it can reach the nested position), or they sit in different + // sub-regions of that op, where transfer between the regions depends on the op's + // semantics (e.g. it can happen across loop iterations). + if from_ancestor == to_ancestor { + return Reachability::Maybe; + } + + let (Some(from_block), Some(to_block)) = + (from_ancestor.borrow().parent(), to_ancestor.borrow().parent()) + else { + return Reachability::Maybe; + }; + + // Within one block an earlier operation always flows into a later one; this is only a + // guarantee when neither position was normalized, since entering a sub-region of an + // ancestor op is generally conditional on that op's semantics. + if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { + return if from_ancestor == from && to_ancestor == to { + Reachability::Guaranteed + } else { + Reachability::Maybe + }; + } + + // A forward path may exist through block successors; earlier positions are only + // reachable through a cycle, either via block successors, or by re-entry of the common + // region itself. + if block_leads_to(from_block, to_block) { + return Reachability::Maybe; + } + if region_can_re_execute(common_region_ref) { + return Reachability::Maybe; + } + + Reachability::Impossible + } +} + +/// Returns the nearest proper ancestor of `op` that resides in a graph-like region (or has no +/// parent at all), i.e. the operation whose body bounds any intra-procedural control-flow +/// reasoning about `op`. In practice this is the enclosing function, whose parent module body +/// is a graph-like region. +fn control_flow_scope(op: OperationRef) -> Option { + let mut current = op.borrow().parent_op(); + while let Some(ancestor) = current { + let Some(parent_block) = ancestor.borrow().parent() else { + return Some(ancestor); + }; + if !parent_block.borrow().has_ssa_dominance() { + return Some(ancestor); + } + current = ancestor.borrow().parent_op(); + } + None +} + +/// Returns true if `region` requires SSA dominance, i.e. operation order within it defines +/// control flow. Regions of operations that do not declare a region kind default to SSA. +fn region_has_ssa_dominance(region: RegionRef) -> bool { + region + .parent() + .and_then(|op| { + op.borrow() + .as_trait::() + .map(|rki| rki.has_ssa_dominance()) + }) + .unwrap_or(true) +} + +/// Returns true if control leaving the end of `from` can reach the start of `to` by following +/// block successors. +/// +/// The walk is not reflexive: `from == to` returns true only when a cycle leads back into the +/// block, which is what [region_can_re_execute] relies on for cycle detection. +fn block_leads_to(from: BlockRef, to: BlockRef) -> bool { + let mut visited = SmallSet::::default(); + let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); + while let Some(block) = worklist.pop() { + if block == to { + return true; + } + if !visited.insert(block) { + continue; + } + worklist.extend(BlockRef::children(block)); + } + false +} + +/// Returns true if `region` can execute more than once within a single execution of the +/// operation bounding control-flow reasoning about it (see [control_flow_scope]). +/// +/// That is the case when an enclosing region is repetitive (e.g. the regions of an `scf.while`, +/// whose back edges are expressed in the region graph of the owning op rather than as block +/// successors), or when an enclosing op itself sits on a CFG cycle in its parent region. +fn region_can_re_execute(region: RegionRef) -> bool { + let mut current = Some(region); + while let Some(r) = current { + let Some(owner) = r.parent() else { + return false; + }; + let owner_op = owner.borrow(); + let Some(owner_block) = owner_op.parent() else { + // A top-level owner cannot be re-entered from anywhere. + return false; + }; + if !owner_block.borrow().has_ssa_dominance() { + // The owner resides in a graph-like region: control-flow reasoning stops here (in + // practice the owner is the enclosing function, and each execution of its body is + // a separate invocation). + return false; + } + if !owner_op.implements::() { + // Unknown region semantics: conservatively treat re-entry as possible. + return true; + } + if r.borrow().is_repetitive_region() { + return true; + } + if block_leads_to(owner_block, owner_block) { + return true; + } + current = owner_op.parent_region(); + } + false +} From 76c18ec7e65b1fbda2e774d0614577627d5e77eb Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 3 Aug 2026 08:55:13 +0300 Subject: [PATCH 16/19] fix(hir): tighten sibling, guarantee, and enclosure Reachability classifications Addresses the second review round on the reachability query. Sibling sub-regions of one region-branch op now consult the op's own region graph instead of a blanket Maybe: the arms of an if outside any loop are provably unreachable from each other, which lets spill pruning remove dead sibling-arm stores, while the before/after regions of a while remain mutually reachable, and a negative region-graph answer still falls back to whether the owner op itself can execute again (an enclosing repetitive region or a CFG cycle through its block), so arms of an if that a loop re-enters stay reachable. Guaranteed is now claimed only for adjacent positions in one block: an intervening operation may loop indefinitely, return from the function, or abort, so earlier placement alone proves nothing beyond Maybe. Enclosure classifies the chain of regions crossed between the two operations: crossing a graph-like region (e.g. a module enclosing a function) defines no control-flow order and is Indeterminate, while enclosure within a function remains Maybe. --- dialects/hir/src/transforms/spill/tests.rs | 38 ++++++- hir/src/ir/reachability.rs | 124 ++++++++++++++++++--- 2 files changed, 142 insertions(+), 20 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 037abb3fe..2cd76241e 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -755,7 +755,12 @@ fn reachability_in_branching_cfg() -> TestResult<()> { assert_eq!( reach(entry_first, entry_second), Reachability::Guaranteed, - "an earlier op always flows into a later op in the same block" + "an earlier op always flows into the adjacent next op" + ); + assert_eq!( + reach(op_at(entry, 0), entry_second), + Reachability::Maybe, + "an intervening op may diverge, so non-adjacent positions are only Maybe" ); assert_eq!( reach(entry_second, entry_first), @@ -863,8 +868,8 @@ fn reachability_across_nested_regions() -> TestResult<()> { ); assert_eq!( reach(then_op, else_op), - Reachability::Maybe, - "sibling regions of one op conservatively reach each other" + Reachability::Impossible, + "mutually exclusive arms of an if outside any loop cannot reach each other" ); assert_eq!( reach(if_op, then_op), @@ -929,6 +934,17 @@ fn reachability_through_region_re_entry() -> TestResult<()> { Reachability::Impossible, "an op after the loop must not reach into it" ); + let after_op = op_in_region(while_op, 1, 0); + assert_eq!( + reach(late_op, after_op), + Reachability::Maybe, + "the after region is reachable from the before region in the while region graph" + ); + assert_eq!( + reach(after_op, early_op), + Reachability::Maybe, + "the before region is reachable from the after region through the loop" + ); Ok(()) } @@ -978,6 +994,12 @@ fn reachability_through_cfg_cycle_re_entry() -> TestResult<()> { Reachability::Impossible, "an op after the loop must not reach into it" ); + let else_op = op_in_region(if_op, 1, 0); + assert_eq!( + reach(early_op, else_op), + Reachability::Maybe, + "sibling if arms stay reachable when an outer CFG cycle re-executes the if" + ); Ok(()) } @@ -1041,5 +1063,15 @@ fn reachability_across_functions_and_graph_regions() -> TestResult<()> { Reachability::Maybe, "enclosure is intra-procedural in both directions" ); + assert_eq!( + reach(module_op, first_fn), + Reachability::Indeterminate, + "enclosure crossing a graph-like region defines no control-flow order" + ); + assert_eq!( + reach(in_first, module_op), + Reachability::Indeterminate, + "enclosure crossing a graph-like region is indeterminate in both directions" + ); Ok(()) } diff --git a/hir/src/ir/reachability.rs b/hir/src/ir/reachability.rs index 601652cde..7123b3586 100644 --- a/hir/src/ir/reachability.rs +++ b/hir/src/ir/reachability.rs @@ -41,16 +41,17 @@ impl Operation { /// [Reachability::MaybeInterprocedurally] and [Reachability::Indeterminate] respectively, /// leaving the interpretation of such queries to the caller. pub fn reachability(from: OperationRef, to: OperationRef) -> Reachability { - // One operation enclosing the other is an intra-procedural relationship no matter where - // the encloser resides: control entering the enclosing op can reach the nested + // One operation enclosing the other relates the two positions through the chain of + // regions between them: control entering the enclosing op can reach the nested // position, and control leaving the nested position flows back through the enclosing - // op. This must precede the scope comparison, which would otherwise misclassify - // enclosure by an op residing in a graph-like region (e.g. a function op and an op in - // its body) as interprocedural. - if from.borrow().is_proper_ancestor_of(&to.borrow()) - || to.borrow().is_proper_ancestor_of(&from.borrow()) + // op — unless the chain crosses a graph-like region (e.g. a module enclosing a + // function), where no control-flow order is defined. This must precede the scope + // comparison, which would otherwise misclassify enclosure by an op residing in a + // graph-like region as interprocedural. + if let Some(result) = + enclosure_reachability(from, to).or_else(|| enclosure_reachability(to, from)) { - return Reachability::Maybe; + return result; } // Intra-procedural reasoning is bounded by the nearest ancestor residing in a @@ -84,12 +85,12 @@ impl Operation { return Reachability::Maybe; }; - // Both operations normalize to the same ancestor op: either one op encloses the other - // (and control entering it can reach the nested position), or they sit in different - // sub-regions of that op, where transfer between the regions depends on the op's - // semantics (e.g. it can happen across loop iterations). + // Both operations normalize to the same ancestor op, i.e. they sit in different + // sub-regions of it (enclosure was handled above). Whether control can transfer from + // one sub-region to a sibling is decided by the op's own region graph, or by the op + // executing more than once. if from_ancestor == to_ancestor { - return Reachability::Maybe; + return sibling_region_reachability(from_ancestor, from, to); } let (Some(from_block), Some(to_block)) = @@ -98,11 +99,12 @@ impl Operation { return Reachability::Maybe; }; - // Within one block an earlier operation always flows into a later one; this is only a - // guarantee when neither position was normalized, since entering a sub-region of an - // ancestor op is generally conditional on that op's semantics. + // Within one block an earlier operation may flow into a later one, but control is only + // guaranteed to arrive when nothing lies between them: an intervening operation may + // loop indefinitely, return from the function, or abort. Positions that were + // normalized are likewise conditional on their ancestor op's semantics. if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { - return if from_ancestor == from && to_ancestor == to { + return if from_ancestor == from && to_ancestor == to && from.next() == Some(to) { Reachability::Guaranteed } else { Reachability::Maybe @@ -141,6 +143,94 @@ fn control_flow_scope(op: OperationRef) -> Option { None } +/// If `ancestor` properly encloses `descendant`, classifies the enclosure: +/// [Reachability::Maybe] when every region crossed between them defines control flow, or +/// [Reachability::Indeterminate] when the chain crosses a graph-like region (e.g. a module +/// enclosing a function). Returns `None` when `ancestor` does not enclose `descendant`. +fn enclosure_reachability( + ancestor: OperationRef, + descendant: OperationRef, +) -> Option { + if !ancestor.borrow().is_proper_ancestor_of(&descendant.borrow()) { + return None; + } + // Walk the regions from `descendant` up to (and including) the region owned by `ancestor`. + let mut region = descendant.borrow().parent_region(); + while let Some(r) = region { + if !region_has_ssa_dominance(r) { + return Some(Reachability::Indeterminate); + } + let Some(owner) = r.parent() else { + break; + }; + if owner == ancestor { + return Some(Reachability::Maybe); + } + region = owner.borrow().parent_region(); + } + // Unreachable given the ancestry check above; defensively report plain enclosure. + Some(Reachability::Maybe) +} + +/// Classifies reachability between two positions in different sub-regions of one `owner` op. +/// +/// The sibling region is reachable when the region graph of `owner` can transfer control from +/// the region holding `from` to the region holding `to` within one execution of `owner` (e.g. +/// from the `before` region of a while to its `after` region), or when `owner` itself can +/// execute more than once (e.g. the arms of an if that an enclosing loop re-enters); otherwise +/// it is provably unreachable (e.g. the arms of an if outside any loop). +fn sibling_region_reachability( + owner: OperationRef, + from: OperationRef, + to: OperationRef, +) -> Reachability { + if !owner.borrow().implements::() { + // Unknown region semantics: conservatively treat transfer as possible. + return Reachability::Maybe; + } + let (Some(from_region), Some(to_region)) = + (child_region_containing(owner, from), child_region_containing(owner, to)) + else { + // Unreachable given the normalization above; defensively treat as reachable. + return Reachability::Maybe; + }; + if to_region.borrow().is_reachable_from(&from_region.borrow()) { + return Reachability::Maybe; + } + if op_can_re_execute(owner) { + return Reachability::Maybe; + } + Reachability::Impossible +} + +/// Returns the direct child region of `owner` that contains `op`. +fn child_region_containing(owner: OperationRef, op: OperationRef) -> Option { + let mut region = op.borrow().parent_region(); + while let Some(r) = region { + let parent = r.parent()?; + if parent == owner { + return Some(r); + } + region = parent.borrow().parent_region(); + } + None +} + +/// Returns true if `op` can execute more than once: its block lies on a CFG cycle, or the +/// region containing it can re-execute. +fn op_can_re_execute(op: OperationRef) -> bool { + let (block, region) = { + let op = op.borrow(); + (op.parent(), op.parent_region()) + }; + if let Some(block) = block + && block_leads_to(block, block) + { + return true; + } + region.is_some_and(region_can_re_execute) +} + /// Returns true if `region` requires SSA dominance, i.e. operation order within it defines /// control flow. Regions of operations that do not declare a region kind default to SSA. fn region_has_ssa_dominance(region: RegionRef) -> bool { From f8571dfb5e59875078e9b55750313a3086d21160 Mon Sep 17 00:00:00 2001 From: Denys Zadorozhnyi Date: Mon, 3 Aug 2026 08:59:36 +0300 Subject: [PATCH 17/19] perf(codegen): cache block reachability and index live reloads by value The pruning loop scanned every reload for every spill and ran a fresh block-reachability walk per matching live pair, O(spills x reloads x CFG) in the worst case. Index the live reloads by spilled value once, snapshotting reload liveness before any erasures (which errs toward keeping a spill whose reload only dies as part of the erasure cascade), so each spill only considers reloads it can possibly cover. Add ReachabilityCache to midenc-hir: a lazily-populated forward-closure cache shared across Operation::reachability_cached queries, so the first query from a block computes its reachable set once and every later query from that block is a set lookup. Pruning holds one cache for the whole rewrite, which stays valid because pruning erases operations, never blocks. --- hir-transform/src/spill.rs | 62 ++++++++++++++++---------- hir/src/ir.rs | 2 +- hir/src/ir/reachability.rs | 90 +++++++++++++++++++++++++------------- 3 files changed, 98 insertions(+), 56 deletions(-) diff --git a/hir-transform/src/spill.rs b/hir-transform/src/spill.rs index 41547d169..9453291da 100644 --- a/hir-transform/src/spill.rs +++ b/hir-transform/src/spill.rs @@ -2,9 +2,9 @@ use alloc::{collections::VecDeque, format, rc::Rc}; use midenc_hir::{ BlockRef, Builder, Context, FxHashMap, OpBuilder, OpOperand, Operation, OperationRef, - ProgramPoint, Reachability, Region, RegionBranchOpInterface, RegionBranchPoint, RegionRef, - Report, Rewriter, SmallVec, SourceSpan, Spanned, StorableEntity, TraceTarget, Usable, - ValueRange, ValueRef, + ProgramPoint, Reachability, ReachabilityCache, Region, RegionBranchOpInterface, + RegionBranchPoint, RegionRef, Report, Rewriter, SmallVec, SourceSpan, Spanned, StorableEntity, + TraceTarget, Usable, ValueRange, ValueRef, adt::{SmallDenseMap, SmallSet}, cfg::Graph, dominance::{DomTreeNode, DominanceFrontier, DominanceInfo}, @@ -871,30 +871,44 @@ fn rewrite_spill_pseudo_instructions( let mut builder = RewriterImpl::::new(context) .with_listener(TracingRewriterListener); + + // Index the live reloads by their spilled value once, so each spill only considers the + // reloads it can possibly cover. Spills and reloads are paired through the analysis's value + // bookkeeping rather than the spill op's current operand, which SSA reconstruction may + // rewrite (only reload operands are exempt). Liveness is snapshotted before any spills are + // erased, which errs toward keeping a spill whose reload only dies as part of the erasure + // cascade below. + let mut live_reloads = SmallDenseMap::, 8>::default(); + for rinfo in analysis.reloads() { + let Some(reload_op) = rinfo.inst else { + continue; + }; + let reload_used = { + let rop = reload_op.borrow(); + let rl = rop + .as_trait::() + .expect("expected materialized reload op to implement ReloadLike"); + rl.reloaded().borrow().is_used() + }; + if reload_used { + live_reloads.entry(rinfo.value).or_default().push(reload_op); + } + } + + // One reachability cache is shared across all pairings: pruning only erases operations, + // never blocks, so the cached block-reachability stays valid throughout. + let mut reachability = ReachabilityCache::default(); for spill in analysis.spills() { let operation = spill.inst.expect("expected spill to have been materialized"); - // Only keep spills that can reach a live reload of their value. Spills and reloads are - // paired through the analysis's value bookkeeping rather than the spill op's current - // operand, which SSA reconstruction may rewrite (only reload operands are exempt). + // Only keep spills that can reach a live reload of their value let mut is_used = false; - for rinfo in analysis.reloads() { - if rinfo.value != spill.value { - continue; - } - let Some(reload_op) = rinfo.inst else { - continue; - }; - let reload_used = { - let rop = reload_op.borrow(); - let rl = rop - .as_trait::() - .expect("expected materialized reload op to implement ReloadLike"); - rl.reloaded().borrow().is_used() - }; - if !reload_used { - continue; - } - match Operation::reachability(operation, reload_op) { + for reload_op in live_reloads + .get(&spill.value) + .map(|reloads| reloads.as_slice()) + .unwrap_or_default() + { + let reload_op = *reload_op; + match Operation::reachability_cached(operation, reload_op, &mut reachability) { Reachability::Guaranteed | Reachability::Maybe => { is_used = true; break; diff --git a/hir/src/ir.rs b/hir/src/ir.rs index 56230c337..31ad1931f 100644 --- a/hir/src/ir.rs +++ b/hir/src/ir.rs @@ -65,7 +65,7 @@ pub use self::{ }, parse::{OpAsmParser, OpParser, ParseResult}, print::{AttrPrinter, OpPrinter, OpPrintingFlags}, - reachability::Reachability, + reachability::{Reachability, ReachabilityCache}, region::{ InvocationBounds, LoopLikeOpInterface, Region, RegionBranchOpInterface, RegionBranchPoint, RegionBranchTerminatorOpInterface, RegionCursor, RegionCursorMut, RegionKind, diff --git a/hir/src/ir/reachability.rs b/hir/src/ir/reachability.rs index 7123b3586..769c23c9b 100644 --- a/hir/src/ir/reachability.rs +++ b/hir/src/ir/reachability.rs @@ -1,6 +1,6 @@ use crate::{ - BlockRef, Operation, OperationRef, Region, RegionBranchOpInterface, RegionKindInterface, - RegionRef, SmallVec, adt::SmallSet, cfg::Graph, + BlockRef, FxHashMap, FxHashSet, Operation, OperationRef, Region, RegionBranchOpInterface, + RegionKindInterface, RegionRef, SmallVec, cfg::Graph, }; /// The answer to a control-flow reachability query between two operations. @@ -25,6 +25,41 @@ pub enum Reachability { Indeterminate, } +/// A lazily-populated cache of forward block reachability, for callers issuing many +/// [Operation::reachability_cached] queries over one body of IR. +/// +/// The first query from a given block computes and stores that block's full forward closure; +/// subsequent queries from the same block are set lookups. The cache assumes the block +/// structure of the IR does not change between queries: discard it after splitting, erasing, +/// or rewiring blocks. +#[derive(Default)] +pub struct ReachabilityCache { + forward: FxHashMap>, +} + +impl ReachabilityCache { + /// Returns true if control leaving the end of `from` can reach the start of `to` by + /// following block successors. + /// + /// The walk is not reflexive: `from == to` returns true only when a cycle leads back into + /// the block, which is what [region_can_re_execute] relies on for cycle detection. + fn leads_to(&mut self, from: BlockRef, to: BlockRef) -> bool { + self.forward + .entry(from) + .or_insert_with(|| { + let mut reachable = FxHashSet::default(); + let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); + while let Some(block) = worklist.pop() { + if reachable.insert(block) { + worklist.extend(BlockRef::children(block)); + } + } + reachable + }) + .contains(&to) + } +} + /// Queries impl Operation { /// Computes whether some control-flow path from `from` can reach `to`. @@ -41,6 +76,18 @@ impl Operation { /// [Reachability::MaybeInterprocedurally] and [Reachability::Indeterminate] respectively, /// leaving the interpretation of such queries to the caller. pub fn reachability(from: OperationRef, to: OperationRef) -> Reachability { + Self::reachability_cached(from, to, &mut ReachabilityCache::default()) + } + + /// Like [Operation::reachability], reusing `cache` across queries. + /// + /// Use this when issuing many queries over one body of IR (e.g. pairing spills with + /// reloads), so that block-reachability walks are shared between them. + pub fn reachability_cached( + from: OperationRef, + to: OperationRef, + cache: &mut ReachabilityCache, + ) -> Reachability { // One operation enclosing the other relates the two positions through the chain of // regions between them: control entering the enclosing op can reach the nested // position, and control leaving the nested position flows back through the enclosing @@ -90,7 +137,7 @@ impl Operation { // one sub-region to a sibling is decided by the op's own region graph, or by the op // executing more than once. if from_ancestor == to_ancestor { - return sibling_region_reachability(from_ancestor, from, to); + return sibling_region_reachability(from_ancestor, from, to, cache); } let (Some(from_block), Some(to_block)) = @@ -114,10 +161,10 @@ impl Operation { // A forward path may exist through block successors; earlier positions are only // reachable through a cycle, either via block successors, or by re-entry of the common // region itself. - if block_leads_to(from_block, to_block) { + if cache.leads_to(from_block, to_block) { return Reachability::Maybe; } - if region_can_re_execute(common_region_ref) { + if region_can_re_execute(common_region_ref, cache) { return Reachability::Maybe; } @@ -183,6 +230,7 @@ fn sibling_region_reachability( owner: OperationRef, from: OperationRef, to: OperationRef, + cache: &mut ReachabilityCache, ) -> Reachability { if !owner.borrow().implements::() { // Unknown region semantics: conservatively treat transfer as possible. @@ -197,7 +245,7 @@ fn sibling_region_reachability( if to_region.borrow().is_reachable_from(&from_region.borrow()) { return Reachability::Maybe; } - if op_can_re_execute(owner) { + if op_can_re_execute(owner, cache) { return Reachability::Maybe; } Reachability::Impossible @@ -218,17 +266,17 @@ fn child_region_containing(owner: OperationRef, op: OperationRef) -> Option bool { +fn op_can_re_execute(op: OperationRef, cache: &mut ReachabilityCache) -> bool { let (block, region) = { let op = op.borrow(); (op.parent(), op.parent_region()) }; if let Some(block) = block - && block_leads_to(block, block) + && cache.leads_to(block, block) { return true; } - region.is_some_and(region_can_re_execute) + region.is_some_and(|region| region_can_re_execute(region, cache)) } /// Returns true if `region` requires SSA dominance, i.e. operation order within it defines @@ -244,33 +292,13 @@ fn region_has_ssa_dominance(region: RegionRef) -> bool { .unwrap_or(true) } -/// Returns true if control leaving the end of `from` can reach the start of `to` by following -/// block successors. -/// -/// The walk is not reflexive: `from == to` returns true only when a cycle leads back into the -/// block, which is what [region_can_re_execute] relies on for cycle detection. -fn block_leads_to(from: BlockRef, to: BlockRef) -> bool { - let mut visited = SmallSet::::default(); - let mut worklist = SmallVec::<[BlockRef; 8]>::from_iter(BlockRef::children(from)); - while let Some(block) = worklist.pop() { - if block == to { - return true; - } - if !visited.insert(block) { - continue; - } - worklist.extend(BlockRef::children(block)); - } - false -} - /// Returns true if `region` can execute more than once within a single execution of the /// operation bounding control-flow reasoning about it (see [control_flow_scope]). /// /// That is the case when an enclosing region is repetitive (e.g. the regions of an `scf.while`, /// whose back edges are expressed in the region graph of the owning op rather than as block /// successors), or when an enclosing op itself sits on a CFG cycle in its parent region. -fn region_can_re_execute(region: RegionRef) -> bool { +fn region_can_re_execute(region: RegionRef, cache: &mut ReachabilityCache) -> bool { let mut current = Some(region); while let Some(r) = current { let Some(owner) = r.parent() else { @@ -294,7 +322,7 @@ fn region_can_re_execute(region: RegionRef) -> bool { if r.borrow().is_repetitive_region() { return true; } - if block_leads_to(owner_block, owner_block) { + if cache.leads_to(owner_block, owner_block) { return true; } current = owner_op.parent_region(); From 95b5d575c8a36733af03b48482578bec6aa9a734 Mon Sep 17 00:00:00 2001 From: Paul Schoenfelder Date: Fri, 7 Aug 2026 21:23:55 -0400 Subject: [PATCH 18/19] hir: make same-block reachability conservative Classify forward same-block operation pairs as maybe reachable because adjacency and ordering do not prove that the source or intervening operations fall through. --- dialects/hir/src/transforms/spill/tests.rs | 13 +++++++++---- hir/src/ir/reachability.rs | 14 +++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index 2cd76241e..f94c38d12 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -754,8 +754,8 @@ fn reachability_in_branching_cfg() -> TestResult<()> { ); assert_eq!( reach(entry_first, entry_second), - Reachability::Guaranteed, - "an earlier op always flows into the adjacent next op" + Reachability::Maybe, + "adjacency alone does not prove that the source operation falls through" ); assert_eq!( reach(op_at(entry, 0), entry_second), @@ -926,8 +926,13 @@ fn reachability_through_region_re_entry() -> TestResult<()> { ); assert_eq!( reach(early_op, late_op), - Reachability::Guaranteed, - "an earlier op always flows into a later op in the same block" + Reachability::Maybe, + "same-block order does not prove that every intervening operation falls through" + ); + assert_eq!( + reach(while_op, ret_op), + Reachability::Maybe, + "an adjacent while may never exit, so reaching its successor is not guaranteed" ); assert_eq!( reach(ret_op, late_op), diff --git a/hir/src/ir/reachability.rs b/hir/src/ir/reachability.rs index 769c23c9b..60028c673 100644 --- a/hir/src/ir/reachability.rs +++ b/hir/src/ir/reachability.rs @@ -146,16 +146,12 @@ impl Operation { return Reachability::Maybe; }; - // Within one block an earlier operation may flow into a later one, but control is only - // guaranteed to arrive when nothing lies between them: an intervening operation may - // loop indefinitely, return from the function, or abort. Positions that were - // normalized are likewise conditional on their ancestor op's semantics. + // Within one block an earlier operation may flow into a later one, but order alone does + // not prove total fallthrough: the source or an intervening operation may loop + // indefinitely, return from the function, or abort. Positions that were normalized are + // likewise conditional on their ancestor op's semantics. if from_block == to_block && from_ancestor.borrow().is_before_in_block(&to_ancestor) { - return if from_ancestor == from && to_ancestor == to && from.next() == Some(to) { - Reachability::Guaranteed - } else { - Reachability::Maybe - }; + return Reachability::Maybe; } // A forward path may exist through block successors; earlier positions are only From 70276710d78a0ac0c89bb7814d646d5039e3406a Mon Sep 17 00:00:00 2001 From: Paul Schoenfelder Date: Fri, 7 Aug 2026 21:35:12 -0400 Subject: [PATCH 19/19] hir: make enclosure reachability directional Prove ancestor entry and descendant exit paths through callable CFGs and concrete region-branch terminators. Preserve conservative maybe results for unmodeled owners, terminators, and nested control flow. --- dialects/hir/src/transforms/spill/tests.rs | 71 +++ hir/src/ir/reachability.rs | 559 ++++++++++++++++++++- 2 files changed, 613 insertions(+), 17 deletions(-) diff --git a/dialects/hir/src/transforms/spill/tests.rs b/dialects/hir/src/transforms/spill/tests.rs index f94c38d12..d9109721f 100644 --- a/dialects/hir/src/transforms/spill/tests.rs +++ b/dialects/hir/src/transforms/spill/tests.rs @@ -950,6 +950,11 @@ fn reachability_through_region_re_entry() -> TestResult<()> { Reachability::Maybe, "the before region is reachable from the after region through the loop" ); + assert_eq!( + reach(after_op, while_op), + Reachability::Maybe, + "leaving the after region can traverse after -> before -> parent" + ); Ok(()) } @@ -1008,6 +1013,72 @@ fn reachability_through_cfg_cycle_re_entry() -> TestResult<()> { Ok(()) } +/// Enclosure reachability is directional and must respect the function body's CFG. +/// +/// A reachable sink can be entered from the function but cannot return to it. Conversely, a +/// disconnected block cannot be entered from the function, although an operation already in that +/// block can follow its own return. +#[test] +fn reachability_across_directional_function_enclosure() -> TestResult<()> { + let source = r#"builtin.function public extern("C") @reachability_across_directional_function_enclosure(%a: u32, %cond: i1) -> u32 { + cf.cond_br %cond ^sink, ^exit : (i1); +^sink: + %sink_value = arith.add %a, %a <{ overflow = #builtin.overflow }>; + cf.br ^sink; +^exit: + %exit_value = arith.add %a, %a <{ overflow = #builtin.overflow }>; + builtin.ret %exit_value : (u32); +^dead: + %dead_value = arith.add %a, %a <{ overflow = #builtin.overflow }>; + builtin.ret %dead_value : (u32); +};"#; + + let context = Rc::new(Context::default()); + let (function, _) = parse_function_fixpoint( + &context, + "reachability_across_directional_function_enclosure.hir", + source, + )?; + + let function_op = function.as_operation_ref(); + let sink_op = op_at(block_at(function, 1), 0); + let exit_op = op_at(block_at(function, 2), 0); + let dead_op = op_at(block_at(function, 3), 0); + + assert_eq!( + reach(function_op, sink_op), + Reachability::Maybe, + "the function entry can flow into the reachable sink" + ); + assert_eq!( + reach(sink_op, function_op), + Reachability::Impossible, + "the sink SCC has no path back to the enclosing function" + ); + assert_eq!( + reach(function_op, dead_op), + Reachability::Impossible, + "the function entry cannot reach a disconnected block" + ); + assert_eq!( + reach(dead_op, function_op), + Reachability::Maybe, + "a position in the disconnected block can still reach its own return" + ); + assert_eq!( + reach(function_op, exit_op), + Reachability::Maybe, + "the ordinary exit block remains reachable from function entry" + ); + assert_eq!( + reach(exit_op, function_op), + Reachability::Maybe, + "the ordinary exit block reaches the enclosing function return" + ); + + Ok(()) +} + /// Queries that leave a single function's control flow have no positional answer. /// /// Operations in two different functions can only be related interprocedurally, and for the diff --git a/hir/src/ir/reachability.rs b/hir/src/ir/reachability.rs index 60028c673..dbd4a2c06 100644 --- a/hir/src/ir/reachability.rs +++ b/hir/src/ir/reachability.rs @@ -1,6 +1,9 @@ use crate::{ - BlockRef, FxHashMap, FxHashSet, Operation, OperationRef, Region, RegionBranchOpInterface, - RegionKindInterface, RegionRef, SmallVec, cfg::Graph, + AttributeRef, BlockRef, CallableOpInterface, FxHashMap, FxHashSet, Operation, OperationRef, + Region, RegionBranchOpInterface, RegionBranchPoint, RegionBranchTerminatorOpInterface, + RegionKindInterface, RegionRef, SmallVec, + cfg::Graph, + traits::{NoTerminator, ReturnLike}, }; /// The answer to a control-flow reachability query between two operations. @@ -95,11 +98,13 @@ impl Operation { // function), where no control-flow order is defined. This must precede the scope // comparison, which would otherwise misclassify enclosure by an op residing in a // graph-like region as interprocedural. - if let Some(result) = - enclosure_reachability(from, to).or_else(|| enclosure_reachability(to, from)) + if let Some(result) = enclosure_reachability(from, to, EnclosureDirection::Entering, cache) { return result; } + if let Some(result) = enclosure_reachability(to, from, EnclosureDirection::Exiting, cache) { + return result; + } // Intra-procedural reasoning is bounded by the nearest ancestor residing in a // graph-like region (in practice, the enclosing function): positions under different @@ -186,35 +191,369 @@ fn control_flow_scope(op: OperationRef) -> Option { None } -/// If `ancestor` properly encloses `descendant`, classifies the enclosure: -/// [Reachability::Maybe] when every region crossed between them defines control flow, or -/// [Reachability::Indeterminate] when the chain crosses a graph-like region (e.g. a module -/// enclosing a function). Returns `None` when `ancestor` does not enclose `descendant`. +#[derive(Clone, Copy)] +enum EnclosureDirection { + Entering, + Exiting, +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum BoundaryReachability { + CanCross, + CannotCross, + Unknown, +} + +struct EnclosureStep { + owner: OperationRef, + region: RegionRef, + position: OperationRef, +} + +struct RegionExitAnalysis { + successors: SmallVec<[RegionBranchPoint; 4]>, + has_unknown_exit: bool, +} + +enum RegionGraphStart { + Parent, + Child { region: RegionRef, block: BlockRef }, +} + +/// If `ancestor` properly encloses `descendant`, classify whether control can enter or leave the +/// descendant position through every SSA region separating the two operations. +/// +/// A graph-like region makes the positional query indeterminate. For SSA regions, `Impossible` +/// is returned only when a fully modeled CFG or region boundary proves that no path exists; +/// incomplete terminator or owner semantics remain conservatively `Maybe`. fn enclosure_reachability( ancestor: OperationRef, descendant: OperationRef, + direction: EnclosureDirection, + cache: &mut ReachabilityCache, ) -> Option { if !ancestor.borrow().is_proper_ancestor_of(&descendant.borrow()) { return None; } - // Walk the regions from `descendant` up to (and including) the region owned by `ancestor`. - let mut region = descendant.borrow().parent_region(); - while let Some(r) = region { - if !region_has_ssa_dominance(r) { + + // Collect the direct owner/child-region boundaries from the descendant out to the ancestor. + // `position` is always directly contained in `region`, which gives each boundary analysis a + // concrete starting or target block. + let mut steps = SmallVec::<[EnclosureStep; 4]>::new(); + let mut position = descendant; + loop { + let Some(region) = position.borrow().parent_region() else { + // Defensive fallback for malformed ancestry. + return Some(Reachability::Maybe); + }; + if !region_has_ssa_dominance(region) { return Some(Reachability::Indeterminate); } - let Some(owner) = r.parent() else { - break; + let Some(owner) = region.parent() else { + return Some(Reachability::Maybe); }; + steps.push(EnclosureStep { + owner, + region, + position, + }); if owner == ancestor { - return Some(Reachability::Maybe); + break; + } + position = owner; + } + + match direction { + EnclosureDirection::Entering => { + for step in steps.iter().rev() { + match region_entry_reachability(step, cache) { + BoundaryReachability::CanCross => {} + BoundaryReachability::CannotCross => { + return Some(Reachability::Impossible); + } + BoundaryReachability::Unknown => return Some(Reachability::Maybe), + } + } + } + EnclosureDirection::Exiting => { + for step in &steps { + match region_exit_reachability(step, cache) { + BoundaryReachability::CanCross => {} + BoundaryReachability::CannotCross => { + return Some(Reachability::Impossible); + } + BoundaryReachability::Unknown => return Some(Reachability::Maybe), + } + } } - region = owner.borrow().parent_region(); } - // Unreachable given the ancestry check above; defensively report plain enclosure. + Some(Reachability::Maybe) } +fn region_entry_reachability( + step: &EnclosureStep, + cache: &mut ReachabilityCache, +) -> BoundaryReachability { + let owner_is_region_branch = step.owner.borrow().implements::(); + let owner_is_callable = is_callable_body(step.owner, step.region); + if !owner_is_region_branch && !owner_is_callable { + return BoundaryReachability::Unknown; + } + + let Some(target_block) = step.position.borrow().parent() else { + return BoundaryReachability::Unknown; + }; + let Some(entry_block) = step.region.borrow().entry_block_ref() else { + return BoundaryReachability::CannotCross; + }; + if !block_reaches(entry_block, target_block, cache) { + return BoundaryReachability::CannotCross; + } + + if owner_is_region_branch { + return region_branch_reachability( + step.owner, + RegionGraphStart::Parent, + RegionBranchPoint::Child(step.region), + cache, + ); + } + + if owner_is_callable { + BoundaryReachability::CanCross + } else { + BoundaryReachability::Unknown + } +} + +fn region_exit_reachability( + step: &EnclosureStep, + cache: &mut ReachabilityCache, +) -> BoundaryReachability { + let Some(start_block) = step.position.borrow().parent() else { + return BoundaryReachability::Unknown; + }; + + if step.owner.borrow().implements::() { + return region_branch_reachability( + step.owner, + RegionGraphStart::Child { + region: step.region, + block: start_block, + }, + RegionBranchPoint::Parent, + cache, + ); + } + + if is_callable_body(step.owner, step.region) { + return callable_region_exit_reachability(step.owner, step.region, start_block, cache); + } + + BoundaryReachability::Unknown +} + +fn region_branch_reachability( + owner: OperationRef, + start: RegionGraphStart, + target: RegionBranchPoint, + cache: &mut ReachabilityCache, +) -> BoundaryReachability { + let mut worklist = SmallVec::<[(RegionRef, Option); 8]>::new(); + + match start { + RegionGraphStart::Parent => { + let successors = { + let owner = owner.borrow(); + let operands = unknown_operands(owner.num_operands()); + let branch = owner + .as_trait::() + .expect("expected a region branch operation"); + branch + .get_entry_successor_regions(&operands) + .map(RegionBranchPoint::from) + .collect::>() + }; + for successor in successors { + if successor == target { + return BoundaryReachability::CanCross; + } + if let RegionBranchPoint::Child(region) = successor { + worklist.push((region, None)); + } + } + } + RegionGraphStart::Child { region, block } => { + worklist.push((region, Some(block))); + } + } + + // A child reached from another child starts at its entry. The starting exit child instead + // starts at the descendant's block; track both states independently so a later region-graph + // cycle may legitimately re-enter that same child through its entry block. + let mut visited = SmallVec::<[(RegionRef, Option); 8]>::new(); + let mut has_unknown_path = false; + + while let Some((region, start_block)) = worklist.pop() { + if visited.contains(&(region, start_block)) { + continue; + } + visited.push((region, start_block)); + + let start_block = match start_block.or_else(|| region.borrow().entry_block_ref()) { + Some(block) => block, + None => { + has_unknown_path = true; + continue; + } + }; + let exits = reachable_region_exits(owner, region, start_block, cache); + has_unknown_path |= exits.has_unknown_exit; + + for successor in exits.successors { + if successor == target { + return BoundaryReachability::CanCross; + } + if let RegionBranchPoint::Child(region) = successor { + worklist.push((region, None)); + } + // Parent reached after a child is terminal for this execution of the region owner; + // it must not be expanded as though the operation were being entered again. + } + } + + if has_unknown_path { + BoundaryReachability::Unknown + } else { + BoundaryReachability::CannotCross + } +} + +fn reachable_region_exits( + owner: OperationRef, + region: RegionRef, + start_block: BlockRef, + cache: &mut ReachabilityCache, +) -> RegionExitAnalysis { + let mut analysis = RegionExitAnalysis { + successors: SmallVec::new(), + has_unknown_exit: false, + }; + + for block in region.borrow().body().iter() { + let block = block.as_block_ref(); + if !block_reaches(start_block, block, cache) { + continue; + } + let terminator = block.borrow().terminator(); + let Some(terminator) = terminator else { + if owner.borrow().implements::() { + let successors = { + let owner = owner.borrow(); + let branch = owner + .as_trait::() + .expect("expected a region branch operation"); + branch + .get_successor_regions(RegionBranchPoint::Child(region)) + .map(RegionBranchPoint::from) + .collect::>() + }; + append_unique_successors(&mut analysis.successors, successors); + } else { + analysis.has_unknown_exit = true; + } + continue; + }; + + let terminator = terminator.borrow(); + if let Some(region_terminator) = + terminator.as_trait::() + { + let operands = unknown_operands(terminator.num_operands()); + let successors = region_terminator + .get_successor_regions(&operands) + .into_iter() + .map(|successor| successor.successor()); + append_unique_successors(&mut analysis.successors, successors); + } else if terminator.implements::() { + // Plain returns are only valid as direct terminators of a callable body. A + // ReturnLike under a RegionBranch owner is malformed or otherwise unmodeled. + analysis.has_unknown_exit = true; + } else if terminator.num_successors() == 0 { + // A terminator with no CFG or region successors may abort, throw, or transfer control + // by semantics unavailable here. It cannot justify a false `Impossible`. + analysis.has_unknown_exit = true; + } + } + + analysis +} + +fn callable_region_exit_reachability( + owner: OperationRef, + region: RegionRef, + start_block: BlockRef, + cache: &mut ReachabilityCache, +) -> BoundaryReachability { + let mut has_unknown_exit = false; + + for block in region.borrow().body().iter() { + let block = block.as_block_ref(); + if !block_reaches(start_block, block, cache) { + continue; + } + let Some(terminator) = block.borrow().terminator() else { + if owner.borrow().implements::() { + return BoundaryReachability::CanCross; + } + has_unknown_exit = true; + continue; + }; + let terminator = terminator.borrow(); + if terminator.implements::() { + // Such a terminator only has a defined destination under a RegionBranch owner. + has_unknown_exit = true; + } else if terminator.implements::() { + return BoundaryReachability::CanCross; + } else if terminator.num_successors() == 0 { + has_unknown_exit = true; + } + } + + if has_unknown_exit { + BoundaryReachability::Unknown + } else { + BoundaryReachability::CannotCross + } +} + +fn is_callable_body(owner: OperationRef, region: RegionRef) -> bool { + owner + .borrow() + .as_trait::() + .is_some_and(|callable| callable.get_callable_region() == Some(region)) +} + +fn block_reaches(from: BlockRef, to: BlockRef, cache: &mut ReachabilityCache) -> bool { + from == to || cache.leads_to(from, to) +} + +fn unknown_operands(count: usize) -> SmallVec<[Option; 4]> { + core::iter::repeat_n(None, count).collect() +} + +fn append_unique_successors( + successors: &mut SmallVec<[RegionBranchPoint; 4]>, + additional: impl IntoIterator, +) { + for successor in additional { + if !successors.contains(&successor) { + successors.push(successor); + } + } +} + /// Classifies reachability between two positions in different sub-regions of one `owner` op. /// /// The sibling region is reachable when the region graph of `owner` can transfer control from @@ -325,3 +664,189 @@ fn region_can_re_execute(region: RegionRef, cache: &mut ReachabilityCache) -> bo } false } + +#[cfg(test)] +mod tests { + use alloc::vec::Vec; + + use super::*; + use crate::{ + Builder, BuilderExt, Op, RegionSuccessorInfo, RegionSuccessorIter, SourceSpan, + SuccessorOperandRange, SuccessorOperandRangeMut, ValueRef, + derive::operation, + dialects::test::TestDialect, + testing::Test, + traits::{AnyType, BranchOpInterface, Terminator}, + }; + + #[operation(dialect = TestDialect, implements(RegionBranchOpInterface))] + pub struct TestRegionBranch { + #[region] + first: Region, + #[region] + second: Region, + } + + impl RegionBranchOpInterface for TestRegionBranch { + fn get_successor_regions(&self, point: RegionBranchPoint) -> RegionSuccessorIter<'_> { + let first = self.first().as_region_ref(); + let second = self.second().as_region_ref(); + let successors = match point { + RegionBranchPoint::Parent => { + SmallVec::from_buf([RegionSuccessorInfo::Entering(first)]) + } + RegionBranchPoint::Child(region) if region == first => { + SmallVec::from_buf([RegionSuccessorInfo::Entering(second)]) + } + RegionBranchPoint::Child(_) => { + SmallVec::from_buf([RegionSuccessorInfo::Returning(SmallVec::new())]) + } + }; + RegionSuccessorIter::new(self.as_operation(), successors) + } + } + + #[operation( + dialect = TestDialect, + traits(Terminator), + implements(BranchOpInterface) + )] + pub struct TestBranch { + #[successor] + target: Successor, + } + + impl BranchOpInterface for TestBranch {} + + #[operation( + dialect = TestDialect, + traits(Terminator), + implements(RegionBranchTerminatorOpInterface) + )] + pub struct TestRegionYield { + #[operands] + yielded: AnyType, + } + + impl RegionBranchTerminatorOpInterface for TestRegionYield { + fn get_successor_operands(&self, _point: RegionBranchPoint) -> SuccessorOperandRange<'_> { + SuccessorOperandRange::forward(self.yielded()) + } + + fn get_mutable_successor_operands( + &mut self, + _point: RegionBranchPoint, + ) -> SuccessorOperandRangeMut<'_> { + SuccessorOperandRangeMut::forward(self.yielded_mut()) + } + + fn get_successor_regions( + &self, + _operands: &[Option], + ) -> SmallVec<[RegionSuccessorInfo; 2]> { + let region = self.parent_region().expect("test yield must be in a region"); + let owner = self.parent_op().expect("test yield region must have an owner"); + let owner = owner.borrow(); + let owner = owner + .downcast_ref::() + .expect("test yield must be nested in TestRegionBranch"); + if region == owner.first().as_region_ref() { + core::iter::once(RegionSuccessorInfo::Entering(owner.second().as_region_ref())) + .collect() + } else { + core::iter::once(RegionSuccessorInfo::Returning(SmallVec::new())).collect() + } + } + } + + #[operation(dialect = TestDialect)] + pub struct TestUnknownRegionOwner { + #[region] + body: Region, + } + + #[operation(dialect = TestDialect)] + pub struct TestMarker {} + + #[test] + fn region_branch_entry_does_not_bypass_an_intermediate_child_sink() { + let mut test = + Test::new("region_branch_entry_does_not_bypass_an_intermediate_child_sink", &[], &[]); + let (owner, target) = { + let mut builder = test.function_builder(); + let owner = builder.builder_mut().create::(SourceSpan::UNKNOWN)() + .unwrap(); + let first = owner.borrow().first().as_region_ref(); + let second = owner.borrow().second().as_region_ref(); + + let sink = builder.builder_mut().create_block(first, None, &[]); + builder.builder_mut().set_insertion_point_to_end(sink); + builder + .builder_mut() + .create::)>(SourceSpan::UNKNOWN)( + sink, + Vec::new(), + ) + .unwrap(); + + // The abstract owner graph advertises first -> second, but this concrete terminator is + // disconnected from the first region's entry and therefore cannot enable that edge. + let disconnected = builder.builder_mut().create_block(first, None, &[]); + builder.builder_mut().set_insertion_point_to_end(disconnected); + builder + .builder_mut() + .create::,)>(SourceSpan::UNKNOWN)( + Vec::new() + ) + .unwrap(); + + let second_entry = builder.builder_mut().create_block(second, None, &[]); + builder.builder_mut().set_insertion_point_to_end(second_entry); + let target = builder + .builder_mut() + .create::,)>(SourceSpan::UNKNOWN)( + Vec::new() + ) + .unwrap(); + (owner.as_operation_ref(), target.as_operation_ref()) + }; + + assert_eq!( + Operation::reachability(owner, target), + Reachability::Impossible, + "an abstract child-to-child edge must not bypass a sink in the intermediate child" + ); + } + + #[test] + fn unknown_region_owner_semantics_remain_maybe() { + let mut test = Test::new("unknown_region_owner_semantics_remain_maybe", &[], &[]); + let (owner, nested) = { + let mut builder = test.function_builder(); + let owner = + builder.builder_mut().create::(SourceSpan::UNKNOWN)() + .unwrap(); + let body = owner.borrow().body().as_region_ref(); + let entry = builder.builder_mut().create_block(body, None, &[]); + builder.builder_mut().set_insertion_point_to_end(entry); + builder + .builder_mut() + .create::)>(SourceSpan::UNKNOWN)( + entry, + Vec::new(), + ) + .unwrap(); + let disconnected = builder.builder_mut().create_block(body, None, &[]); + builder.builder_mut().set_insertion_point_to_end(disconnected); + let nested = + builder.builder_mut().create::(SourceSpan::UNKNOWN)().unwrap(); + (owner.as_operation_ref(), nested.as_operation_ref()) + }; + + assert_eq!( + Operation::reachability(owner, nested), + Reachability::Maybe, + "an unmodeled region owner must stay conservative even for a disconnected block" + ); + } +}