Mem width fix (#1096)

* Create multipe toplevel brams from mem parmeters

Previously, a single bram module was instantiated and used for
all axi memory controllers. Now, differing size brams are created
based on the calyx program arguments for each memory

* Replace mapping of .clone() to cloned()

* Change bram_logic to work with multiple brams

Previously, a single bram was used for every memory controller. As a
result, when bram_logic didn't properly interface with previous changes
made which give each memory controller their own custom bram. These
changes address that issue. Now, each memory controller should properly
interface with it's own bram.

* Combine 3 memory methods into get_mem_info

* Change bram module to be instantiaed by name

Previously, bram was instantiated by index. Now it assumes that
the name passed to it is of form ""Memory_controller_axi_<suffix>" and
correctly numbers bram instances accordingly.

* Remove most hardcoded width values

* Cleaned up comments

Co-authored-by: Nathaniel Navarro <[email protected]>

src/backend/xilinx/memory_axi.rs

@@ -20,6 +20,8 @@ pub trait MemoryInterface {
         bus_data_width: u64,
         bus_addr_width: u64,
         data_width: u64,
+        memory_size: u64,
+        addr_width: u64,
     ) -> v::Module;
 }
 
@@ -98,11 +100,11 @@ impl MemoryInterface for AxiInterface {
         name: &str,
         bus_data_width: u64,
         bus_addr_width: u64,
-        // address_width: u64,
         data_width: u64,
+        memory_size: u64,
+        addr_width: u64,
     ) -> v::Module {
         let mut module = v::Module::new(name);
-        let memory_size = 32;
         let memory_size_bits: u64 = utils::math::bits_needed_for(memory_size); // TODO make memory size parametric
 
         module.add_input("ACLK", 1);
@@ -121,7 +123,7 @@ impl MemoryInterface for AxiInterface {
         // BRAM interface
         module.add_input("WRITE_DATA", data_width);
         module.add_output("READ_DATA", data_width);
-        module.add_input("ADDR", memory_size_bits);
+        module.add_input("ADDR", addr_width);
         module.add_input("WE", 1);
         module.add_output("DONE", 1);
 
@@ -162,12 +164,21 @@ impl MemoryInterface for AxiInterface {
         ));
 
         // bram reading / writing logic
-        bram_logic(&axi4, &mut module, &mode_fsm, "read_txn_count".into());
+        bram_logic(
+            name,
+            &axi4,
+            &mut module,
+            &mode_fsm,
+            "read_txn_count".into(),
+            data_width,
+            addr_width,
+        );
         module.add_stmt(v::Parallel::Assign(
             "READ_DATA".into(),
             "bram_read_data".into(),
         ));
 
+        //TODO(nathanielnrn) change this to use addr_width? In `x_done` assignment fix
         let offset_size_bits = memory_size_bits + 1;
 
         // synchronise channels
@@ -189,6 +200,8 @@ impl MemoryInterface for AxiInterface {
 
         // addresses are byte addressed which means addresses are computed as
         // base + (offset << shift_by)
+        // TODO(nathanielnrn): Fix the burst size and shifting values based on
+        // pynq(?) input? Or memory size? unclear.
         let shift_by = 2;
         let burst_size: i32 = utils::math::bits_needed_for(32 / 8) as i32;
 
@@ -233,7 +246,7 @@ impl MemoryInterface for AxiInterface {
         );
 
         module.add_stmt(axi4.write_address.assign("ID", 0));
-        // assign shift to a wire to circumvent vast order of operations issues
+        // assign shift to a wire to circumvent `vast` order of operations issues
         let send_shift = "send_shift";
         module.add_decl(v::Decl::new_wire(send_shift, bus_addr_width));
         let mut concat = v::ExprConcat::default();
@@ -293,18 +306,23 @@ fn module_mode_fsm(module: &mut v::Module) -> fsm::LinearFsm {
 }
 
 fn bram_logic(
+    name: &str, //assumed to be of form [Memory_controller_axi_<suffix>]
     axi4: &AxiInterface,
     module: &mut v::Module,
     mode_fsm: &fsm::LinearFsm,
     txn_count: v::Expr,
+    data_width: u64,
+    addr_width: u64,
 ) {
-    module.add_decl(v::Decl::new_wire("bram_addr", 5));
-    module.add_decl(v::Decl::new_wire("bram_write_data", 32));
+    module.add_decl(v::Decl::new_wire("bram_addr", addr_width));
+    module.add_decl(v::Decl::new_wire("bram_write_data", data_width));
     module.add_decl(v::Decl::new_wire("bram_we", 1));
-    module.add_decl(v::Decl::new_wire("bram_read_data", 32));
+    module.add_decl(v::Decl::new_wire("bram_read_data", data_width));
     module.add_decl(v::Decl::new_wire("bram_done", 1));
-
-    let mut ram_instance = v::Instance::new("bram", "SINGLE_PORT_BRAM");
+    let suffix_idx = "Memory_controller_axi_".len();
+    let suffix = &name[suffix_idx..];
+    let mut ram_instance =
+        v::Instance::new("bram", &format!("SINGLE_PORT_BRAM_{}", suffix));
     ram_instance.connect_ref("ACLK", "ACLK");
     ram_instance.connect_ref("ADDR", "bram_addr");
     ram_instance.connect_ref("Din", "bram_write_data");
@@ -315,9 +333,9 @@ fn bram_logic(
     module.add_stmt(v::Parallel::Assign("DONE".into(), "bram_done".into()));
 
     // bram address logic
-    let copy_address = v::Expr::new_slice("copy_addr_offset", 4, 0);
+    let copy_address = v::Expr::new_slice("copy_addr_offset", 4, 0); // XXX(nathanielnrn) does this need to use parameterization?
     let bram_address: v::Expr = "ADDR".into();
-    let send_address = v::Expr::new_slice("send_addr_offset", 4, 0);
+    let send_address = v::Expr::new_slice("send_addr_offset", 4, 0); // same here
     let mux_address = v::Expr::new_mux(
         v::Expr::new_logical_and(
             axi4.read_data.handshake(),
@@ -349,7 +367,7 @@ fn bram_logic(
     let copy_data: v::Expr = v::Expr::new_index_slice(
         &axi4.read_data.get("DATA"),
         v::Expr::new_mul(txn_count, 32),
-        32, /* bram data width */
+        data_width as u32, // bram data width
     );
     let bram_data: v::Expr = "WRITE_DATA".into();
     let mux_data = v::Expr::new_mux(
@@ -387,8 +405,13 @@ fn incr_addr(
     module.add_stmt(always);
 }
 
-pub fn bram(data_width: u64, size: u64, addr_width: u64) -> v::Module {
-    let mut module = v::Module::new("SINGLE_PORT_BRAM");
+pub fn bram(
+    name: &str,
+    data_width: u64,
+    size: u64,
+    addr_width: u64,
+) -> v::Module {
+    let mut module = v::Module::new(name);
     module.add_input("ACLK", 1);
     module.add_input("ADDR", addr_width);
     module.add_input("Din", data_width);
@@ -420,7 +443,7 @@ pub fn bram(data_width: u64, size: u64, addr_width: u64) -> v::Module {
         "Dout".into(),
         v::Expr::new_index_expr("ram_core", "ADDR"),
     ));
-    //add a simple assign <String1> = <String2>
+    //add a simple assign Done = done_reg
     module.add_stmt(v::Parallel::Assign("Done".into(), "done_reg".into()));
     module
 }

src/backend/xilinx/toplevel.rs

@@ -46,18 +46,37 @@ impl Backend for XilinxInterfaceBackend {
                     " Please make sure that at least one memory is marked as @external."));
         }
 
+        let mem_info = get_mem_info(toplevel);
+
         let mut modules = vec![
+            // XXX(nathanielnrn) what defines top_level address_width and data_width?
             top_level(12, 32, &memories),
-            bram(32, 32, 5),
-            axi::AxiInterface::control_module("Control_axi", 12, 32, &memories),
         ];
+        for (i, _mem) in memories.iter().enumerate() {
+            modules.push(bram(
+                &format!("SINGLE_PORT_BRAM_{}", i),
+                mem_info[i].0,
+                mem_info[i].1,
+                mem_info[i].2,
+            ))
+        }
+
+        modules.push(axi::AxiInterface::control_module(
+            "Control_axi",
+            // XXX(nathanielnrn) these match numbers above, unclear where they're from
+            12,
+            32,
+            &memories,
+        ));
 
         for (i, _mem) in memories.iter().enumerate() {
             modules.push(axi::AxiInterface::memory_module(
                 &format!("Memory_controller_axi_{}", i),
                 512,
                 64,
-                32,
+                mem_info[i].0,
+                mem_info[i].1,
+                mem_info[i].2,
             ))
         }
 
@@ -79,13 +98,36 @@ impl Backend for XilinxInterfaceBackend {
     }
 }
 
-fn external_memories(comp: &ir::Component) -> Vec<String> {
-    // find external memories
+fn external_memories_cells(
+    comp: &ir::Component,
+) -> Vec<calyx::ir::RRC<ir::Cell>> {
     comp.cells
         .iter()
-        .filter(|cell_ref| {
-            matches!(cell_ref.borrow().get_attribute("external"), Some(&1))
+        // find external memories
+        .filter(|cell_ref| cell_ref.borrow().attributes.has("external"))
+        .cloned()
+        .collect()
+}
+
+// Returns a vector of tuples containing external memory info of form:
+// (WIDTH, SIZE, IDX_SIZE)
+fn get_mem_info(comp: &ir::Component) -> Vec<(u64, u64, u64)> {
+    external_memories_cells(comp)
+        .iter()
+        .map(|cell_ref| {
+            (
+                cell_ref.borrow().get_parameter("WIDTH").unwrap(),
+                cell_ref.borrow().get_parameter("SIZE").unwrap(),
+                cell_ref.borrow().get_parameter("IDX_SIZE").unwrap(),
+            )
         })
+        .collect()
+}
+
+// Returns Vec<String> of memory names
+fn external_memories(comp: &ir::Component) -> Vec<String> {
+    external_memories_cells(comp)
+        .iter()
         .map(|cell_ref| cell_ref.borrow().name().to_string())
         .collect()
 }
@@ -171,7 +213,7 @@ fn top_level(
         let done = format!("{}_done", mem);
         module.add_decl(v::Decl::new_wire(&write_data, data_width));
         module.add_decl(v::Decl::new_wire(&read_data, data_width));
-        module.add_decl(v::Decl::new_wire(&addr0, 5));
+        module.add_decl(v::Decl::new_wire(&addr0, address_width));
         module.add_decl(v::Decl::new_wire(&write_en, 1));
         module.add_decl(v::Decl::new_wire(&done, 1));