Mem width fix #1096

nathanielnrn · 2022-07-13T16:00:16Z

Creating a draft PR to get feedback from @rachitnigam and others.

The first part of the fix (up to commit "replace mapping of .clone() to cloned()") creates multiple bram modules based on the size of memories in a calyx program.

Had to wrangle a lot with this helper function (copied below) due to rust typing and ownership things. Still not completely happy with how this came out.

fn external_memories_cells(
    comp: &ir::Component,
) -> Vec<std::rc::Rc<std::cell::RefCell<ir::Cell>>> {
    comp.cells
        .iter()
        .filter(|cell_ref| {
            matches!(cell_ref.borrow().get_attribute("external"), Some(&1))
        })
        .map(|cell_ref| cell_ref.clone())
        .collect()
}

Would particularly appreciate feedback on:

The creation of separate modules for each memory, specifically, should we look into sharing identically sized memories/doing this some other way?
Creating the separate modules by iteration through these vectors
If the helper function can be simplified and improved

Of course any other feedback is always appreciated

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

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.

src/backend/xilinx/memory_axi.rs

src/backend/xilinx/toplevel.rs

rachitnigam · 2022-07-13T18:46:26Z

To answer your questions:

No. Let's get the simplest working thing done first so we can start testing it. I'd like us to add a new runt test that checks the generated output of the AXI generator once we are done with Issues with the AXI implementation #1071.
The link doesn't work for me so I'm not sure which vectors
Left some inline comments in the code for this

src/backend/xilinx/memory_axi.rs

rachitnigam · 2022-07-13T18:48:49Z

There seem to be a couple of other places where 32 is hardcoded: https://github.com/cucapra/calyx/pull/1096/files#diff-ec4d7ca2c558f4a7eed43fa73ae42ddce8596bda9076d8dea7facd1006e46d7bR368

Are those safe or do they need the data width values?

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.

nathanielnrn · 2022-07-13T22:38:14Z

Addressed all of the comments, got rid of almost all of the hardcoded values except the ones marked with xxxs or todos. If @rachitnigam you happen to know how they should behave or where they are from at a glance I will fix these otherwise I will look at them more tomorrow. Think it's ready to be reviewed and hopefully merged so moving it out of draft status.

src/backend/xilinx/memory_axi.rs

src/backend/xilinx/toplevel.rs

rachitnigam · 2022-07-14T16:33:12Z

src/backend/xilinx/toplevel.rs

-    // find external memories
+// XXX(nathanielnrn) I __think__ this should always be in the same order because
+// of IdList being in the same order
+// also, should this be a macro?


What should be a macro? This function? Is so why?

Initially I was wondering if the helper function should be a macro due to some trouble I was having implementing it. But if the helper function looks ok:

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

And the way it is used in these two functions is fine:

// 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() }

Then it should be fine to leave it as is.

rachitnigam

A few comments here and there but overall looks good. Regarding correctness, we’ll only know if it works when you run it with hw_emu or on an FPGA

nathanielnrn · 2022-07-14T18:09:37Z

If you're happy with @rachitnigam this then I will merge

rachitnigam · 2022-07-14T18:58:13Z

Yeah, do it!

sampsyo

Super nice work, y'all. Very cool to see the memory parameters extracted and then propagated all the way through the AXI generation. Just wanted to read through so I understand how this all works; you can probably just ignore my two scant comments.

sampsyo · 2022-07-14T22:36:34Z

src/backend/xilinx/memory_axi.rs

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


Hopefully memory_size_bits == addr_width… maybe adding an assert here to check that would be cool?

This is actually slightly changed in #1103.
One thing that occurred to me on the subject is that because calyx-memory has the width of it's address port defined through an argument, there could be a (perhaps strange? Not sure how common this would be) case where there is a wider address port than needed for the size of a memory. In that case the two wouldn't be equal.

Ah, that makes sense; thanks for clarifying! Yeah, it's technically true that the Calyx memory primitives could declare a larger address space than they actually need. It would be weird, but it would be a mistake to assert that it's impossible.

It's mostly a consequence of not being able to specify things like ADDR0: $clog2(SIZE) in the parameters of a calyx primitive

sampsyo · 2022-07-14T22:38:30Z

src/backend/xilinx/memory_axi.rs

+    let suffix_idx = "Memory_controller_axi_".len();
+    let suffix = &name[suffix_idx..];


Not a big deal, but it could be ever so slightly nicer to take suffix in specifically as a parameter?

nathanielnrn added 4 commits July 13, 2022 11:49

Create multipe toplevel brams from mem parmeters

2ec5381

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

Merge branch 'master' into mem-width-fix

f33f0a8

Replace mapping of .clone() to cloned()

0ac12c4