diff --git a/CHANGELOG.md b/CHANGELOG.md index 3790c6c810..5d7a8f6d7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ +## Unreleased +- Deprecate `Cell::find_with_attr` in favor of `Cell::find_with_unique_attr`. The former is error-prone because pass logic might implicitly assume that there is only one port with a particular attribute. + ## 0.5.1 - Change the `calyx` build script to use the `CALYX_PRIMITIVES_DIR` env variable to install primitive libraries. If unset, use `$HOME/.calyx`. diff --git a/calyx-ir/src/structure.rs b/calyx-ir/src/structure.rs index d722c34aa5..8d40bd8f8d 100644 --- a/calyx-ir/src/structure.rs +++ b/calyx-ir/src/structure.rs @@ -7,7 +7,7 @@ use super::{ Attributes, Direction, GetAttributes, Guard, Id, PortDef, RRC, WRC, }; use calyx_frontend::Attribute; -use calyx_utils::GetName; +use calyx_utils::{CalyxResult, Error, GetName}; use itertools::Itertools; use smallvec::{smallvec, SmallVec}; use std::hash::Hash; @@ -292,31 +292,45 @@ impl Cell { .map(Rc::clone) } - /// Get a reference to the first port that has the attribute `attr`. - pub fn find_with_attr(&self, attr: A) -> Option> + /// Return all ports that have the attribute `attr`. + pub fn find_all_with_attr( + &self, + attr: A, + ) -> impl Iterator> + '_ where A: Into, { let attr = attr.into(); self.ports .iter() - .find(|&g| g.borrow().attributes.has(attr)) + .filter(move |&p| p.borrow().attributes.has(attr)) .map(Rc::clone) } - /// Return all ports that have the attribute `attr`. - pub fn find_all_with_attr( + /// Return the unique port with the given attribute. + /// If multiple ports have the same attribute, then we panic. + /// If there are not ports with the give attribute, then we return None. + pub fn find_unique_with_attr( &self, attr: A, - ) -> impl Iterator> + '_ + ) -> CalyxResult>> where A: Into, { let attr = attr.into(); - self.ports - .iter() - .filter(move |&p| p.borrow().attributes.has(attr)) - .map(Rc::clone) + let mut ports = self.find_all_with_attr(attr); + if let Some(port) = ports.next() { + if ports.next().is_some() { + Err(Error::malformed_structure(format!( + "Multiple ports with attribute `{}` found on cell `{}`", + attr, self.name + ))) + } else { + Ok(Some(port)) + } + } else { + Ok(None) + } } /// Get a reference to the named port and throw an error if it doesn't @@ -362,18 +376,18 @@ impl Cell { } } - /// Get a reference to the first port with the attribute `attr` and throw an error if none - /// exist. - pub fn get_with_attr(&self, attr: A) -> RRC + /// Get the unique port with the given attribute. + /// Panic if no port with the attribute is found and returns an error if multiple ports with the attribute are found. + pub fn get_unique_with_attr(&self, attr: A) -> CalyxResult> where A: Into + std::fmt::Display + Copy, { - self.find_with_attr(attr).unwrap_or_else(|| { + Ok(self.find_unique_with_attr(attr)?.unwrap_or_else(|| { panic!( "Port with attribute `{attr}' not found on cell `{}'", self.name, ) - }) + })) } /// Returns the name of the component that is this cells type. diff --git a/calyx-opt/src/analysis/variable_detection.rs b/calyx-opt/src/analysis/variable_detection.rs index a965eb6b67..46535f5a33 100644 --- a/calyx-opt/src/analysis/variable_detection.rs +++ b/calyx-opt/src/analysis/variable_detection.rs @@ -37,7 +37,8 @@ impl VariableDetection { // if guard is empty, because if it isn't this would show up as // a write let graph = GraphAnalysis::from(&*group); - let go_port = cell.find_with_attr(ir::NumAttr::Go)?; + let go_port = + cell.find_unique_with_attr(ir::NumAttr::Go).ok().flatten()?; let activation = graph .writes_to(&go_port.borrow()) .map(|src| src.borrow().is_constant(1, 1)) diff --git a/calyx-opt/src/passes/clk_insertion.rs b/calyx-opt/src/passes/clk_insertion.rs index e0ca0dbea6..28dd9b982c 100644 --- a/calyx-opt/src/passes/clk_insertion.rs +++ b/calyx-opt/src/passes/clk_insertion.rs @@ -32,12 +32,14 @@ impl Visitor for ClkInsertion { .component .signature .borrow() - .find_with_attr(ir::BoolAttr::Clk); + .find_unique_with_attr(ir::BoolAttr::Clk)?; if let Some(clk) = clk { for cell_ref in builder.component.cells.iter() { let cell = cell_ref.borrow(); - if let Some(port) = cell.find_with_attr(ir::BoolAttr::Clk) { + if let Some(port) = + cell.find_unique_with_attr(ir::BoolAttr::Clk)? + { builder.component.continuous_assignments.push( builder.build_assignment( port, @@ -50,7 +52,7 @@ impl Visitor for ClkInsertion { } else { for cell_ref in builder.component.cells.iter() { let cell = cell_ref.borrow(); - if cell.find_with_attr(ir::BoolAttr::Clk).is_some() { + if cell.find_unique_with_attr(ir::BoolAttr::Clk)?.is_some() { return Err(Error::malformed_structure(format!( "Cell `{}' in component `{}' has a clk port, \ but the component does not have a clk port.", diff --git a/calyx-opt/src/passes/compile_invoke.rs b/calyx-opt/src/passes/compile_invoke.rs index 157431b7e6..8cf3676819 100644 --- a/calyx-opt/src/passes/compile_invoke.rs +++ b/calyx-opt/src/passes/compile_invoke.rs @@ -13,14 +13,17 @@ fn get_go_port(cell_ref: ir::RRC) -> CalyxResult> { let name = cell.name(); // Get the go port - let mut go_ports = cell.find_all_with_attr(ir::NumAttr::Go).collect_vec(); - if go_ports.len() > 1 { - return Err(Error::malformed_control(format!("Invoked component `{name}` defines multiple @go signals. Cannot compile the invoke"))); - } else if go_ports.is_empty() { - return Err(Error::malformed_control(format!("Invoked component `{name}` does not define a @go signal. Cannot compile the invoke"))); + match cell.find_unique_with_attr(ir::NumAttr::Go) { + Ok(Some(p)) => Ok(p), + Ok(None) => Err(Error::malformed_control(format!( + "Invoked component `{name}` does not define a @go signal. Cannot compile the invoke", + ))), + Err(_) => { + Err(Error::malformed_control(format!( + "Invoked component `{name}` defines multiple @go signals. Cannot compile the invoke", + ))) + } } - - Ok(go_ports.pop().unwrap()) } // given inputs and outputs (of the invoke), and the `enable_assignments` (e.g., invoked_component.go = 1'd1) diff --git a/calyx-opt/src/passes/group_to_invoke.rs b/calyx-opt/src/passes/group_to_invoke.rs index 69161d7f30..441d847a31 100644 --- a/calyx-opt/src/passes/group_to_invoke.rs +++ b/calyx-opt/src/passes/group_to_invoke.rs @@ -105,7 +105,8 @@ fn construct_invoke( // inputs. we can ignore the cell.go assignment, since that is not // going to be part of the `invoke`. else if parent_is_cell(&assign.dst.borrow()) - && assign.dst != comp.borrow().get_with_attr(ir::NumAttr::Go) + && assign.dst + != comp.borrow().get_unique_with_attr(ir::NumAttr::Go).unwrap() { let name = assign.dst.borrow().name; if assign.guard.is_true() { @@ -264,22 +265,14 @@ impl GroupToInvoke { return; } - // Component must define a @go/@done interface - let maybe_go_port = cell.find_with_attr(ir::NumAttr::Go); - let maybe_done_port = cell.find_with_attr(ir::NumAttr::Done); - if maybe_go_port.is_none() || maybe_done_port.is_none() { + // Component must define exactly one @go/@done interface + let Ok(Some(go_port)) = cell.find_unique_with_attr(ir::NumAttr::Go) else { return; - } - - // Component must have a single @go/@done pair - let go_ports = cell.find_all_with_attr(ir::NumAttr::Go).count(); - let done_ports = cell.find_all_with_attr(ir::NumAttr::Done).count(); - if go_ports > 1 || done_ports > 1 { + }; + let Ok(Some(done_port)) = cell.find_unique_with_attr(ir::NumAttr::Done) else { return; - } + }; - let go_port = maybe_go_port.unwrap(); - let done_port = maybe_done_port.unwrap(); let mut go_wr_cnt = 0; let mut done_wr_cnt = 0; @@ -288,9 +281,9 @@ impl GroupToInvoke { if assign.dst == go_port { if go_wr_cnt > 0 { log::info!( - "Cannot transform `{}` due to multiple writes to @go port", - group_name, - ); + "Cannot transform `{}` due to multiple writes to @go port", + group_name, + ); return; } else if !assign.guard.is_true() { log::info!( diff --git a/calyx-opt/src/passes/hole_inliner.rs b/calyx-opt/src/passes/hole_inliner.rs index 75b32d5ac7..b453cd2ba4 100644 --- a/calyx-opt/src/passes/hole_inliner.rs +++ b/calyx-opt/src/passes/hole_inliner.rs @@ -123,11 +123,11 @@ impl Visitor for HoleInliner { let mut asgns = vec![ builder.build_assignment( top_level.borrow().get("go"), - this_comp.borrow().get_with_attr(ir::NumAttr::Go), + this_comp.borrow().get_unique_with_attr(ir::NumAttr::Go)?, ir::Guard::True, ), builder.build_assignment( - this_comp.borrow().get_with_attr(ir::NumAttr::Done), + this_comp.borrow().get_unique_with_attr(ir::NumAttr::Done)?, top_level.borrow().get("done"), ir::Guard::True, ), diff --git a/calyx-opt/src/passes/reset_insertion.rs b/calyx-opt/src/passes/reset_insertion.rs index ef105404dd..cfe9223f81 100644 --- a/calyx-opt/src/passes/reset_insertion.rs +++ b/calyx-opt/src/passes/reset_insertion.rs @@ -30,12 +30,14 @@ impl Visitor for ResetInsertion { .component .signature .borrow() - .find_with_attr(ir::BoolAttr::Reset); + .find_unique_with_attr(ir::BoolAttr::Reset)?; if let Some(reset) = reset { for cell_ref in builder.component.cells.iter() { let cell = cell_ref.borrow(); - if let Some(port) = cell.find_with_attr(ir::BoolAttr::Reset) { + if let Some(port) = + cell.find_unique_with_attr(ir::BoolAttr::Reset)? + { builder.component.continuous_assignments.push( builder.build_assignment( port, @@ -48,7 +50,7 @@ impl Visitor for ResetInsertion { } else { for cell_ref in builder.component.cells.iter() { let cell = cell_ref.borrow(); - if cell.find_with_attr(ir::BoolAttr::Reset).is_some() { + if cell.find_unique_with_attr(ir::BoolAttr::Reset)?.is_some() { return Err(Error::malformed_structure(format!( "Cell `{}' in component `{}' has a reset port, \ but the component does not have a reset port.", diff --git a/interp/src/interpreter/control_interpreter.rs b/interp/src/interpreter/control_interpreter.rs index db406bea19..75d75b792a 100644 --- a/interp/src/interpreter/control_interpreter.rs +++ b/interp/src/interpreter/control_interpreter.rs @@ -1042,12 +1042,13 @@ impl InvokeInterpreter { assignment_vec.extend(w_ref.assignments.iter().cloned()); } - let go_port = comp_cell.get_with_attr(ir::NumAttr::Go); + let go_port = comp_cell.get_unique_with_attr(ir::NumAttr::Go).unwrap(); // insert one into the go_port // should probably replace with an actual assignment from a constant one env.insert(go_port, Value::bit_high()); - let comp_done_port = comp_cell.get_with_attr(ir::NumAttr::Done); + let comp_done_port = + comp_cell.get_unique_with_attr(ir::NumAttr::Done).unwrap(); let interp = AssignmentInterpreter::new( env, comp_done_port.into(), @@ -1078,7 +1079,12 @@ impl Interpreter for InvokeInterpreter { let mut env = self.assign_interp.reset()?; // set go low - let go_port = self.invoke.comp.borrow().get_with_attr(ir::NumAttr::Go); + let go_port = self + .invoke + .comp + .borrow() + .get_unique_with_attr(ir::NumAttr::Go) + .unwrap(); // insert one into the go_port // should probably replace with an actual assignment from a constant one env.insert(go_port, Value::bit_low()); @@ -1238,7 +1244,8 @@ impl StructuralInterpreter { env: InterpreterState, ) -> Self { let comp_sig = comp.signature.borrow(); - let done_port = comp_sig.get_with_attr(ir::NumAttr::Done); + let done_port = + comp_sig.get_unique_with_attr(ir::NumAttr::Done).unwrap(); let done_raw = done_port.as_raw(); let continuous = Rc::clone(&comp.continuous_assignments); let assigns: Vec> = vec![];