From 53f6723feb3139b7e769c685d1a67c436a4e8958 Mon Sep 17 00:00:00 2001 From: bcarlet <8906114+bcarlet@users.noreply.github.com> Date: Tue, 31 Oct 2023 17:10:40 -0400 Subject: [PATCH] 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 | 24 ++----- calyx-opt/src/default_passes.rs | 5 +- calyx-opt/src/passes/dead_cell_removal.rs | 56 ++++++++++++---- tests/passes/cell-share/empty-invoke.expect | 58 ++++++++++++++++ tests/passes/cell-share/empty-invoke.futil | 33 ++++++++++ tests/passes/cell-share/inline.expect | 66 +++++++++++++++++++ tests/passes/cell-share/inline.futil | 32 +++++++++ tests/passes/cell-share/ref-share.expect | 2 + 8 files changed, 244 insertions(+), 32 deletions(-) create mode 100644 tests/passes/cell-share/empty-invoke.expect create mode 100644 tests/passes/cell-share/empty-invoke.futil create mode 100644 tests/passes/cell-share/inline.expect create mode 100644 tests/passes/cell-share/inline.futil diff --git a/calyx-opt/src/analysis/live_range_analysis.rs b/calyx-opt/src/analysis/live_range_analysis.rs index c1552ebc82..76b4f2c952 100644 --- a/calyx-opt/src/analysis/live_range_analysis.rs +++ b/calyx-opt/src/analysis/live_range_analysis.rs @@ -853,8 +853,8 @@ impl LiveRangeAnalysis { comp: &ir::RRC, 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)| { @@ -862,21 +862,18 @@ impl LiveRangeAnalysis { }) .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( diff --git a/calyx-opt/src/default_passes.rs b/calyx-opt/src/default_passes.rs index 72f1062d84..940d75fe3a 100644 --- a/calyx-opt/src/default_passes.rs +++ b/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, diff --git a/calyx-opt/src/passes/dead_cell_removal.rs b/calyx-opt/src/passes/dead_cell_removal.rs index f4ceb60e2f..ceefddb94e 100644 --- a/calyx-opt/src/passes/dead_cell_removal.rs +++ b/calyx-opt/src/passes/dead_cell_removal.rs @@ -48,6 +48,23 @@ impl DeadCellRemoval { out } } + + fn visit_invoke( + &mut self, + comp: &ir::RRC, + inputs: &[(ir::Id, ir::RRC)], + outputs: &[(ir::Id, ir::RRC)], + ref_cells: &[(ir::Id, ir::RRC)], + ) { + 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()), ); diff --git a/tests/passes/cell-share/empty-invoke.expect b/tests/passes/cell-share/empty-invoke.expect new file mode 100644 index 0000000000..98918ce5a4 --- /dev/null +++ b/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; + } + } +} diff --git a/tests/passes/cell-share/empty-invoke.futil b/tests/passes/cell-share/empty-invoke.futil new file mode 100644 index 0000000000..604f394077 --- /dev/null +++ b/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)(); + } + } +} diff --git a/tests/passes/cell-share/inline.expect b/tests/passes/cell-share/inline.expect new file mode 100644 index 0000000000..9a7e2e66b4 --- /dev/null +++ b/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; + } + } + } +} diff --git a/tests/passes/cell-share/inline.futil b/tests/passes/cell-share/inline.futil new file mode 100644 index 0000000000..f3ce491934 --- /dev/null +++ b/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)(); + } + } +} diff --git a/tests/passes/cell-share/ref-share.expect b/tests/passes/cell-share/ref-share.expect index fcf531a2cb..055c6293d4 100644 --- a/tests/passes/cell-share/ref-share.expect +++ b/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 {