From 7e28c31e32e1482f1aec6aca4c9dcb518ea73826 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 17:24:01 +0100 Subject: [PATCH 1/7] Add HierarchyTester --- hugr-core/src/hierarchy.rs | 138 +++++++++++++++++++++++++++++++++++++ hugr-core/src/lib.rs | 1 + 2 files changed, 139 insertions(+) create mode 100644 hugr-core/src/hierarchy.rs diff --git a/hugr-core/src/hierarchy.rs b/hugr-core/src/hierarchy.rs new file mode 100644 index 0000000000..f49640c753 --- /dev/null +++ b/hugr-core/src/hierarchy.rs @@ -0,0 +1,138 @@ +//! Çontains [HierarchyTester], a tool for examining the hierarchy +//! about ancestry and descendancy. + +use std::collections::{HashMap, hash_map::Entry}; + +use itertools::Itertools; + +use crate::HugrView; + +/// Caches enough information on the hierarchy of an immutably-held Hugr +/// to allow efficient querying of [Self::is_ancestor_of] and [Self::which_child_contains] +#[derive(Clone, Debug)] +pub struct HierarchyTester<'a, H: HugrView> { + /// This both allows us to access the Hugr, but also guarantees the Hugr isn't + /// changing beneath our back to invalidate the results. + hugr: &'a H, + /// The entry and exit indices (inclusive) of every node in the Hugr + /// beneath the entrypoint. (Note: every entry number is different.) + entry_exit: HashMap, +} + +impl<'a, H: HugrView> HierarchyTester<'a, H> { + /// Creates a new instance that knows about all descendants of the + /// specified Hugr's entrypoint + pub fn new(hugr: &'a H) -> Self { + let mut entry_exit = HashMap::new(); + fn traverse(hugr: &H, n: H::Node, ee: &mut HashMap) { + let old = ee.insert(n, (ee.len(), usize::MAX)); // second is placeholder for now + debug_assert!(old.is_none()); + for ch in hugr.children(n) { + traverse(hugr, ch, ee) + } + let end_idx = ee.len() - 1; + let Entry::Occupied(oe) = ee.entry(n) else { + panic!() + }; + let (_, end) = oe.into_mut(); + *end = end_idx; + debug_assert!( + // Could do this on every which_child_contains?! + hugr.children(n) + .tuple_windows() + .all(|(a, b)| ee.get(&a).unwrap().1 == ee.get(&b).unwrap().0 - 1) + ); + } + traverse(hugr, hugr.entrypoint(), &mut entry_exit); + Self { hugr, entry_exit } + } + + /// Returns true if `anc` is an ancestor of `desc`, including `anc == desc`. + /// (See also [Self::is_strict_ancestor_of].) + /// Constant time regardless of size/depth of Hugr. + pub fn is_ancestor_of(&self, anc: H::Node, desc: H::Node) -> bool { + let anc = self.entry_exit.get(&anc).unwrap(); + let desc = self.entry_exit.get(&desc).unwrap(); + anc.0 <= desc.0 && desc.0 <= anc.1 + } + + /// Returns true if `anc` is an ancestor of `desc`, excluding `anc == desc`. + /// (See also [Self::is_ancestor_of].) + /// Constant time regardless of size/depth of Hugr. + pub fn is_strict_ancestor_of(&self, anc: H::Node, desc: H::Node) -> bool { + let anc = self.entry_exit.get(&anc).unwrap(); + let desc = self.entry_exit.get(&desc).unwrap(); + anc.0 < desc.0 && desc.1 <= anc.1 + } + + /// Returns the child of `parent` which is an ancestor of `desc` - unique if there is one. + /// Time O(n) in number of children of `parent` regardless of size/depth of Hugr. + pub fn which_child_contains(&self, parent: H::Node, desc: H::Node) -> Option { + // If we cached child *Vecs* then we could do a binary search, but since we don't, + // even just getting the children into a Vec is linear, so we might as well... + for c in self.hugr.children(parent) { + if self.is_ancestor_of(c, desc) { + return Some(c); + } + } + None + } +} + +#[cfg(test)] +mod test { + use std::iter; + + use proptest::prelude::{Just, Strategy}; + use proptest::proptest; + + use crate::builder::{DFGBuilder, Dataflow, HugrBuilder, SubContainer}; + use crate::extension::prelude::usize_t; + use crate::types::Signature; + use crate::{Hugr, HugrView}; + + use super::HierarchyTester; + + #[derive(Clone, Debug)] + struct Layout(Vec); + + fn make + AsRef>(dfg: &mut DFGBuilder, l: Layout) { + let [mut val] = dfg.input_wires_arr(); + for c in l.0 { + let mut c_b = dfg + .dfg_builder(Signature::new_endo(usize_t()), [val]) + .unwrap(); + make(&mut c_b, c); + let [res] = c_b.finish_sub_container().unwrap().outputs_arr(); + val = res; + } + dfg.set_outputs([val]).unwrap() + } + + fn any_layout() -> impl Strategy { + Just(Layout(vec![])).prop_recursive(5, 10, 3, |elem| { + proptest::collection::vec(elem, 1..5).prop_map(Layout) + }) + } + + fn strict_ancestors(h: &H, n: H::Node) -> impl Iterator { + iter::successors(h.get_parent(n), |n| h.get_parent(*n)) + } + + proptest! { + #[test] + fn hierarchy_test(ly in any_layout()) { + let mut h = DFGBuilder::new(Signature::new_endo(usize_t())).unwrap(); + make(&mut h, ly); + let h = h.finish_hugr().unwrap(); + let ht = HierarchyTester::new(&h); + for n1 in h.entry_descendants() { + for n2 in h.entry_descendants() { + let is_strict_ancestor = strict_ancestors(&h, n1).any(|item| item==n2); + assert_eq!(ht.is_strict_ancestor_of(n2, n1), is_strict_ancestor); + assert_eq!(ht.is_ancestor_of(n2, n1), is_strict_ancestor || n1 == n2); + } + } + } + } +} diff --git a/hugr-core/src/lib.rs b/hugr-core/src/lib.rs index e5f57d2a8f..e22bbb5162 100644 --- a/hugr-core/src/lib.rs +++ b/hugr-core/src/lib.rs @@ -14,6 +14,7 @@ pub mod core; pub mod envelope; pub mod export; pub mod extension; +pub mod hierarchy; pub mod hugr; pub mod import; pub mod macros; From 05cf0bc1d407f1272ecaa35ce666a42b4236a533 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 20:25:41 +0100 Subject: [PATCH 2/7] Add HierarchyTester::nearest_enclosing_funcdefn --- hugr-core/src/hierarchy.rs | 52 +++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/hugr-core/src/hierarchy.rs b/hugr-core/src/hierarchy.rs index f49640c753..3c42d782fc 100644 --- a/hugr-core/src/hierarchy.rs +++ b/hugr-core/src/hierarchy.rs @@ -1,5 +1,5 @@ -//! Çontains [HierarchyTester], a tool for examining the hierarchy -//! about ancestry and descendancy. +//! Çontains [HierarchyTester], a tool for efficiently querying the hierarchy +//! (constant-time in size and depth of Hugr) use std::collections::{HashMap, hash_map::Entry}; @@ -7,16 +7,20 @@ use itertools::Itertools; use crate::HugrView; +type NodeData = (usize, usize, Option); // start, end, enclosing func + /// Caches enough information on the hierarchy of an immutably-held Hugr -/// to allow efficient querying of [Self::is_ancestor_of] and [Self::which_child_contains] +/// to allow efficient querying of [Self::is_ancestor_of] and [Self::nearest_enclosing_funcdefn]. +/// Also supports [Self::which_child_contains] (less efficiently). #[derive(Clone, Debug)] pub struct HierarchyTester<'a, H: HugrView> { /// This both allows us to access the Hugr, but also guarantees the Hugr isn't /// changing beneath our back to invalidate the results. hugr: &'a H, - /// The entry and exit indices (inclusive) of every node in the Hugr - /// beneath the entrypoint. (Note: every entry number is different.) - entry_exit: HashMap, + /// The entry and exit indices (inclusive), of every node in the Hugr + /// beneath the entrypoint, and the nearest strictly-enclosing FuncDefn. + /// (Note: every entry number is different.) + entry_exit: HashMap, } impl<'a, H: HugrView> HierarchyTester<'a, H> { @@ -24,17 +28,25 @@ impl<'a, H: HugrView> HierarchyTester<'a, H> { /// specified Hugr's entrypoint pub fn new(hugr: &'a H) -> Self { let mut entry_exit = HashMap::new(); - fn traverse(hugr: &H, n: H::Node, ee: &mut HashMap) { - let old = ee.insert(n, (ee.len(), usize::MAX)); // second is placeholder for now + fn traverse( + hugr: &H, + n: H::Node, + mut fd: Option, + ee: &mut HashMap, + ) { + let old = ee.insert(n, (ee.len(), usize::MAX, fd)); // second is placeholder for now debug_assert!(old.is_none()); + if hugr.get_optype(n).is_func_defn() { + fd = Some(n) + } for ch in hugr.children(n) { - traverse(hugr, ch, ee) + traverse(hugr, ch, fd, ee) } let end_idx = ee.len() - 1; let Entry::Occupied(oe) = ee.entry(n) else { panic!() }; - let (_, end) = oe.into_mut(); + let (_, end, _) = oe.into_mut(); *end = end_idx; debug_assert!( // Could do this on every which_child_contains?! @@ -43,13 +55,16 @@ impl<'a, H: HugrView> HierarchyTester<'a, H> { .all(|(a, b)| ee.get(&a).unwrap().1 == ee.get(&b).unwrap().0 - 1) ); } - traverse(hugr, hugr.entrypoint(), &mut entry_exit); + traverse(hugr, hugr.entrypoint(), None, &mut entry_exit); Self { hugr, entry_exit } } /// Returns true if `anc` is an ancestor of `desc`, including `anc == desc`. /// (See also [Self::is_strict_ancestor_of].) - /// Constant time regardless of size/depth of Hugr. + /// + /// # Panics + /// + /// if `n` is not an entry-descendant in the Hugr pub fn is_ancestor_of(&self, anc: H::Node, desc: H::Node) -> bool { let anc = self.entry_exit.get(&anc).unwrap(); let desc = self.entry_exit.get(&desc).unwrap(); @@ -59,12 +74,25 @@ impl<'a, H: HugrView> HierarchyTester<'a, H> { /// Returns true if `anc` is an ancestor of `desc`, excluding `anc == desc`. /// (See also [Self::is_ancestor_of].) /// Constant time regardless of size/depth of Hugr. + /// + /// # Panics + /// + /// if `n` is not an entry-descendant in the Hugr pub fn is_strict_ancestor_of(&self, anc: H::Node, desc: H::Node) -> bool { let anc = self.entry_exit.get(&anc).unwrap(); let desc = self.entry_exit.get(&desc).unwrap(); anc.0 < desc.0 && desc.1 <= anc.1 } + /// Returns the nearest strictly-enclosing [FuncDefn](crate::ops::FuncDefn) of a node + /// + /// # Panics + /// + /// if `n` is not an entry-descendant in the Hugr + pub fn nearest_enclosing_funcdefn(&self, n: H::Node) -> Option { + self.entry_exit.get(&n).unwrap().2 + } + /// Returns the child of `parent` which is an ancestor of `desc` - unique if there is one. /// Time O(n) in number of children of `parent` regardless of size/depth of Hugr. pub fn which_child_contains(&self, parent: H::Node, desc: H::Node) -> Option { From e0282867ed34d68e78ecebe4a29fc984d0a8c9a0 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 21:10:27 +0100 Subject: [PATCH 3/7] Add HierarchyTester::nearest_enclosing_funcdefn --- hugr-core/src/hierarchy.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/hugr-core/src/hierarchy.rs b/hugr-core/src/hierarchy.rs index 3c42d782fc..73fa2c75c7 100644 --- a/hugr-core/src/hierarchy.rs +++ b/hugr-core/src/hierarchy.rs @@ -7,7 +7,9 @@ use itertools::Itertools; use crate::HugrView; -type NodeData = (usize, usize, Option); // start, end, enclosing func +/// The entry and exit indices (inclusive, note every entry number is different); +/// and the nearest strictly-enclosing FuncDefn. +type NodeData = (usize, usize, Option); /// Caches enough information on the hierarchy of an immutably-held Hugr /// to allow efficient querying of [Self::is_ancestor_of] and [Self::nearest_enclosing_funcdefn]. @@ -17,10 +19,7 @@ pub struct HierarchyTester<'a, H: HugrView> { /// This both allows us to access the Hugr, but also guarantees the Hugr isn't /// changing beneath our back to invalidate the results. hugr: &'a H, - /// The entry and exit indices (inclusive), of every node in the Hugr - /// beneath the entrypoint, and the nearest strictly-enclosing FuncDefn. - /// (Note: every entry number is different.) - entry_exit: HashMap, + entry_exit: HashMap> // for every node beneath entrypoint } impl<'a, H: HugrView> HierarchyTester<'a, H> { @@ -32,7 +31,7 @@ impl<'a, H: HugrView> HierarchyTester<'a, H> { hugr: &H, n: H::Node, mut fd: Option, - ee: &mut HashMap, + ee: &mut HashMap>, ) { let old = ee.insert(n, (ee.len(), usize::MAX, fd)); // second is placeholder for now debug_assert!(old.is_none()); From 06572ccf940ee4b11808adaa42d0601746f966f6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 20:29:00 +0100 Subject: [PATCH 4/7] Try to rewrite validation. break 'found_ancestor... --- hugr-core/src/hugr/validate.rs | 165 +++++++++++++++++---------------- 1 file changed, 84 insertions(+), 81 deletions(-) diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index e07df4ed12..56f3a206e0 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -1,7 +1,6 @@ //! HUGR invariant checks. use std::collections::HashMap; -use std::iter; use itertools::Itertools; use petgraph::algo::dominators::{self, Dominators}; @@ -11,6 +10,7 @@ use thiserror::Error; use crate::core::HugrNode; use crate::extension::SignatureError; +use crate::hierarchy::HierarchyTester; use crate::ops::constant::ConstTypeError; use crate::ops::custom::{ExtensionOp, OpaqueOpError}; use crate::ops::validate::{ @@ -32,6 +32,7 @@ use super::views::HugrView; /// Hugr to avoid recomputing it every time. pub(super) struct ValidationContext<'a, H: HugrView> { hugr: &'a H, + hierarchy_tester: Option>, /// Dominator tree for each CFG region, using the container node as index. dominators: HashMap, H::RegionPortgraphNodes)>, } @@ -40,7 +41,11 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { /// Create a new validation context. pub fn new(hugr: &'a H) -> Self { let dominators = HashMap::new(); - Self { hugr, dominators } + Self { + hugr, + hierarchy_tester: None, + dominators, + } } /// Check the validity of the HUGR. @@ -87,11 +92,11 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { /// The results of this computation should be cached in `self.dominators`. /// We don't do it here to avoid mutable borrows. fn compute_dominator( - &self, + h: &H, parent: H::Node, ) -> (Dominators, H::RegionPortgraphNodes) { - let (region, node_map) = self.hugr.region_portgraph(parent); - let entry_node = self.hugr.children(parent).next().unwrap(); + let (region, node_map) = h.region_portgraph(parent); + let entry_node = h.children(parent).next().unwrap(); let doms = dominators::simple_fast(®ion, node_map.to_portgraph(entry_node)); (doms, node_map) } @@ -437,94 +442,92 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { }); } - // To detect either external or dominator edges, we traverse the ancestors - // of the target until we find either `from_parent` (in the external - // case), or the parent of `from_parent` (in the dominator case). - // - // This search could be sped-up with a pre-computed LCA structure, but - // for valid Hugrs this search should be very short. - // - // For Value edges only, we record any FuncDefn we went through; if there is - // any such, then that is an error, but we report that only if the dom/ext - // relation was otherwise ok (an error about an edge "entering" some ancestor - // node could be misleading if the source isn't where it's expected) - let mut err_entered_func = None; + let ht = &*self + .hierarchy_tester + .get_or_insert_with(|| HierarchyTester::new(self.hugr)); let from_parent_parent = self.hugr.get_parent(from_parent); - for (ancestor, ancestor_parent) in - iter::successors(to_parent, |&p| self.hugr.get_parent(p)).tuple_windows() - { - if !is_static && self.hugr.get_optype(ancestor).is_func_defn() { - err_entered_func.get_or_insert(InterGraphEdgeError::ValueEdgeIntoFunc { - to, - to_offset, - from, - from_offset, - func: ancestor, - }); - } - if ancestor_parent == from_parent { + + 'found_ancestor: { + if ht.is_strict_ancestor_of(from_parent, to) { // External edge. - err_entered_func.map_or(Ok(()), Err)?; - if !is_static { - // Must have an order edge. - self.hugr - .node_connections(from, ancestor) - .find(|&[p, _]| from_optype.port_kind(p) == Some(EdgeKind::StateOrder)) - .ok_or(InterGraphEdgeError::MissingOrderEdge { - from, - from_offset, - to, - to_offset, - to_ancestor: ancestor, - })?; + if is_static { + return Ok(()); } - return Ok(()); - } else if Some(ancestor_parent) == from_parent_parent && !is_static { - // Dominator edge - let ancestor_parent_op = self.hugr.get_optype(ancestor_parent); - if ancestor_parent_op.tag() != OpTag::Cfg { - return Err(InterGraphEdgeError::NonCFGAncestor { - from, - from_offset, - to, - to_offset, - ancestor_parent_op: ancestor_parent_op.clone(), - }); - } - err_entered_func.map_or(Ok(()), Err)?; - // Check domination - let (dominator_tree, node_map) = - if let Some(tree) = self.dominators.get(&ancestor_parent) { - tree - } else { - let (tree, node_map) = self.compute_dominator(ancestor_parent); - self.dominators.insert(ancestor_parent, (tree, node_map)); - self.dominators.get(&ancestor_parent).unwrap() - }; - if !dominator_tree - .dominators(node_map.to_portgraph(ancestor)) - .is_some_and(|mut ds| ds.any(|n| n == node_map.to_portgraph(from_parent))) + //Check for Order edge: some Order-successor should contain the target + assert_eq!( + from_optype.other_port_kind(Direction::Outgoing), + Some(EdgeKind::StateOrder) + ); + + if !self + .hugr + .linked_inputs(from, from_optype.other_output_port().unwrap()) + .any(|(sibling, _)| ht.is_strict_ancestor_of(sibling, to)) { - return Err(InterGraphEdgeError::NonDominatedAncestor { + return Err(InterGraphEdgeError::MissingOrderEdge { from, from_offset, to, to_offset, - from_parent, - ancestor, - }); + to_ancestor: ht.which_child_contains(from_parent, to).unwrap(), + })?; } - - return Ok(()); + break 'found_ancestor; + } else if let Some(fpp) = from_parent_parent { + if !is_static && ht.is_strict_ancestor_of(fpp, to) { + // Dominator edge + let from_grandparent_op = self.hugr.get_optype(fpp); + if from_grandparent_op.tag() != OpTag::Cfg { + return Err(InterGraphEdgeError::NonCFGAncestor { + from, + from_offset, + to, + to_offset, + ancestor_parent_op: from_grandparent_op.clone(), + }); + } + // Check domination + let (dominator_tree, node_map) = self + .dominators + .entry(fpp) + .or_insert_with(|| Self::compute_dominator(self.hugr, fpp)); + let ancestor = ht.which_child_contains(fpp, to).unwrap(); + if !dominator_tree + .dominators(node_map.to_portgraph(ancestor)) + .is_some_and(|mut ds| ds.any(|n| n == node_map.to_portgraph(from_parent))) + { + return Err(InterGraphEdgeError::NonDominatedAncestor { + from, + from_offset, + to, + to_offset, + from_parent, + ancestor, + }); + } + break 'found_ancestor; + } //else, NoRelation } + return Err(InterGraphEdgeError::NoRelation { + from, + from_offset, + to, + to_offset, + }); } - Err(InterGraphEdgeError::NoRelation { - from, - from_offset, - to, - to_offset, - }) + if let Some(func) = ht.nearest_enclosing_funcdefn(to) { + if !ht.is_strict_ancestor_of(func, from) { + return Err(InterGraphEdgeError::ValueEdgeIntoFunc { + to, + to_offset, + from, + from_offset, + func, + }); + } + } + Ok(()) } /// Validates that `TypeArgs` are valid wrt the [`ExtensionRegistry`] and that nodes From 9e4b9653dc2affd80452a2409c134611ef077d3b Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 20:40:30 +0100 Subject: [PATCH 5/7] Use Option.filter to flatten nested if and remove label/break --- hugr-core/src/hugr/validate.rs | 117 ++++++++++++++++----------------- 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index 56f3a206e0..90aca0f57c 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -447,67 +447,64 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { .get_or_insert_with(|| HierarchyTester::new(self.hugr)); let from_parent_parent = self.hugr.get_parent(from_parent); - 'found_ancestor: { - if ht.is_strict_ancestor_of(from_parent, to) { - // External edge. - if is_static { - return Ok(()); - } - //Check for Order edge: some Order-successor should contain the target - assert_eq!( - from_optype.other_port_kind(Direction::Outgoing), - Some(EdgeKind::StateOrder) - ); - - if !self - .hugr - .linked_inputs(from, from_optype.other_output_port().unwrap()) - .any(|(sibling, _)| ht.is_strict_ancestor_of(sibling, to)) - { - return Err(InterGraphEdgeError::MissingOrderEdge { - from, - from_offset, - to, - to_offset, - to_ancestor: ht.which_child_contains(from_parent, to).unwrap(), - })?; - } - break 'found_ancestor; - } else if let Some(fpp) = from_parent_parent { - if !is_static && ht.is_strict_ancestor_of(fpp, to) { - // Dominator edge - let from_grandparent_op = self.hugr.get_optype(fpp); - if from_grandparent_op.tag() != OpTag::Cfg { - return Err(InterGraphEdgeError::NonCFGAncestor { - from, - from_offset, - to, - to_offset, - ancestor_parent_op: from_grandparent_op.clone(), - }); - } - // Check domination - let (dominator_tree, node_map) = self - .dominators - .entry(fpp) - .or_insert_with(|| Self::compute_dominator(self.hugr, fpp)); - let ancestor = ht.which_child_contains(fpp, to).unwrap(); - if !dominator_tree - .dominators(node_map.to_portgraph(ancestor)) - .is_some_and(|mut ds| ds.any(|n| n == node_map.to_portgraph(from_parent))) - { - return Err(InterGraphEdgeError::NonDominatedAncestor { - from, - from_offset, - to, - to_offset, - from_parent, - ancestor, - }); - } - break 'found_ancestor; - } //else, NoRelation + if ht.is_strict_ancestor_of(from_parent, to) { + // External edge. + if is_static { + return Ok(()); + } + //Check for Order edge: some Order-successor should contain the target + assert_eq!( + from_optype.other_port_kind(Direction::Outgoing), + Some(EdgeKind::StateOrder) + ); + + if !self + .hugr + .linked_inputs(from, from_optype.other_output_port().unwrap()) + .any(|(sibling, _)| ht.is_strict_ancestor_of(sibling, to)) + { + return Err(InterGraphEdgeError::MissingOrderEdge { + from, + from_offset, + to, + to_offset, + to_ancestor: ht.which_child_contains(from_parent, to).unwrap(), + })?; + } + } else if let Some(fpp) = + from_parent_parent.filter(|fpp| !is_static && ht.is_strict_ancestor_of(*fpp, to)) + { + // Dominator edge + let from_grandparent_op = self.hugr.get_optype(fpp); + if from_grandparent_op.tag() != OpTag::Cfg { + return Err(InterGraphEdgeError::NonCFGAncestor { + from, + from_offset, + to, + to_offset, + ancestor_parent_op: from_grandparent_op.clone(), + }); + } + // Check domination + let (dominator_tree, node_map) = self + .dominators + .entry(fpp) + .or_insert_with(|| Self::compute_dominator(self.hugr, fpp)); + let ancestor = ht.which_child_contains(fpp, to).unwrap(); + if !dominator_tree + .dominators(node_map.to_portgraph(ancestor)) + .is_some_and(|mut ds| ds.any(|n| n == node_map.to_portgraph(from_parent))) + { + return Err(InterGraphEdgeError::NonDominatedAncestor { + from, + from_offset, + to, + to_offset, + from_parent, + ancestor, + }); } + } else { return Err(InterGraphEdgeError::NoRelation { from, from_offset, From 14bb9bcfb4dae93ed3e31440a7a3fc4d57263d62 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 20:53:48 +0100 Subject: [PATCH 6/7] Remove which_child_contains, add `containing_child` local helper --- hugr-core/src/hierarchy.rs | 19 ++----------------- hugr-core/src/hugr/validate.rs | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/hugr-core/src/hierarchy.rs b/hugr-core/src/hierarchy.rs index 73fa2c75c7..5aa3dc7ef6 100644 --- a/hugr-core/src/hierarchy.rs +++ b/hugr-core/src/hierarchy.rs @@ -13,13 +13,11 @@ type NodeData = (usize, usize, Option); /// Caches enough information on the hierarchy of an immutably-held Hugr /// to allow efficient querying of [Self::is_ancestor_of] and [Self::nearest_enclosing_funcdefn]. -/// Also supports [Self::which_child_contains] (less efficiently). #[derive(Clone, Debug)] pub struct HierarchyTester<'a, H: HugrView> { - /// This both allows us to access the Hugr, but also guarantees the Hugr isn't - /// changing beneath our back to invalidate the results. + #[allow(unused)] // Just make sure the Hugr isn't changing behind our back hugr: &'a H, - entry_exit: HashMap> // for every node beneath entrypoint + entry_exit: HashMap>, // for every node beneath entrypoint } impl<'a, H: HugrView> HierarchyTester<'a, H> { @@ -91,19 +89,6 @@ impl<'a, H: HugrView> HierarchyTester<'a, H> { pub fn nearest_enclosing_funcdefn(&self, n: H::Node) -> Option { self.entry_exit.get(&n).unwrap().2 } - - /// Returns the child of `parent` which is an ancestor of `desc` - unique if there is one. - /// Time O(n) in number of children of `parent` regardless of size/depth of Hugr. - pub fn which_child_contains(&self, parent: H::Node, desc: H::Node) -> Option { - // If we cached child *Vecs* then we could do a binary search, but since we don't, - // even just getting the children into a Vec is linear, so we might as well... - for c in self.hugr.children(parent) { - if self.is_ancestor_of(c, desc) { - return Some(c); - } - } - None - } } #[cfg(test)] diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index 90aca0f57c..da25c2733e 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -422,6 +422,17 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { to: H::Node, to_offset: Port, ) -> Result<(), InterGraphEdgeError> { + fn containing_child(hugr: &H, parent: H::Node, desc: H::Node) -> H::Node { + let mut n = desc; + loop { + let p = hugr.get_parent(n).unwrap(); + if p == parent { + return n; + } + n = p; + } + } + let from_parent = self .hugr .get_parent(from) @@ -468,7 +479,7 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { from_offset, to, to_offset, - to_ancestor: ht.which_child_contains(from_parent, to).unwrap(), + to_ancestor: containing_child(self.hugr, from_parent, to), })?; } } else if let Some(fpp) = @@ -490,7 +501,7 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { .dominators .entry(fpp) .or_insert_with(|| Self::compute_dominator(self.hugr, fpp)); - let ancestor = ht.which_child_contains(fpp, to).unwrap(); + let ancestor = containing_child(self.hugr, fpp, to); if !dominator_tree .dominators(node_map.to_portgraph(ancestor)) .is_some_and(|mut ds| ds.any(|n| n == node_map.to_portgraph(from_parent))) From d8bf18023735af7a7b8cb9b1efff9f5ad4016efa Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 16 May 2025 21:07:49 +0100 Subject: [PATCH 7/7] inline compute_dominator --- hugr-core/src/hugr/validate.rs | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index da25c2733e..cb5b24ac1f 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -86,21 +86,6 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { Ok(()) } - /// Compute the dominator tree for a CFG region, identified by its container - /// node. - /// - /// The results of this computation should be cached in `self.dominators`. - /// We don't do it here to avoid mutable borrows. - fn compute_dominator( - h: &H, - parent: H::Node, - ) -> (Dominators, H::RegionPortgraphNodes) { - let (region, node_map) = h.region_portgraph(parent); - let entry_node = h.children(parent).next().unwrap(); - let doms = dominators::simple_fast(®ion, node_map.to_portgraph(entry_node)); - (doms, node_map) - } - /// Check the constraints on a single node. /// /// This includes: @@ -497,10 +482,12 @@ impl<'a, H: HugrView> ValidationContext<'a, H> { }); } // Check domination - let (dominator_tree, node_map) = self - .dominators - .entry(fpp) - .or_insert_with(|| Self::compute_dominator(self.hugr, fpp)); + let (dominator_tree, node_map) = self.dominators.entry(fpp).or_insert_with(|| { + let (region, node_map) = self.hugr.region_portgraph(fpp); + let entry_node = self.hugr.children(fpp).next().unwrap(); + let doms = dominators::simple_fast(®ion, node_map.to_portgraph(entry_node)); + (doms, node_map) + }); let ancestor = containing_child(self.hugr, fpp, to); if !dominator_tree .dominators(node_map.to_portgraph(ancestor))