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 4 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
47 changes: 34 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 @@ -16,10 +16,12 @@ pub trait MemoryInterface {
prefix: &str,
) -> Self;
fn memory_module(
name: &str,
index: u64, //index of memory modules in toplevel
bus_data_width: u64,
bus_addr_width: u64,
data_width: u64,
memory_size: u64,
addr_width: u64,
) -> v::Module;
}

Expand Down Expand Up @@ -95,14 +97,16 @@ impl MemoryInterface for AxiInterface {
}

fn memory_module(
name: &str,
index: u64,
bus_data_width: u64,
bus_addr_width: u64,
// address_width: u64,
data_width: u64,
memory_size: u64,
addr_width: u64,
) -> v::Module {
let name = &format!("Memory_controller_axi_{}", index);
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 +125,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,7 +166,15 @@ impl MemoryInterface for AxiInterface {
));

// bram reading / writing logic
bram_logic(&axi4, &mut module, &mode_fsm, "read_txn_count".into());
bram_logic(
index,
&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(),
Expand Down Expand Up @@ -293,18 +305,22 @@ fn module_mode_fsm(module: &mut v::Module) -> fsm::LinearFsm {
}

fn bram_logic(
index: u64,
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
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 name = &format!("SINGLE_PORT_BRAM_{}", index);
let mut ram_instance = v::Instance::new("bram", name);
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 +331,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); // does this need to use parameterization?
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -387,8 +403,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
73 changes: 68 additions & 5 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,43 @@ impl Backend for XilinxInterfaceBackend {
" Please make sure that at least one memory is marked as @external."));
}

let widths = get_mem_widths(toplevel);
let sizes = get_mem_sizes(toplevel);
let idx_sizes = get_mem_idx_sizes(toplevel);

let mut modules = vec![
// what defines top_level data_width and addr_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),
widths[i],
sizes[i],
idx_sizes[i],
))
}

// Possible to have this parameterized for every memory and then
// have as many bram modules as there are memories. <- this is current method

//July 11 2022: replaced by for loop above
//bram(32, 32, 5),
modules.push(axi::AxiInterface::control_module(
"Control_axi",
12,
32,
&memories,
));

for (i, _mem) in memories.iter().enumerate() {
modules.push(axi::AxiInterface::memory_module(
&format!("Memory_controller_axi_{}", i),
i as u64, // is this safe?
512,
64,
32,
widths[i],
sizes[i],
idx_sizes[i],
))
}

Expand All @@ -79,6 +104,44 @@ impl Backend for XilinxInterfaceBackend {
}
}

////I __think__ this should always be in the same order because
//of IdList being in the same order
//should this be a macro?
fn external_memories_cells(
comp: &ir::Component,
) -> Vec<std::rc::Rc<std::cell::RefCell<ir::Cell>>> {
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
comp.cells
.iter()
.filter(|cell_ref| {
matches!(cell_ref.borrow().get_attribute("external"), Some(&1))
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
})
.cloned()
.collect()
}

//returns vector of memory widths from a component
nathanielnrn marked this conversation as resolved.
Show resolved Hide resolved
fn get_mem_widths(comp: &ir::Component) -> Vec<u64> {
external_memories_cells(comp)
.iter()
.map(|cell_ref| cell_ref.borrow().get_parameter("WIDTH").unwrap())
.collect()
}

fn get_mem_sizes(comp: &ir::Component) -> Vec<u64> {
external_memories_cells(comp)
.iter()
.map(|cell_ref| cell_ref.borrow().get_parameter("SIZE").unwrap())
.collect()
}

fn get_mem_idx_sizes(comp: &ir::Component) -> Vec<u64> {
external_memories_cells(comp)
.iter()
.map(|cell_ref| cell_ref.borrow().get_parameter("IDX_SIZE").unwrap())
.collect()
}

// Returns Vec<String> of memory names
fn external_memories(comp: &ir::Component) -> Vec<String> {
// find external memories
comp.cells
Expand Down Expand Up @@ -171,7 +234,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, 5)); //width should match memory width in calyx
module.add_decl(v::Decl::new_wire(&write_en, 1));
module.add_decl(v::Decl::new_wire(&done, 1));

Expand Down