Deprecate Cell::find_with_attr (#1690)

* Deprecate `find_with_attr` in favor of `find_with_unique_attr`

* update changelog

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`.
 

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<A>(&self, attr: A) -> Option<RRC<Port>>
+    /// Return all ports that have the attribute `attr`.
+    pub fn find_all_with_attr<A>(
+        &self,
+        attr: A,
+    ) -> impl Iterator<Item = RRC<Port>> + '_
     where
         A: Into<Attribute>,
     {
         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<A>(
+    /// 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<A>(
         &self,
         attr: A,
-    ) -> impl Iterator<Item = RRC<Port>> + '_
+    ) -> CalyxResult<Option<RRC<Port>>>
     where
         A: Into<Attribute>,
     {
         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<A>(&self, attr: A) -> RRC<Port>
+    /// 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<A>(&self, attr: A) -> CalyxResult<RRC<Port>>
     where
         A: Into<Attribute> + 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.

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))

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.",

calyx-opt/src/passes/compile_invoke.rs

@@ -13,14 +13,17 @@ fn get_go_port(cell_ref: ir::RRC<ir::Cell>) -> CalyxResult<ir::RRC<ir::Port>> {
     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)

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!(

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,
             ),

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.",

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<ir::Assignment<ir::Nothing>> = vec![];