Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mem width fix #1096

Merged
merged 10 commits into from
Jul 14, 2022
Merged

Mem width fix #1096

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2ec5381
Create multipe toplevel brams from mem parmeters
nathanielnrn Jul 13, 2022
f33f0a8
Merge branch 'master' into mem-width-fix
nathanielnrn Jul 13, 2022
0ac12c4
Replace mapping of .clone() to cloned()
nathanielnrn Jul 13, 2022
15cc8d6
Change bram_logic to work with multiple brams
nathanielnrn Jul 13, 2022
914829a
Combine 3 memory methods into get_mem_info
nathanielnrn Jul 13, 2022
e46d4d0
Change bram module to be instantiaed by name
nathanielnrn Jul 13, 2022
3d29f9e
Remove most hardcoded width values
nathanielnrn Jul 13, 2022
8054847
Merge branch 'master' into mem-width-fix
nathanielnrn Jul 13, 2022
4686692
Cleaned up comments
nathanielnrn Jul 14, 2022
75130f2
Merge branch 'master' into mem-width-fix
nathanielnrn Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions src/backend/xilinx/memory_axi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -100,9 +102,10 @@ impl MemoryInterface for AxiInterface {
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


module.add_input("ACLK", 1);
Expand All @@ -121,7 +124,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);

Expand Down Expand Up @@ -162,13 +165,22 @@ impl MemoryInterface for AxiInterface {
));

// bram reading / writing logic
bram_logic(&axi4, &mut module, &mode_fsm, "read_txn_count".into());
// TODO(nathanielnrn) fix this
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
bram_logic(
name, // 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(),
));

let offset_size_bits = memory_size_bits + 1;
let offset_size_bits = memory_size_bits + 1; //TODO(nathanielnrn) change this to use width?

// synchronise channels
let read_controller = axi4
Expand All @@ -189,6 +201,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;

Expand Down Expand Up @@ -293,18 +307,24 @@ 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..];
Comment on lines +322 to +323
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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");
Expand All @@ -315,9 +335,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(),
Expand Down Expand Up @@ -349,7 +369,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.try_into().unwrap(), /* bram data width */
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
);
let bram_data: v::Expr = "WRITE_DATA".into();
let mux_data = v::Expr::new_mux(
Expand Down Expand Up @@ -387,8 +407,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);
Expand Down
61 changes: 53 additions & 8 deletions src/backend/xilinx/toplevel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
))
}

Expand All @@ -79,13 +98,39 @@ impl Backend for XilinxInterfaceBackend {
}
}

fn external_memories(comp: &ir::Component) -> Vec<String> {
// find external memories
// XXX(nathanielnrn) I __think__ this should always be in the same order because
// of IdList being in the same order
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
// also, should this be a macro?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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()
}
Expand Down Expand Up @@ -171,7 +216,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));

Expand Down