From 3c3825f5f37fe49c94f1fce25ca22ba6b4a3ecb3 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 14:13:14 +0000 Subject: [PATCH 1/6] Add 50%-failing test, run it 10 times --- src/extension/infer.rs | 56 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index db0b66694..2af9c2af4 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -690,11 +690,15 @@ mod test { use super::*; use crate::builder::test::closed_dfg_root_hugr; + use crate::builder::{DFGBuilder, Dataflow, DataflowHugr}; + use crate::extension::prelude::QB_T; use crate::extension::{prelude::PRELUDE_REGISTRY, ExtensionSet}; use crate::hugr::{validate::ValidationError, Hugr, HugrMut, HugrView, NodeType}; use crate::macros::const_extension_ids; - use crate::ops::OpType; + use crate::ops::custom::{ExternalOp, OpaqueOp}; use crate::ops::{self, dataflow::IOTrait, handle::NodeHandle, OpTrait}; + use crate::ops::{LeafOp, OpType}; + use crate::type_row; use crate::types::{FunctionType, Type, TypeRow}; @@ -1557,4 +1561,54 @@ mod test { Ok(()) } + + /// This was stack-overflowing approx 50% of the time, + /// see https://github.com/CQCL/hugr/issues/633 + #[test] + fn plus_on_self() -> Result<(), Box> { + let ext = ExtensionId::new("unknown1").unwrap(); + let delta = ExtensionSet::singleton(&ext); + let ft = FunctionType::new_linear(type_row![QB_T, QB_T]).with_extension_delta(&delta); + let mut dfg = DFGBuilder::new(ft.clone())?; + + // While https://github.com/CQCL-DEV/hugr/issues/388 is unsolved, + // most operations have empty extension_reqs (not including their own extension). + // Define some that do. + let binop: LeafOp = ExternalOp::Opaque(OpaqueOp::new( + ext.clone(), + "2qb_op", + String::new(), + vec![], + Some(ft), + )) + .into(); + let unary_sig = FunctionType::new_linear(type_row![QB_T]) + .with_extension_delta(&ExtensionSet::singleton(&ext)); + let unop: LeafOp = ExternalOp::Opaque(OpaqueOp::new( + ext, + "1qb_op", + String::new(), + vec![], + Some(unary_sig), + )) + .into(); + // Constrain q1,q2 as PLUS(ext1, inputs): + let [q1, q2] = dfg + .add_dataflow_op(binop.clone(), dfg.input_wires())? + .outputs_arr(); + // Constrain q1 as PLUS(ext2, q2): + let [q1] = dfg.add_dataflow_op(unop, [q1])?.outputs_arr(); + // Constrain q1 as EQUALS(q2) by using both together + dfg.finish_hugr_with_outputs([q1, q2], &PRELUDE_REGISTRY)?; + // The combined q1+q2 variable now has two PLUS constraints - on itself and the inputs. + // That leads to this stack-overflowing ~50% of the time + Ok(()) + } + + /// [plus_on_self] has about a 50% rate of failing with stack overflow. + /// So if we run 10 times, that should succeed about 1 run in 2^10, i.e. <0.1% + #[test] + fn plus_on_self_10_times() { + [0; 10].iter().for_each(|_| plus_on_self().unwrap()) + } } From 780ed6d3e8ea24a2115ff2080d0d80d7809a540c Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 15:12:44 +0000 Subject: [PATCH 2/6] Rewrite live-vars: live if ANY input live, excluding cycles --- src/extension/infer.rs | 77 +++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index 2af9c2af4..f90512445 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -22,7 +22,7 @@ use super::validate::ExtensionError; use petgraph::graph as pg; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use thiserror::Error; @@ -546,57 +546,56 @@ impl UnificationContext { pub fn results(&self) -> Result { // Check that all of the metavariables associated with nodes of the // graph are solved + let dependees = { + let mut h: HashMap> = HashMap::new(); + for (m, m2) in self.constraints.iter().flat_map(|(m, cs)| { + cs.iter().flat_map(|c| match c { + Constraint::Plus(_, m2) => Some((*m, *m2)), + _ => None, + }) + }) { + h.entry(m2).or_default().push(m); + } + h + }; + // Calculate "live" metas, i.e. those that depend upon an unsolved non-variable + let mut live_metas = HashSet::new(); + // Start with definitively live metas: + let mut queue = VecDeque::from_iter(self.extensions.values().filter(|m| { + let m = self.resolve(**m); + !self.variables.contains(&m) + && !self.solved.contains_key(&m) + // if it depends on anything, it might not be live + // (thus, nodes on cycles considered non-live, and "ok" for below.) + // TODO: remove cycles of plus constraints first? + && !dependees.contains_key(&m) + })); + while let Some(m) = queue.pop_front() { + if live_metas.insert(m) { + if let Some(d) = dependees.get(m) { + queue.extend(d.iter()) + } + } + } + let mut results: ExtensionSolution = HashMap::new(); for (loc, meta) in self.extensions.iter() { if let Some(rs) = self.get_solution(meta) { if loc.1 == Direction::Incoming { results.insert(loc.0, rs.clone()); } - } else if self.live_var(meta).is_some() { + } else if live_metas.contains(meta) { // If it depends on some other live meta, that's bad news. return Err(InferExtensionError::Unsolved { location: *loc }); } // If it only depends on graph variables, then we don't have // a *solution*, but it's fine } - debug_assert!(self.live_metas().is_empty()); - Ok(results) - } - - // Get the live var associated with a meta. - // TODO: This should really be a list - fn live_var(&self, m: &Meta) -> Option { - if self.variables.contains(m) || self.variables.contains(&self.resolve(*m)) { - return None; - } - - // TODO: We should be doing something to ensure that these are the same check... - if self.get_solution(m).is_none() { - if let Some(cs) = self.get_constraints(m) { - for c in cs { - match c { - Constraint::Plus(_, m) => return self.live_var(m), - _ => panic!("we shouldn't be here!"), - } - } - } - Some(*m) - } else { - None - } - } - - /// Return the set of "live" metavariables in the context. - /// "Live" here means a metavariable: - /// - Is associated to a location in the graph in `UnifyContext.extensions` - /// - Is still unsolved - /// - Isn't a variable - fn live_metas(&self) -> HashSet { - self.extensions + debug_assert!(self + .extensions .values() - .filter_map(|m| self.live_var(m)) - .filter(|m| !self.variables.contains(m)) - .collect() + .all(|m| !live_metas.contains(&self.resolve(*m)))); + Ok(results) } /// Iterates over a set of metas (the argument) and tries to solve From db742fb1fba8b4fc87385e2cc78114f4cac49059 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 16:48:41 +0000 Subject: [PATCH 3/6] fix: use a simple definition, and resolve more --- src/extension/infer.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index f90512445..1af9dbf6a 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -550,7 +550,7 @@ impl UnificationContext { let mut h: HashMap> = HashMap::new(); for (m, m2) in self.constraints.iter().flat_map(|(m, cs)| { cs.iter().flat_map(|c| match c { - Constraint::Plus(_, m2) => Some((*m, *m2)), + Constraint::Plus(_, m2) => Some((*m, self.resolve(*m2))), _ => None, }) }) { @@ -558,20 +558,15 @@ impl UnificationContext { } h }; - // Calculate "live" metas, i.e. those that depend upon an unsolved non-variable - let mut live_metas = HashSet::new(); - // Start with definitively live metas: - let mut queue = VecDeque::from_iter(self.extensions.values().filter(|m| { - let m = self.resolve(**m); - !self.variables.contains(&m) - && !self.solved.contains_key(&m) - // if it depends on anything, it might not be live - // (thus, nodes on cycles considered non-live, and "ok" for below.) - // TODO: remove cycles of plus constraints first? - && !dependees.contains_key(&m) - })); + // Calculate everything dependent upon a variable. + // Note it would be better to find metas ALL of whose dependencies + // were only on variables, but this is more complex, and hard to define + // if there are cycles of PLUS constraints, so leaving that as a TODO + // until we've handled such cycles. + let mut var_deps = HashSet::new(); + let mut queue = VecDeque::from_iter(self.variables.iter()); while let Some(m) = queue.pop_front() { - if live_metas.insert(m) { + if var_deps.insert(m) { if let Some(d) = dependees.get(m) { queue.extend(d.iter()) } @@ -584,7 +579,7 @@ impl UnificationContext { if loc.1 == Direction::Incoming { results.insert(loc.0, rs.clone()); } - } else if live_metas.contains(meta) { + } else if !var_deps.contains(&self.resolve(*meta)) { // If it depends on some other live meta, that's bad news. return Err(InferExtensionError::Unsolved { location: *loc }); } @@ -594,7 +589,7 @@ impl UnificationContext { debug_assert!(self .extensions .values() - .all(|m| !live_metas.contains(&self.resolve(*m)))); + .all(|m| self.get_solution(m).is_some() || var_deps.contains(&self.resolve(*m)))); Ok(results) } From 44b94754ce6ef4f03131de65b428efd38a3bca6a Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 17:12:06 +0000 Subject: [PATCH 4/6] Update comments --- src/extension/infer.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index 1af9dbf6a..660fdce29 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -559,10 +559,9 @@ impl UnificationContext { h }; // Calculate everything dependent upon a variable. - // Note it would be better to find metas ALL of whose dependencies - // were only on variables, but this is more complex, and hard to define - // if there are cycles of PLUS constraints, so leaving that as a TODO - // until we've handled such cycles. + // Note it would be better to find metas ALL of whose dependencies were (transitively) + // on variables, but this is more complex, and hard to define if there are cycles + // of PLUS constraints, so leaving that as a TODO until we've handled such cycles. let mut var_deps = HashSet::new(); let mut queue = VecDeque::from_iter(self.variables.iter()); while let Some(m) = queue.pop_front() { @@ -1595,12 +1594,11 @@ mod test { // Constrain q1 as EQUALS(q2) by using both together dfg.finish_hugr_with_outputs([q1, q2], &PRELUDE_REGISTRY)?; // The combined q1+q2 variable now has two PLUS constraints - on itself and the inputs. - // That leads to this stack-overflowing ~50% of the time Ok(()) } - /// [plus_on_self] has about a 50% rate of failing with stack overflow. - /// So if we run 10 times, that should succeed about 1 run in 2^10, i.e. <0.1% + /// [plus_on_self] had about a 50% rate of failing with stack overflow. + /// So if we run 10 times, that would succeed about 1 run in 2^10, i.e. <0.1% #[test] fn plus_on_self_10_times() { [0; 10].iter().for_each(|_| plus_on_self().unwrap()) From 7780c5c5fd7442b7ed00a224e01d2a4df8a9fd11 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 3 Nov 2023 09:11:03 +0000 Subject: [PATCH 5/6] Renames, comments --- src/extension/infer.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index 660fdce29..c86710e76 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -546,7 +546,7 @@ impl UnificationContext { pub fn results(&self) -> Result { // Check that all of the metavariables associated with nodes of the // graph are solved - let dependees = { + let depended_upon = { let mut h: HashMap> = HashMap::new(); for (m, m2) in self.constraints.iter().flat_map(|(m, cs)| { cs.iter().flat_map(|c| match c { @@ -562,11 +562,11 @@ impl UnificationContext { // Note it would be better to find metas ALL of whose dependencies were (transitively) // on variables, but this is more complex, and hard to define if there are cycles // of PLUS constraints, so leaving that as a TODO until we've handled such cycles. - let mut var_deps = HashSet::new(); + let mut depends_on_var = HashSet::new(); let mut queue = VecDeque::from_iter(self.variables.iter()); while let Some(m) = queue.pop_front() { - if var_deps.insert(m) { - if let Some(d) = dependees.get(m) { + if depends_on_var.insert(m) { + if let Some(d) = depended_upon.get(m) { queue.extend(d.iter()) } } @@ -578,17 +578,17 @@ impl UnificationContext { if loc.1 == Direction::Incoming { results.insert(loc.0, rs.clone()); } - } else if !var_deps.contains(&self.resolve(*meta)) { + } else if !depends_on_var.contains(&self.resolve(*meta)) { // If it depends on some other live meta, that's bad news. return Err(InferExtensionError::Unsolved { location: *loc }); } - // If it only depends on graph variables, then we don't have + // If it ("only"??) depends on graph variables, then we don't have // a *solution*, but it's fine } debug_assert!(self .extensions .values() - .all(|m| self.get_solution(m).is_some() || var_deps.contains(&self.resolve(*m)))); + .all(|m| self.get_solution(m).is_some() || depends_on_var.contains(&self.resolve(*m)))); Ok(results) } From 89b45ed82a6910be2a5c8c8de303c95d7b385c5d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 09:26:51 +0000 Subject: [PATCH 6/6] Update comments, remove redundant assert --- src/extension/infer.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/extension/infer.rs b/src/extension/infer.rs index 5ebb0b8b7..47eb88e67 100644 --- a/src/extension/infer.rs +++ b/src/extension/infer.rs @@ -553,17 +553,13 @@ impl UnificationContext { if loc.1 == Direction::Incoming { results.insert(loc.0, rs.clone()); } - } else if !depends_on_var.contains(&self.resolve(*meta)) { - // If it depends on some other live meta, that's bad news. - return Err(InferExtensionError::Unsolved { location: *loc }); + } else { + // Unsolved nodes must be unsolved because they depend on graph variables. + if !depends_on_var.contains(&self.resolve(*meta)) { + return Err(InferExtensionError::Unsolved { location: *loc }); + } } - // If it ("only"??) depends on graph variables, then we don't have - // a *solution*, but it's fine } - debug_assert!(self - .extensions - .values() - .all(|m| self.get_solution(m).is_some() || depends_on_var.contains(&self.resolve(*m)))); Ok(results) }