memrouter changes#16
Conversation
|
cool |
There was a problem hiding this comment.
Pull request overview
This PR refactors the memory routing architecture to support virtualization by consolidating separate SPAD and ACC memory interfaces into a unified virtual bank system. The changes simplify the architecture by using a single shared ToPhysicalLine module instead of per-ball instances, and introduce new bundle types (SramReadWithInfo and SramWriteWithInfo) that carry additional routing metadata.
- Unified memory bank interfaces (combining SPAD and ACC into numBanks)
- Replaced per-ball
ToPhysicalLinemodules with a single shared instance - Updated tutorial documentation with config parameter
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/bb-note/src/tutorial/tutorial.md | Added config parameter to verilator command |
| arch/src/main/scala/framework/switcher/ToPhysicalLine.scala | Changed from per-ball RobId bundles to unified IO bundles, removed rob_id field assignments |
| arch/src/main/scala/framework/balldomain/blink/blink.scala | Updated comments to document additional fields in WithInfo bundles |
| arch/src/main/scala/framework/balldomain/bbus/memrouter/memRouter.scala | Consolidated SPAD/ACC interfaces into unified numBanks structure, updated routing logic |
| arch/src/main/scala/framework/balldomain/bbus/memrouter/SramIOAdapter.scala | Changed interfaces to use WithInfo bundles, added wire for proper field propagation |
| arch/src/main/scala/framework/balldomain/bbus/bbus.scala | Replaced per-ball ToPhysicalLine instances with single shared module, updated signal routing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } |
There was a problem hiding this comment.
Incomplete signal connection. Only io.req is connected, but SramReadWithInfo also includes rob_id, is_acc, and bank_id fields that need to be propagated. Consider connecting the entire bundle or explicitly handling all fields.
| } | |
| } | |
| io.sramRead_o(j).io.rob_id := io.sramRead_i(i)(j).io.rob_id | |
| io.sramRead_o(j).io.is_acc := io.sramRead_i(i)(j).io.is_acc | |
| io.sramRead_o(j).io.bank_id := io.sramRead_i(i)(j).io.bank_id | |
| } |
| io.sramRead_i(i)(j).io.resp <> io.sramRead_o(j).resp | ||
| for(j <- 0 until numBanks){ | ||
| when(io.sramRead_o(j).io.resp.valid){ | ||
| io.sramRead_i(i)(j).io.resp <> io.sramRead_o(j).io.resp |
There was a problem hiding this comment.
Incomplete signal connection. Only io.resp is connected, but SramReadWithInfo also includes rob_id, is_acc, and bank_id fields that need to be propagated. Consider connecting the entire bundle or explicitly handling all fields.
| io.sramRead_i(i)(j).io.resp <> io.sramRead_o(j).io.resp | |
| io.sramRead_i(i)(j).io <> io.sramRead_o(j).io |
|
|
||
| read_o.rob_id := io.sramWrite_i.rob_id | ||
| read_o.is_acc := io.sramWrite_i.is_acc | ||
| read_o.bank_id := io.sramWrite_i.bank_id |
There was a problem hiding this comment.
Trailing whitespace after the assignment. Remove the extra spaces at the end of this line.
| read_o.bank_id := io.sramWrite_i.bank_id | |
| read_o.bank_id := io.sramWrite_i.bank_id |
| read_o.is_acc := io.sramWrite_i.is_acc | ||
| read_o.bank_id := io.sramWrite_i.bank_id | ||
|
|
||
| io.sramRead_o <> read_o |
There was a problem hiding this comment.
Trailing whitespace after the assignment. Remove the extra spaces at the end of this line.
| io.sramRead_o <> read_o | |
| io.sramRead_o <> read_o |
| } | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
| // ToPhysicalLine - per-ball conversion from virtual to physical line |
There was a problem hiding this comment.
Misleading comment. The comment says "per-ball conversion" but the code creates a single shared ToPhysicalLine module (not one per ball). Update the comment to reflect that this is a shared module that converts from unified virtual banks to separate physical SPAD and ACC banks.
| // ToPhysicalLine - per-ball conversion from virtual to physical line | |
| // ToPhysicalLine - shared module converting from unified virtual banks to separate physical SPAD and ACC banks |
| // SramWriteIO with rob_id, is_acc, bank_id | ||
| class SramWriteWithInfo(val n: Int, val w: Int, val mask_len: Int)(implicit b: CustomBuckyballConfig, p: Parameters) extends Bundle { | ||
| val io = new SramWriteIO(n, w, mask_len) | ||
| // Input because theSramWriteIO outer layer has Flipped |
There was a problem hiding this comment.
Typo in comment: "theSramWriteIO" should have a space - "the SramWriteIO".
| // Input because theSramWriteIO outer layer has Flipped | |
| // Input because the SramWriteIO outer layer has Flipped |
| for(j <- 0 until b.acc_banks){ | ||
| when(io.accWrite_i(i)(j).io.req.valid){ | ||
| io.accWrite_o(j) <> io.accWrite_i(i)(j).io | ||
| io.sramWrite_o(j)<> io.sramWrite_i(i)(j) |
There was a problem hiding this comment.
Missing space before the <> operator. Should be io.sramWrite_o(j) <> io.sramWrite_i(i)(j) for consistency with other connection statements.
| io.sramWrite_o(j)<> io.sramWrite_i(i)(j) | |
| io.sramWrite_o(j) <> io.sramWrite_i(i)(j) |
Memrouter and related modules changes for virtualization
Tutorail updated