Share cells in more situations (#1753)

* Sharing fixes

* `is_some_and` not stable in 1.67

* Suggested changes

* Treat all invokes as writes

* Test empty invokes

calyx-opt/src/analysis/live_range_analysis.rs

@@ -853,30 +853,27 @@ impl LiveRangeAnalysis {
         comp: &ir::RRC<ir::Cell>,
         shareable_components: &ShareSet,
     ) -> (TypeNameSet, TypeNameSet) {
-        // The writes of the invoke include its outputs. Also, if the input to the invoke
-        // is not empty, we also count the cell being invoked as being written to.
+        // The writes of the invoke include its outputs. Also, we count the cell
+        // being invoked as being written to.
         let mut write_set: TypeNameSet = outputs
             .iter()
             .filter_map(|(_, src)| {
                 Self::port_to_cell_name(src, shareable_components)
             })
             .collect();
 
-        let comp_is_written = !inputs.is_empty()
-            && shareable_components.is_shareable_component(comp);
-        if comp_is_written {
+        if shareable_components.is_shareable_component(comp) {
             write_set.insert((
                 comp.borrow().prototype.clone(),
                 comp.borrow().name(),
             ));
         }
 
-        // The reads of the invoke include its inputs. Also, if the outputs are
-        // not empty, the cell being invoked will be considered as being read from.
-        // One quick note: if the component is written to, there is no need to include this
+        // The reads of the invoke include its inputs.
+        // One quick note: since the component is written to, there is no need to include this
         // component as being read from since we know the write to the component
         // precedes the read from it, due to the nature of `invoke` statements.
-        // This is "cheating" in a sense, since the componenet is technically being
+        // This is "cheating" in a sense, since the component is technically being
         // read from. However, since we know that there is a write to the component
         // that that precedes the read from it within the very same invoke statement,
         // it "appears" to all the other control statements in the program that the
@@ -887,15 +884,6 @@ impl LiveRangeAnalysis {
                 Self::port_to_cell_name(src, shareable_components)
             })
             .collect();
-        if !outputs.is_empty()
-            && !comp_is_written
-            && shareable_components.is_shareable_component(comp)
-        {
-            read_set.insert((
-                comp.borrow().prototype.clone(),
-                comp.borrow().name(),
-            ));
-        }
 
         if let Some(comb_group) = comb_group_info {
             read_set.extend(

calyx-opt/src/default_passes.rs

@@ -89,9 +89,10 @@ impl PassManager {
                 InferShare,
                 ComponentInliner,
                 CombProp,
-                CellShare, // LiveRangeAnalaysis should handle comb groups
+                DeadCellRemoval, // Clean up dead wires left by CombProp
+                CellShare,       // LiveRangeAnalaysis should handle comb groups
                 SimplifyWithControl, // Must run before compile-invoke
-                CompileInvoke, // creates dead comb groups
+                CompileInvoke,   // creates dead comb groups
                 AttributePromotion,
                 StaticPromotion,
                 ScheduleCompaction,

calyx-opt/src/passes/dead_cell_removal.rs

@@ -48,6 +48,23 @@ impl DeadCellRemoval {
             out
         }
     }
+
+    fn visit_invoke(
+        &mut self,
+        comp: &ir::RRC<ir::Cell>,
+        inputs: &[(ir::Id, ir::RRC<ir::Port>)],
+        outputs: &[(ir::Id, ir::RRC<ir::Port>)],
+        ref_cells: &[(ir::Id, ir::RRC<ir::Cell>)],
+    ) {
+        let cells = inputs
+            .iter()
+            .map(|(_, p)| p)
+            .chain(outputs.iter().map(|(_, p)| p))
+            .map(|p| p.borrow().get_parent_name())
+            .chain(iter::once(comp.borrow().name()))
+            .chain(ref_cells.iter().map(|(_, c)| c.borrow().name()));
+        self.all_reads.extend(cells);
+    }
 }
 
 impl Visitor for DeadCellRemoval {
@@ -62,6 +79,17 @@ impl Visitor for DeadCellRemoval {
         Ok(Action::Continue)
     }
 
+    fn start_static_if(
+        &mut self,
+        s: &mut ir::StaticIf,
+        _comp: &mut ir::Component,
+        _sigs: &ir::LibrarySignatures,
+        _comps: &[ir::Component],
+    ) -> VisResult {
+        self.all_reads.insert(s.port.borrow().get_parent_name());
+        Ok(Action::Continue)
+    }
+
     fn start_while(
         &mut self,
         s: &mut ir::While,
@@ -80,16 +108,18 @@ impl Visitor for DeadCellRemoval {
         _sigs: &ir::LibrarySignatures,
         _comps: &[ir::Component],
     ) -> VisResult {
-        let ir::Invoke {
-            inputs, outputs, ..
-        } = s;
-        let cells = inputs
-            .iter()
-            .map(|(_, p)| p)
-            .chain(outputs.iter().map(|(_, p)| p))
-            .map(|p| p.borrow().get_parent_name())
-            .chain(iter::once(s.comp.borrow().name()));
-        self.all_reads.extend(cells);
+        self.visit_invoke(&s.comp, &s.inputs, &s.outputs, &s.ref_cells);
+        Ok(Action::Continue)
+    }
+
+    fn static_invoke(
+        &mut self,
+        s: &mut ir::StaticInvoke,
+        _comp: &mut ir::Component,
+        _sigs: &ir::LibrarySignatures,
+        _comps: &[ir::Component],
+    ) -> VisResult {
+        self.visit_invoke(&s.comp, &s.inputs, &s.outputs, &s.ref_cells);
         Ok(Action::Continue)
     }
 
@@ -99,12 +129,14 @@ impl Visitor for DeadCellRemoval {
         _sigs: &ir::LibrarySignatures,
         _comps: &[ir::Component],
     ) -> VisResult {
-        // Add @external cells.
+        // Add @external cells and ref cells.
         self.all_reads.extend(
             comp.cells
                 .iter()
                 .filter(|c| {
-                    c.borrow().attributes.get(ir::BoolAttr::External).is_some()
+                    let cell = c.borrow();
+                    cell.attributes.get(ir::BoolAttr::External).is_some()
+                        || cell.is_reference()
                 })
                 .map(|c| c.borrow().name()),
         );

tests/passes/cell-share/empty-invoke.expect

@@ -0,0 +1,58 @@
+import "primitives/core.futil";
+component write_one<"state_share"=1>(@go go: 1, @clk clk: 1, @reset reset: 1) -> (out: 32, @done done: 1) {
+  cells {
+    @data x = std_reg(32);
+  }
+  wires {
+    group invoke0<"promote_static"=1> {
+      x.write_en = 1'd1;
+      invoke0[done] = x.done;
+      x.in = 32'd1;
+    }
+    out = x.out;
+  }
+  control {
+    @promote_static invoke0;
+  }
+}
+component main(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {
+  cells {
+    @external @data mem = std_mem_d1(32, 2, 1);
+    @data x = write_one();
+  }
+  wires {
+    group invoke0 {
+      x.go = 1'd1;
+      invoke0[done] = x.done;
+    }
+    group invoke1 {
+      x.go = 1'd1;
+      invoke1[done] = x.done;
+    }
+    group invoke2<"promote_static"=1> {
+      mem.write_en = 1'd1;
+      invoke2[done] = mem.done;
+      mem.addr0 = 1'd0;
+      mem.write_data = x.out;
+    }
+    group invoke3 {
+      x.go = 1'd1;
+      invoke3[done] = x.done;
+    }
+    group invoke4<"promote_static"=1> {
+      mem.write_en = 1'd1;
+      invoke4[done] = mem.done;
+      mem.addr0 = 1'd1;
+      mem.write_data = x.out;
+    }
+  }
+  control {
+    seq {
+      invoke0;
+      invoke1;
+      @promote_static invoke2;
+      invoke3;
+      @promote_static invoke4;
+    }
+  }
+}

tests/passes/cell-share/empty-invoke.futil

@@ -0,0 +1,33 @@
+// -p pre-opt -p post-opt
+import "primitives/core.futil";
+
+component write_one() -> (out: 32) {
+  cells {
+    x = std_reg(32);
+  }
+  wires {
+    out = x.out;
+  }
+  control {
+    invoke x(in = 32'd1)();
+  }
+}
+
+component main() -> () {
+  cells {
+    @external mem = std_mem_d1(32, 2, 1);
+    // these should be shared
+    x = write_one();
+    y = write_one();
+  }
+  wires {}
+  control {
+    seq {
+      invoke x()();
+      invoke y()(); // x is dead here
+      invoke mem(addr0 = 1'd0, write_data = y.out)();
+      invoke x()();
+      invoke mem(addr0 = 1'd1, write_data = x.out)();
+    }
+  }
+}

tests/passes/cell-share/inline.expect

@@ -0,0 +1,66 @@
+import "primitives/core.futil";
+component my_reg<"state_share"=1>(@data in: 32, @go go: 1, @clk clk: 1, @reset reset: 1) -> (@stable out: 32, @done done: 1) {
+  cells {
+    @data r = std_reg(32);
+  }
+  wires {
+    group invoke0<"promote_static"=1> {
+      r.write_en = 1'd1;
+      invoke0[done] = r.done;
+      r.in = in;
+    }
+    out = r.out;
+  }
+  control {
+    @promote_static invoke0;
+  }
+}
+static<4> component main(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {
+  cells {
+    @external @data mem = std_mem_d1(32, 2, 1);
+    @generated r = std_reg(32);
+  }
+  wires {
+    static<1> group invoke00 {
+      r.write_en = 1'd1;
+      r.in = 32'd0;
+    }
+    static<1> group invoke10 {
+      mem.write_en = 1'd1;
+      mem.addr0 = 1'd0;
+      mem.write_data = r.out;
+    }
+    static<1> group invoke20 {
+      r.write_en = 1'd1;
+      r.in = 32'd1;
+    }
+    static<1> group invoke30 {
+      mem.write_en = 1'd1;
+      mem.addr0 = 1'd1;
+      mem.write_data = r.out;
+    }
+    static<1> group no-op {
+    }
+    static<2> group no-op0 {
+    }
+    static<3> group no-op1 {
+    }
+  }
+  control {
+    static<4> par {
+      invoke00;
+      static<2> seq  {
+        no-op;
+        invoke10;
+      }
+      static<3> seq  {
+        no-op0;
+        invoke20;
+      }
+      static<4> seq  {
+        no-op1;
+        invoke30;
+      }
+    }
+  }
+}

tests/passes/cell-share/inline.futil

@@ -0,0 +1,32 @@
+// -p pre-opt -p post-opt
+import "primitives/core.futil";
+
+component my_reg<"state_share"=1>(@data in: 32) -> (@stable out: 32) {
+  cells {
+    r = std_reg(32);
+  }
+  wires {
+    out = r.out;
+  }
+  control {
+    invoke r(in = in)();
+  }
+}
+
+component main() -> () {
+  cells {
+    @external mem = std_mem_d1(32, 2, 1);
+    // the two `std_reg` created after inlining should be shared
+    @inline r0 = my_reg();
+    @inline r1 = my_reg();
+  }
+  wires {}
+  control {
+    seq {
+      invoke r0(in = 32'd0)();
+      invoke mem(addr0 = 1'd0, write_data = r0.out)();
+      invoke r1(in = 32'd1)();
+      invoke mem(addr0 = 1'd1, write_data = r1.out)();
+    }
+  }
+}

tests/passes/cell-share/ref-share.expect

@@ -26,6 +26,8 @@ component foo(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {
 component main(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {
   cells {
     f = foo();
+    r1 = std_reg(32);
+    r2 = std_reg(32);
   }
   wires {}
   control {