From c868b86b71e740d32aff0370a50179dd169c3ec3 Mon Sep 17 00:00:00 2001 From: Agustin Borgna Date: Fri, 29 Sep 2023 06:57:39 +0100 Subject: [PATCH 1/2] fix: Don't insert temporary wires when extracting a subgraph We connected the temporary built graph inputs and outputs, but the logic didn't take into account reorderings in the signature, or non-linear parameters. We can avoid the headache by not validating the intermediate construct. --- src/hugr/views/sibling_subgraph.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/hugr/views/sibling_subgraph.rs b/src/hugr/views/sibling_subgraph.rs index b94bd993d..1cf95ca1a 100644 --- a/src/hugr/views/sibling_subgraph.rs +++ b/src/hugr/views/sibling_subgraph.rs @@ -10,13 +10,14 @@ //! hierarchy. use std::collections::HashSet; +use std::mem; use itertools::Itertools; use portgraph::{view::Subgraph, Direction, PortView}; use thiserror::Error; -use crate::builder::{Dataflow, DataflowHugr, FunctionBuilder}; -use crate::extension::{ExtensionSet, PRELUDE_REGISTRY}; +use crate::builder::{Container, FunctionBuilder}; +use crate::extension::ExtensionSet; use crate::hugr::{HugrError, HugrMut}; use crate::types::Signature; use crate::{ @@ -415,23 +416,20 @@ impl SiblingSubgraph { signature: self.signature(hugr), input_extensions, }; - let builder = FunctionBuilder::new(name, signature).unwrap(); - let inputs = builder.input_wires(); - let mut extracted = builder - .finish_hugr_with_outputs(inputs, &PRELUDE_REGISTRY) - .unwrap(); + let mut builder = FunctionBuilder::new(name, signature).unwrap(); + // Take the unfinished Hugr from the builder, to avoid unnecessary + // validation checks that require connecting the inputs an outputs. + let mut extracted = mem::take(builder.hugr_mut()); let node_map = extracted .insert_subgraph(extracted.root(), hugr, self)? .node_map; - // Disconnect the input and output nodes, and connect the inserted nodes - // in-between. + // Connect the inserted nodes in-between the input and output nodes. let [inp, out] = extracted.get_io(extracted.root()).unwrap(); for (inp_port, repl_ports) in extracted .node_ports(inp, Direction::Outgoing) .zip(self.inputs.iter()) { - extracted.disconnect(inp, inp_port)?; for (repl_node, repl_port) in repl_ports { extracted.connect(inp, inp_port, node_map[repl_node], *repl_port)?; } @@ -651,6 +649,7 @@ pub enum InvalidSubgraph { mod tests { use std::error::Error; + use crate::extension::PRELUDE_REGISTRY; use crate::{ builder::{ BuildError, DFGBuilder, Dataflow, DataflowHugr, DataflowSubContainer, HugrBuilder, From e3a68201ea4451415151bccc0b9f8cd219a10a1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agust=C3=ADn=20Borgna?= <121866228+aborgna-q@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:51:06 +0100 Subject: [PATCH 2/2] Update src/hugr/views/sibling_subgraph.rs Co-authored-by: Seyon Sivarajah --- src/hugr/views/sibling_subgraph.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hugr/views/sibling_subgraph.rs b/src/hugr/views/sibling_subgraph.rs index 1cf95ca1a..0ea1a434b 100644 --- a/src/hugr/views/sibling_subgraph.rs +++ b/src/hugr/views/sibling_subgraph.rs @@ -418,7 +418,7 @@ impl SiblingSubgraph { }; let mut builder = FunctionBuilder::new(name, signature).unwrap(); // Take the unfinished Hugr from the builder, to avoid unnecessary - // validation checks that require connecting the inputs an outputs. + // validation checks that require connecting the inputs and outputs. let mut extracted = mem::take(builder.hugr_mut()); let node_map = extracted .insert_subgraph(extracted.root(), hugr, self)?