From 4bd058fc1b10e6382a9fb72f6f4c3c2fa728ff53 Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud <7252775+indietyp@users.noreply.github.com> Date: Sun, 5 Apr 2026 21:00:09 +0200 Subject: [PATCH 1/3] fix: behaviour --- libs/@local/hashql/compiletest/src/lib.rs | 1 - libs/@local/hashql/eval/src/lib.rs | 1 - libs/@local/hashql/mir/src/lib.rs | 3 +- .../pass/analysis/data_dependency/resolve.rs | 144 +++++++----------- .../pass/analysis/data_dependency/tests.rs | 43 ++++++ .../data-dependency/load_param_mixed.snap | 21 +++ 6 files changed, 124 insertions(+), 89 deletions(-) create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/load_param_mixed.snap diff --git a/libs/@local/hashql/compiletest/src/lib.rs b/libs/@local/hashql/compiletest/src/lib.rs index 51e9b74cf78..c2280893dcd 100644 --- a/libs/@local/hashql/compiletest/src/lib.rs +++ b/libs/@local/hashql/compiletest/src/lib.rs @@ -7,7 +7,6 @@ // Library Features allocator_api, - assert_matches, duration_millis_float, exitcode_exit_method, file_buffered, diff --git a/libs/@local/hashql/eval/src/lib.rs b/libs/@local/hashql/eval/src/lib.rs index 627b7411bc5..06d48157bb7 100644 --- a/libs/@local/hashql/eval/src/lib.rs +++ b/libs/@local/hashql/eval/src/lib.rs @@ -11,7 +11,6 @@ // Library Features iterator_try_collect, - assert_matches, allocator_api, iter_array_chunks, maybe_uninit_fill, diff --git a/libs/@local/hashql/mir/src/lib.rs b/libs/@local/hashql/mir/src/lib.rs index dcbb5b2831a..e83d45234d7 100644 --- a/libs/@local/hashql/mir/src/lib.rs +++ b/libs/@local/hashql/mir/src/lib.rs @@ -21,14 +21,15 @@ get_mut_unchecked, iter_array_chunks, iter_collect_into, + iterator_try_reduce, likely_unlikely, maybe_uninit_fill, + maybe_uninit_uninit_array_transpose, option_into_flat_iter, step_trait, temporary_niche_types, try_trait_v2, variant_count, - maybe_uninit_uninit_array_transpose )] #![cfg_attr(test, feature( // Library Features diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs index 254394474da..2066bec724e 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs @@ -181,11 +181,12 @@ fn traverse<'heap, A: Allocator + Clone>( } } -/// Attempts to resolve a block parameter by checking all predecessor edges. +/// Attempts to resolve a block parameter by checking all predecessor values. /// -/// A block parameter may receive values from multiple predecessor blocks. This function -/// traverses all [`Param`] edges and checks whether they resolve to the same source. -/// If all predecessors agree, resolution continues through that common source. +/// A block parameter may receive values from multiple predecessor blocks, either as +/// graph edges (when arguments are places) or constant bindings (when arguments are +/// constants). This function checks whether all predecessors, from both sources, +/// resolve to the same value. /// /// Handles cycle detection: if we encounter a local already in the `visited` set, /// we return [`Backtrack`] to unwind. The cycle root (where `visited` was first @@ -198,45 +199,70 @@ fn resolve_params<'heap, A: Allocator + Clone>( mut state: ResolutionState<'_, '_, 'heap, A>, place: PlaceRef<'_, 'heap>, ) -> ControlFlow, Local> { - let mut edges = state.graph.outgoing_edges(place.local); - let Some(head) = edges.next() else { - unreachable!("caller must guarantee that at least one Param edge exists") - }; + let graph = state.graph; + + // Check whether graph Param edges exist (cycle detection is only relevant for graph edges, + // which are the only source of back-edges). + let has_graph_edges = graph.outgoing_edges(place.local).next().is_some(); // Cycle detection: if we've already visited this local, backtrack. - if let Some(visited) = &mut state.visited + if has_graph_edges + && let Some(visited) = &mut state.visited && !visited.insert(place.local) { return ControlFlow::Break(ResolutionResult::Backtrack); } - // Initialize cycle tracking if this is the first Param traversal. + // Initialize cycle tracking if this is the first Param traversal with graph edges. let mut owned_visited = None; - let visited_ref = state.visited.as_deref_mut().or_else(|| { - let mut set = DenseBitSet::new_empty(state.graph.graph.node_count()); - set.insert(place.local); - - owned_visited = Some(set); - owned_visited.as_mut() - }); + let visited_ref = if has_graph_edges { + state.visited.as_deref_mut().or_else(|| { + let mut set = DenseBitSet::new_empty(graph.graph.node_count()); + set.insert(place.local); + + owned_visited = Some(set); + owned_visited.as_mut() + }) + } else { + state.visited.as_deref_mut() + }; let mut rec_state = ResolutionState { - graph: state.graph, + graph, interner: state.interner, alloc: state.alloc.clone(), visited: visited_ref, }; - let first = traverse(rec_state.cloned(), place, head); + // Graph Param edges (when a local has Param edges, all its outgoing edges are Param). + let graph_edges = graph + .outgoing_edges(place.local) + .map(|edge| traverse(rec_state.cloned(), place, edge)); + let constant_edges = graph + .constant_bindings + .iter_by_kind(place.local, EdgeKind::Param) + .map(|constant| { + ControlFlow::Break(ResolutionResult::Resolved(Operand::Constant(constant))) + }); - // Check consensus: all predecessors must resolve to the same result. - let all_agree = edges.all(|edge| traverse(rec_state.cloned(), place, edge) == first); + // `try_reduce` returns: + // `Some(Some(v))` when all predecessors agree on `v` + // `Some(None)` when the iterator is empty (unreachable: caller guarantees predecessors) + // `None` when the closure short-circuits (predecessors disagree) + let consensus = match graph_edges + .chain(constant_edges) + .try_reduce(|lhs, rhs| (lhs == rhs).then_some(lhs)) + { + Some(None) => unreachable!("caller must guarantee at least one Param predecessor exists"), + Some(consensus) => consensus, + None => None, + }; - if all_agree { + if let Some(consensus) = consensus { // If we initiated backtracking (owned_visited is Some) and got Backtrack, // we are the cycle root and should treat this as incomplete. let is_cycle_root = - first == ControlFlow::Break(ResolutionResult::Backtrack) && owned_visited.is_some(); + consensus == ControlFlow::Break(ResolutionResult::Backtrack) && owned_visited.is_some(); if !is_cycle_root { // Clean up visited state before returning. @@ -244,7 +270,7 @@ fn resolve_params<'heap, A: Allocator + Clone>( visited.remove(place.local); } - return first; + return consensus; } } @@ -263,46 +289,6 @@ fn resolve_params<'heap, A: Allocator + Clone>( })) } -/// Attempts to resolve a block parameter by checking constant bindings from all predecessors. -/// -/// This handles the case where a block parameter receives constant values from predecessor -/// blocks, but has no graph edges (only constant bindings with [`Param`] kind). The function -/// checks whether all predecessors provide the same constant value. -/// -/// Unlike [`resolve_params`], this function does not need cycle detection because it only -/// examines constant bindings, not graph edges that could form back-edges. -/// -/// # Returns -/// -/// - [`Resolved(Constant)`] if all predecessor constants agree on the same value -/// - [`Resolved(Place)`] if predecessors diverge (the place remains valid but has no constant) -/// -/// [`Param`]: EdgeKind::Param -/// [`Resolved(Constant)`]: ResolutionResult::Resolved -/// [`Resolved(Place)`]: ResolutionResult::Resolved -fn resolve_params_const<'heap, A: Allocator + Clone>( - state: &ResolutionState<'_, '_, 'heap, A>, - place: PlaceRef<'_, 'heap>, -) -> ResolutionResult<'heap, A> { - debug_assert!(place.projections.is_empty()); - let mut constants = state - .graph - .constant_bindings - .iter_by_kind(place.local, EdgeKind::Param); - let Some(head) = constants.next() else { - unreachable!("caller must guarantee that at least one Param edge exists") - }; - - let all_agree = constants.all(|constant| constant == head); - if all_agree { - ResolutionResult::Resolved(Operand::Constant(head)) - } else { - // We have finished (we have terminated on a param, which is divergent, therefore the place - // is still valid, just doesn't have a constant value) - ResolutionResult::Resolved(Operand::Place(Place::local(place.local))) - } -} - /// Resolves a place to its ultimate data source by traversing the dependency graph. /// /// Starting from `place`, this function follows edges in the dependency graph to find where @@ -332,13 +318,10 @@ pub(crate) fn resolve<'heap, A: Allocator + Clone>( mut place: PlaceRef<'_, 'heap>, ) -> ResolutionResult<'heap, A> { // Scan outgoing edges to find Load and count Param edges. - let mut edges = 0_usize; let mut params = 0_usize; let mut load_edge = None; for edge in state.graph.outgoing_edges(place.local) { - edges += 1; - match edge.data.kind { EdgeKind::Load => load_edge = Some(edge), EdgeKind::Param => params += 1, @@ -355,26 +338,15 @@ pub(crate) fn resolve<'heap, A: Allocator + Clone>( } // Attempt to resolve through Param edges, if all predecessors agree. - // There are fundamentally two cases: - // - Either all graph edges are Param edges, or - // - all constant bindings are Param edges - if edges == 0 - && state - .graph - .constant_bindings - .find_by_kind(place.local, EdgeKind::Param) - .is_some() - { - return resolve_params_const(&state, place); - } + // Predecessors may arrive as graph edges (place arguments), constant bindings + // (constant arguments), or a mix of both. All sources are checked for consensus. + let has_param_constants = state + .graph + .constant_bindings + .find_by_kind(place.local, EdgeKind::Param) + .is_some(); - if params > 0 - && state - .graph - .constant_bindings - .find_by_kind(place.local, EdgeKind::Param) - .is_none() - { + if params > 0 || has_param_constants { place.local = tri!(resolve_params(state.cloned(), place)); } diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs index b840c7435f0..6e3b5a26275 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs @@ -544,3 +544,46 @@ fn projection_prepending_opaque_source() { }, ); } + +/// Tests mixed Param resolution through nested tuple wrapping where predecessors provide +/// a mix of constants and projections that all resolve to the same value. +#[test] +fn load_param_mixed() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl _1: (Int), _3: Int, _4: (Int), _5: Int; + @proj _1_0 = _1.0: Int, _4_0 = _4.0: Int; + + bb0() { + goto bb2(42); + }, + bb1() { + _1 = tuple 42; + goto bb2(_1_0); + }, + bb2(_3) { + _4 = tuple _3; + goto bb4(_4_0); + }, + bb3() { + goto bb4(42); + }, + bb4(_5) { + return _5; + } + }); + + assert_data_dependency( + "load_param_mixed", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/load_param_mixed.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/load_param_mixed.snap new file mode 100644 index 00000000000..d641ecea391 --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/load_param_mixed.snap @@ -0,0 +1,21 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +assertion_line: 32 +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%1 -> %0 [Param, projections: .0] +%2 -> %1 [Index(FieldIndex(0))] +%3 -> %2 [Param, projections: .0] +%0 -> 42 [Index(FieldIndex(0))] +%1 -> 42 [Param] +%3 -> 42 [Param] + + +===== + +%0 -> 42 [Index(FieldIndex(0))] +%1 -> 42 [Param] +%1 -> 42 [Param] +%2 -> 42 [Index(FieldIndex(0))] +%3 -> 42 [Param] +%3 -> 42 [Param] From 9297e69b010e37d6f45ec781eb56846ceead3d4f Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud <7252775+indietyp@users.noreply.github.com> Date: Sun, 5 Apr 2026 21:43:23 +0200 Subject: [PATCH 2/3] feat: allow for recursive loops --- libs/@local/hashql/mir/src/builder/mod.rs | 3 +- libs/@local/hashql/mir/src/lib.rs | 2 +- .../pass/analysis/data_dependency/resolve.rs | 59 +++++--- .../pass/analysis/data_dependency/tests.rs | 131 ++++++++++++++++++ .../param_cycle_detection.snap | 5 +- ...aram_cycle_multi_node_with_const_init.snap | 17 +++ ...aram_cycle_visited_cleanup_on_diverge.snap | 19 +++ .../param_cycle_with_const_init.snap | 15 ++ 8 files changed, 226 insertions(+), 25 deletions(-) create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_multi_node_with_const_init.snap create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_visited_cleanup_on_diverge.snap create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_with_const_init.snap diff --git a/libs/@local/hashql/mir/src/builder/mod.rs b/libs/@local/hashql/mir/src/builder/mod.rs index b56c57f31f0..67057786240 100644 --- a/libs/@local/hashql/mir/src/builder/mod.rs +++ b/libs/@local/hashql/mir/src/builder/mod.rs @@ -119,10 +119,9 @@ pub use self::{ /// # Unary Operators /// /// ``` -/// use hashql_hir::node::operation::UnOp; +/// use hashql_mir::body::rvalue::UnOp; /// use hashql_mir::op; /// -/// assert!(matches!(op![!], UnOp::Not)); /// assert!(matches!(op![neg], UnOp::Neg)); // `neg` is used since `-` alone is ambiguous /// assert!(matches!(op![~], UnOp::BitNot)); /// ``` diff --git a/libs/@local/hashql/mir/src/lib.rs b/libs/@local/hashql/mir/src/lib.rs index e83d45234d7..ee755063446 100644 --- a/libs/@local/hashql/mir/src/lib.rs +++ b/libs/@local/hashql/mir/src/lib.rs @@ -21,7 +21,6 @@ get_mut_unchecked, iter_array_chunks, iter_collect_into, - iterator_try_reduce, likely_unlikely, maybe_uninit_fill, maybe_uninit_uninit_array_transpose, @@ -30,6 +29,7 @@ temporary_niche_types, try_trait_v2, variant_count, + iterator_try_reduce )] #![cfg_attr(test, feature( // Library Features diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs index 2066bec724e..8dfbaa8f1fb 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs @@ -185,12 +185,13 @@ fn traverse<'heap, A: Allocator + Clone>( /// /// A block parameter may receive values from multiple predecessor blocks, either as /// graph edges (when arguments are places) or constant bindings (when arguments are -/// constants). This function checks whether all predecessors, from both sources, -/// resolve to the same value. +/// constants). This function checks whether all non-cyclic predecessors, from both +/// sources, resolve to the same value. /// -/// Handles cycle detection: if we encounter a local already in the `visited` set, -/// we return [`Backtrack`] to unwind. The cycle root (where `visited` was first -/// initialized) catches the backtrack and returns [`Incomplete`]. +/// Cyclic predecessors ([`Backtrack`]) are filtered out before consensus checking. +/// Since [`Param`] edges are identity transfers, the value is fully determined by +/// the non-cyclic init edges. If only cyclic predecessors exist (no external source), +/// the cycle root returns [`Incomplete`] and non-root nodes propagate [`Backtrack`]. /// /// [`Param`]: EdgeKind::Param /// [`Backtrack`]: ResolutionResult::Backtrack @@ -234,7 +235,10 @@ fn resolve_params<'heap, A: Allocator + Clone>( visited: visited_ref, }; - // Graph Param edges (when a local has Param edges, all its outgoing edges are Param). + // Resolve all predecessor candidates and check consensus. + // Cyclic predecessors (Backtrack) are skipped: since Param edges are identity transfers, + // the value is fully determined by the non-cyclic init edges. If only cyclic predecessors + // exist, we cannot resolve (the value has no external source). let graph_edges = graph .outgoing_edges(place.local) .map(|edge| traverse(rec_state.cloned(), place, edge)); @@ -249,22 +253,22 @@ fn resolve_params<'heap, A: Allocator + Clone>( // `Some(Some(v))` when all predecessors agree on `v` // `Some(None)` when the iterator is empty (unreachable: caller guarantees predecessors) // `None` when the closure short-circuits (predecessors disagree) - let consensus = match graph_edges + let mut backtrack_occurred = false; + let consensus = graph_edges .chain(constant_edges) - .try_reduce(|lhs, rhs| (lhs == rhs).then_some(lhs)) - { - Some(None) => unreachable!("caller must guarantee at least one Param predecessor exists"), - Some(consensus) => consensus, - None => None, - }; + .filter(|candidate| { + if matches!(candidate, ControlFlow::Break(ResolutionResult::Backtrack)) { + backtrack_occurred = true; + return false; + } - if let Some(consensus) = consensus { - // If we initiated backtracking (owned_visited is Some) and got Backtrack, - // we are the cycle root and should treat this as incomplete. - let is_cycle_root = - consensus == ControlFlow::Break(ResolutionResult::Backtrack) && owned_visited.is_some(); + true + }) + .try_reduce(|lhs, rhs| (lhs == rhs).then_some(lhs)); - if !is_cycle_root { + match consensus { + // Predecessors agree on a value. + Some(Some(consensus)) => { // Clean up visited state before returning. if let Some(visited) = state.visited { visited.remove(place.local); @@ -272,6 +276,21 @@ fn resolve_params<'heap, A: Allocator + Clone>( return consensus; } + + // All candidates were cyclic (no non-cyclic predecessors to determine the value). + // If we're not the cycle root, propagate Backtrack so the root can handle it. + Some(None) if backtrack_occurred && owned_visited.is_none() => { + if let Some(visited) = &mut state.visited { + visited.remove(place.local); + } + + return ControlFlow::Break(ResolutionResult::Backtrack); + } + // Pure cycle at root: fall through to Incomplete. + Some(None) if backtrack_occurred => {} + Some(None) => unreachable!("caller must guarantee at least one Param predecessor exists"), + // Predecessors disagree. + None => {} } // Clean up visited state before returning incomplete. @@ -279,7 +298,7 @@ fn resolve_params<'heap, A: Allocator + Clone>( visited.remove(place.local); } - // Predecessors diverge or a cycle was detected; cannot resolve through this param. + // Non-cyclic predecessors diverge, or pure cycle at root. let mut projections = VecDeque::new_in(state.alloc.clone()); projections.extend(place.projections); diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs index 6e3b5a26275..a3d53b7b5eb 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs @@ -300,6 +300,137 @@ fn param_cycle_detection() { ); } +/// Tests that a loop-carried parameter resolves through the non-cyclic init edge +/// when the back-edge just passes the value through unchanged. +/// +/// The init edge provides constant 42, the back-edge creates a cycle (x depends on x). +/// Since cyclic predecessors are identity transfers, the non-cyclic init edge determines +/// the value: x should resolve to 42. +#[test] +fn param_cycle_with_const_init() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl x: Int, cond: Int; + + bb0() { + cond = input.load! "cond"; + goto bb1(42); + }, + bb1(x) { + if cond then bb1(x) else bb2(x); + }, + bb2(x) { + return x; + } + }); + + assert_data_dependency( + "param_cycle_with_const_init", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} + +/// Tests that a multi-node cycle with a constant init edge resolves correctly, +/// even when the node with the init edge is not the cycle root. +/// +/// The cycle is x -> y -> x (through bb1 -> bb2 -> bb1). The init edge provides +/// constant 42 to x from bb0. During resolution of y, x is encountered as a non-root +/// participant in the cycle. x must skip the cyclic Backtrack from y and use its +/// non-cyclic constant init edge to resolve to 42, which then propagates through y +/// and out to the consumer (result). +#[test] +fn param_cycle_multi_node_with_const_init() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl x: Int, y: Int, cond: Int, result: Int; + + bb0() { + cond = input.load! "cond"; + goto bb1(42); + }, + bb1(x) { + goto bb2(x); + }, + bb2(y) { + if cond then bb1(y) else bb3(y); + }, + bb3(result) { + return result; + } + }); + + assert_data_dependency( + "param_cycle_multi_node_with_const_init", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} + +/// Tests that the visited set is cleaned up when non-cyclic predecessors disagree +/// inside another node's cycle resolution. +/// +/// y has a self-loop (creating a cycle). When resolving y, the cycle root tracks +/// visited locals. x is resolved inside y's resolution and has disagreeing predecessors +/// (constant 42 from bb0, opaque `input` from bb1). x must remove itself from the +/// visited set before returning Incomplete, otherwise later resolutions would see +/// false cycle detections. +#[test] +fn param_cycle_visited_cleanup_on_diverge() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl input: Int, x: Int, y: Int, cond: Int; + + bb0() { + input = input.load! "x"; + cond = input.load! "cond"; + goto bb3(42); + }, + bb1() { + goto bb3(input); + }, + bb3(x) { + goto bb4(x); + }, + bb4(y) { + if cond then bb4(y) else bb5(y); + }, + bb5(y) { + return y; + } + }); + + assert_data_dependency( + "param_cycle_visited_cleanup_on_diverge", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} + /// Tests constant propagation through edges. #[test] fn constant_propagation() { diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_detection.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_detection.snap index 78e219ea059..e174f7581df 100644 --- a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_detection.snap +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_detection.snap @@ -1,5 +1,6 @@ --- source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +assertion_line: 32 expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" --- %1 -> %0 [Param] @@ -10,5 +11,5 @@ expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" ===== %1 -> %0 [Param] -%3 -> %1 [Param] -%1 -> %1 [Param] +%3 -> %0 [Param] +%1 -> %0 [Param] diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_multi_node_with_const_init.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_multi_node_with_const_init.snap new file mode 100644 index 00000000000..5627aa46982 --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_multi_node_with_const_init.snap @@ -0,0 +1,17 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +assertion_line: 32 +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%1 -> %0 [Param] +%3 -> %1 [Param] +%0 -> %1 [Param] +%0 -> 42 [Param] + + +===== + +%0 -> 42 [Param] +%0 -> 42 [Param] +%1 -> 42 [Param] +%3 -> 42 [Param] diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_visited_cleanup_on_diverge.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_visited_cleanup_on_diverge.snap new file mode 100644 index 00000000000..4417a007b7c --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_visited_cleanup_on_diverge.snap @@ -0,0 +1,19 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +assertion_line: 32 +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%1 -> %0 [Param] +%2 -> %1 [Param] +%2 -> %2 [Param] +%2 -> %2 [Param] +%1 -> 42 [Param] + + +===== + +%1 -> %0 [Param] +%2 -> %1 [Param] +%2 -> %1 [Param] +%2 -> %1 [Param] +%1 -> 42 [Param] diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_with_const_init.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_with_const_init.snap new file mode 100644 index 00000000000..3e000a7998f --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_with_const_init.snap @@ -0,0 +1,15 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +assertion_line: 32 +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%0 -> %0 [Param] +%0 -> %0 [Param] +%0 -> 42 [Param] + + +===== + +%0 -> 42 [Param] +%0 -> 42 [Param] +%0 -> 42 [Param] From 77eb677126f8d1a6380a1fb6077ab449c85d160b Mon Sep 17 00:00:00 2001 From: Bilal Mahmoud <7252775+indietyp@users.noreply.github.com> Date: Mon, 6 Apr 2026 21:27:32 +0200 Subject: [PATCH 3/3] chore: follow projections on param edges --- .../pass/analysis/data_dependency/resolve.rs | 49 +++++- .../pass/analysis/data_dependency/tests.rs | 148 ++++++++++++++++++ .../src/pass/transform/inst_simplify/mod.rs | 4 + ...param_consensus_projected_field_const.snap | 22 +++ ...param_consensus_projected_field_place.snap | 22 +++ ...param_cycle_invariant_projected_field.snap | 22 +++ 6 files changed, 263 insertions(+), 4 deletions(-) create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_const.snap create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_place.snap create mode 100644 libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_invariant_projected_field.snap diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs index 8dfbaa8f1fb..f57dc651b3d 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/resolve.rs @@ -188,6 +188,20 @@ fn traverse<'heap, A: Allocator + Clone>( /// constants). This function checks whether all non-cyclic predecessors, from both /// sources, resolve to the same value. /// +/// # Projection-aware consensus +/// +/// When the queried place has a projection suffix (e.g., resolving `x.0` where `x` is a +/// block parameter), consensus is checked on the *fully resolved* result per predecessor, +/// not on the partially resolved predecessor bases. This is necessary because different +/// predecessor locals can still agree on a projected field. +/// +/// For example, if predecessor A passes `(42, u)` and predecessor B passes `(42, v)`, +/// the bases disagree but `A.0 == B.0 == 42`. The algorithm resolves each predecessor +/// through the full projection suffix before comparing, so this case correctly yields +/// `Resolved(42)` rather than `Incomplete(x.0)`. +/// +/// # Cycle handling +/// /// Cyclic predecessors ([`Backtrack`]) are filtered out before consensus checking. /// Since [`Param`] edges are identity transfers, the value is fully determined by /// the non-cyclic init edges. If only cyclic predecessors exist (no external source), @@ -236,12 +250,35 @@ fn resolve_params<'heap, A: Allocator + Clone>( }; // Resolve all predecessor candidates and check consensus. + // + // When the queried place has projections (e.g., `x.field`), each predecessor is resolved + // through the full projection suffix before consensus comparison. If `traverse` returns + // `Continue(local)` (predecessor base resolved to a bare local), we call `resolve` on + // `local.projections` to complete the resolution. This ensures consensus is checked on + // the final value, not intermediate bases that may differ structurally but agree on the + // projected component. + // // Cyclic predecessors (Backtrack) are skipped: since Param edges are identity transfers, // the value is fully determined by the non-cyclic init edges. If only cyclic predecessors // exist, we cannot resolve (the value has no external source). - let graph_edges = graph - .outgoing_edges(place.local) - .map(|edge| traverse(rec_state.cloned(), place, edge)); + let graph_edges = graph.outgoing_edges(place.local).map(|edge| { + let result = traverse(rec_state.cloned(), place, edge); + + match result { + // Predecessor resolved to a bare local, but the query has remaining projections. + // Finish resolving through the projection suffix so consensus compares final values. + ControlFlow::Continue(local) if !place.projections.is_empty() => { + ControlFlow::Break(resolve( + rec_state.cloned(), + PlaceRef { + local, + projections: place.projections, + }, + )) + } + ControlFlow::Continue(_) | ControlFlow::Break(_) => result, + } + }); let constant_edges = graph .constant_bindings .iter_by_kind(place.local, EdgeKind::Param) @@ -314,7 +351,11 @@ fn resolve_params<'heap, A: Allocator + Clone>( /// the data ultimately originates. The algorithm handles three types of edges: /// /// - **[`Load`]**: Always followed transitively (a load has exactly one source) -/// - **[`Param`]**: Followed only if all predecessors agree on the same source (consensus) +/// - **[`Param`]**: Followed only if all predecessors agree on the same source (consensus). +/// Consensus is checked on fully resolved results: when the queried place has projections, each +/// predecessor is resolved through the complete projection suffix before comparison. This allows +/// resolution through φ-nodes where predecessor bases differ but the projected component agrees +/// (e.g., `(42, a)` and `(42, b)` agree on field `.0`). /// - **[`Index`]/[`Field`]**: Matched against projections to trace through aggregates /// /// Resolution terminates with: diff --git a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs index a3d53b7b5eb..ee724ed3cf6 100644 --- a/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +++ b/libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs @@ -718,3 +718,151 @@ fn load_param_mixed() { }, ); } + +/// Tests that Param consensus resolves through projections when predecessors are +/// different tuples but the queried field is the same constant. +/// +/// Both paths construct different tuples (`a = (42, u)`, `b = (42, v)`) but the +/// `.0` field is the same constant `42` in both. Current algorithm compares the +/// tuple bases (`a` vs `b`), which disagree, so it returns `Incomplete(x.0)`. +/// Correct behavior: resolve `a.0` and `b.0` individually, find they both yield +/// `42`, and return `Resolved(42)`. +#[test] +fn param_consensus_projected_field_const() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl u: Int, v: Int, a: (Int, Int), b: (Int, Int), cond: Int, x: (Int, Int), result: Int; + @proj x_0 = x.0: Int; + + bb0() { + u = input.load! "u"; + v = input.load! "v"; + cond = input.load! "cond"; + a = tuple 42, u; + b = tuple 42, v; + if cond then bb1() else bb2(); + }, + bb1() { + goto bb3(a); + }, + bb2() { + goto bb3(b); + }, + bb3(x) { + result = load x_0; + return result; + } + }); + + assert_data_dependency( + "param_consensus_projected_field_const", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} + +/// Tests that Param consensus resolves through projections when predecessors are +/// different tuples but the queried field is the same place. +/// +/// Both paths construct different tuples (`a = (src, u)`, `b = (src, v)`) but the +/// `.0` field is the same local `src` in both. Current algorithm compares the +/// tuple bases (`a` vs `b`), which disagree, so it returns `Incomplete(x.0)`. +/// Correct behavior: resolve `a.0` and `b.0` individually, find they both yield +/// `src`, and return `Resolved(src)`. +#[test] +fn param_consensus_projected_field_place() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl src: Int, u: Int, v: Int, a: (Int, Int), b: (Int, Int), cond: Int, x: (Int, Int), result: Int; + @proj x_0 = x.0: Int; + + bb0() { + src = input.load! "src"; + u = input.load! "u"; + v = input.load! "v"; + cond = input.load! "cond"; + a = tuple src, u; + b = tuple src, v; + if cond then bb1() else bb2(); + }, + bb1() { + goto bb3(a); + }, + bb2() { + goto bb3(b); + }, + bb3(x) { + result = load x_0; + return result; + } + }); + + assert_data_dependency( + "param_consensus_projected_field_place", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} + +/// Tests that a cycle with a loop-invariant projected field resolves correctly. +/// +/// The cycle is `x -> x` via the back-edge in `bb1`. The init edge provides +/// `init = (src, other)`. The back-edge reconstructs `t = (x.0, other)`, +/// preserving `x.0` across iterations. So `x.0` is loop-invariant and should +/// resolve to `src`. Current algorithm compares `init` vs `t` as bases, which +/// disagree, yielding `Incomplete(x.0)`. Correct behavior: resolve the full +/// `init.0 = src` and see that `t.0 = x.0` is a cyclic identity, so the +/// non-cyclic init determines the answer. +#[test] +fn param_cycle_invariant_projected_field() { + let heap = Heap::new(); + let interner = Interner::new(&heap); + let env = Environment::new(&heap); + + let body = body!(interner, env; fn@0/0 -> Int { + decl src: Int, other: Int, init: (Int, Int), x: (Int, Int), t: (Int, Int), cond: Int, result: Int; + @proj x_0 = x.0: Int; + + bb0() { + src = input.load! "src"; + other = input.load! "other"; + cond = input.load! "cond"; + init = tuple src, other; + goto bb1(init); + }, + bb1(x) { + t = tuple x_0, other; + if cond then bb1(t) else bb2(x_0); + }, + bb2(result) { + return result; + } + }); + + assert_data_dependency( + "param_cycle_invariant_projected_field", + &body, + &mut MirContext { + heap: &heap, + env: &env, + interner: &interner, + diagnostics: DiagnosticIssues::new(), + }, + ); +} diff --git a/libs/@local/hashql/mir/src/pass/transform/inst_simplify/mod.rs b/libs/@local/hashql/mir/src/pass/transform/inst_simplify/mod.rs index ade4c6e0626..2fc24b17e1c 100644 --- a/libs/@local/hashql/mir/src/pass/transform/inst_simplify/mod.rs +++ b/libs/@local/hashql/mir/src/pass/transform/inst_simplify/mod.rs @@ -309,6 +309,8 @@ impl<'heap, A: Allocator> InstSimplifyVisitor<'_, 'heap, A> { (BinOp::BitAnd, 0) if is_bool => { Some(RValue::Load(Operand::Constant(Constant::Int(false.into())))) } + // 0 & rhs => 0 (annihilator) + (BinOp::BitAnd, 0) => Some(RValue::Load(Operand::Constant(Constant::Int(0.into())))), (BinOp::BitAnd, _) => None, // 0 | rhs => rhs (identity) (BinOp::BitOr, 0) => Some(RValue::Load(Operand::Place(rhs))), @@ -369,6 +371,8 @@ impl<'heap, A: Allocator> InstSimplifyVisitor<'_, 'heap, A> { (BinOp::BitAnd, 0) if is_bool => { Some(RValue::Load(Operand::Constant(Constant::Int(false.into())))) } + // 0 & lhs => 0 (annihilator) + (BinOp::BitAnd, 0) => Some(RValue::Load(Operand::Constant(Constant::Int(0.into())))), (BinOp::BitAnd, _) => None, // lhs | 0 => lhs (identity) (BinOp::BitOr, 0) => Some(RValue::Load(Operand::Place(lhs))), diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_const.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_const.snap new file mode 100644 index 00000000000..7efaeecb79c --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_const.snap @@ -0,0 +1,22 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%2 -> %0 [Index(FieldIndex(1))] +%3 -> %1 [Index(FieldIndex(1))] +%5 -> %2 [Param] +%5 -> %3 [Param] +%6 -> %5 [Load, projections: .0] +%2 -> 42 [Index(FieldIndex(0))] +%3 -> 42 [Index(FieldIndex(0))] + + +===== + +%2 -> %0 [Index(FieldIndex(1))] +%3 -> %1 [Index(FieldIndex(1))] +%5 -> %2 [Param] +%5 -> %3 [Param] +%2 -> 42 [Index(FieldIndex(0))] +%3 -> 42 [Index(FieldIndex(0))] +%6 -> 42 [Load] diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_place.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_place.snap new file mode 100644 index 00000000000..d9c5b6d6de4 --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_consensus_projected_field_place.snap @@ -0,0 +1,22 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%3 -> %0 [Index(FieldIndex(0))] +%3 -> %1 [Index(FieldIndex(1))] +%4 -> %0 [Index(FieldIndex(0))] +%4 -> %2 [Index(FieldIndex(1))] +%6 -> %3 [Param] +%6 -> %4 [Param] +%7 -> %6 [Load, projections: .0] + + +===== + +%3 -> %0 [Index(FieldIndex(0))] +%3 -> %1 [Index(FieldIndex(1))] +%4 -> %0 [Index(FieldIndex(0))] +%4 -> %2 [Index(FieldIndex(1))] +%6 -> %3 [Param] +%6 -> %4 [Param] +%7 -> %0 [Load] diff --git a/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_invariant_projected_field.snap b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_invariant_projected_field.snap new file mode 100644 index 00000000000..601e7e15f75 --- /dev/null +++ b/libs/@local/hashql/mir/tests/ui/pass/data-dependency/param_cycle_invariant_projected_field.snap @@ -0,0 +1,22 @@ +--- +source: libs/@local/hashql/mir/src/pass/analysis/data_dependency/tests.rs +expression: "format!(\"{graph}\\n\\n=====\\n\\n{transient}\")" +--- +%2 -> %0 [Index(FieldIndex(0))] +%2 -> %1 [Index(FieldIndex(1))] +%3 -> %2 [Param] +%4 -> %3 [Index(FieldIndex(0)), projections: .0] +%4 -> %1 [Index(FieldIndex(1))] +%6 -> %3 [Param, projections: .0] +%3 -> %4 [Param] + + +===== + +%2 -> %0 [Index(FieldIndex(0))] +%2 -> %1 [Index(FieldIndex(1))] +%3 -> %2 [Param] +%4 -> %0 [Index(FieldIndex(0))] +%4 -> %1 [Index(FieldIndex(1))] +%6 -> %0 [Param] +%3 -> %4 [Param]