Replace separate memory modules with single unified memory module by spr4bhu · Pull Request #22 · SRA-VJTI/synapse32

spr4bhu · 2026-02-12T10:43:55Z

removed separate instr_mem and data_mem modules and added a single unified_memory module

Copilot

Pull request overview

Replaces separate instruction and data memory modules with a single unified dual-port memory, updating the top-level wiring and related address/memory-map logic.

Changes:

Added unified_memory RTL module implementing instruction fetch + data read/write in one memory.
Updated top.v to instantiate unified_memory and translate instruction/data addresses into a unified address space.
Simplified/renamed pipeline stall wiring in riscv_cpu.v and removed legacy instr_mem.v / data_mem.v.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
rtl/unified_memory.v	New unified dual-port memory with init + hex loading + data load formatting
rtl/top.v	Swaps old memories for `unified_memory` and adds address translation/control wiring
rtl/riscv_cpu.v	Renames stall wiring and adjusts IF/ID stall/flush interaction
rtl/include/memory_map.vh	Updates `IS_INSTR_MEM` macro to use `INSTR_MEM_END`
rtl/instr_mem.v	Removed legacy instruction memory module
rtl/data_mem.v	Removed legacy data memory module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-12T16:04:41Z

rtl/unified_memory.v

+            // Read program as words
+            $readmemh(`INSTR_HEX_FILE, word_mem);
+
+            // Unpack words into byte array (little-endian)
+            for (i = 0; i < MEM_SIZE/4; i = i + 1) begin
+                ram[i*4 + 0] = word_mem[i][7:0];
+                ram[i*4 + 1] = word_mem[i][15:8];
+                ram[i*4 + 2] = word_mem[i][23:16];
+                ram[i*4 + 3] = word_mem[i][31:24];
+            end


word_mem is never initialized before $readmemh, and the unpack loop copies every entry into ram. For addresses not present in the hex file, word_mem[i] can remain X/undefined, which then overwrites the earlier NOP initialization in ram and can poison instruction/data reads beyond the loaded program. Initialize word_mem to NOPs (or another defined value) before $readmemh, or only unpack the range actually loaded.

rtl/unified_memory.v

Copilot · 2026-02-12T16:04:42Z

rtl/top.v

-        .MEM_SIZE(1048576)  // 1MB in bytes
-    ) data_mem_inst (
+        .DATA_WIDTH(32),
+        .MEM_SIZE(1572864)  // 1.5MB in bytes (instr + data)


UNIFIED_MEM_SIZE is computed but not used, while MEM_SIZE is hard-coded to 1572864. This risks silent mismatches if INSTR_MEM_SIZE / DATA_MEM_SIZE change. Use UNIFIED_MEM_SIZE when setting the unified_memory parameter.

Suggested change

.MEM_SIZE(1572864) // 1.5MB in bytes (instr + data)

.MEM_SIZE(UNIFIED_MEM_SIZE) // unified size = instr + data

rtl/include/memory_map.vh

Copilot · 2026-02-12T16:04:43Z

rtl/unified_memory.v

+    reg [7:0] ram [0:MEM_SIZE-1];
+    reg [31:0] word_mem [0:MEM_SIZE/4-1];


word_mem is a second large memory array used only for initialization/loading. Depending on toolchain, this can increase compile/synthesis time and memory usage (and may not always be optimized away cleanly). Consider guarding word_mem under a simulation-only define (e.g., ifndef SYNTHESIS) or restructuring the hex load to avoid a second full-sized array.

fix this @spr4bhu

5iri

@spr4bhu I think fixing these ones are more important than the ones suggested by copilot.

do fix these and I'll merge this as well

5iri · 2026-02-24T17:37:16Z

rtl/unified_memory.v

+    reg [7:0] ram [0:MEM_SIZE-1];
+    reg [31:0] word_mem [0:MEM_SIZE/4-1];


fix this @spr4bhu

5iri · 2026-02-24T17:39:30Z

rtl/top.v

+    assign instr_addr = cpu_pc_out;
+    assign data_addr = instr_mem_access ? (data_mem_addr - `INSTR_MEM_BASE) :
+                                          (data_mem_addr - `DATA_MEM_BASE + `INSTR_MEM_SIZE);
+    assign data_we = cpu_mem_write_en && data_mem_access;


data_we should include instr_mem_access imo.
this allows for later bootloader firmware that copies code to instruction memory

5iri · 2026-02-24T17:41:18Z

rtl/riscv_cpu.v

    wire [31:0] pc_inst0_jump;
-    wire stall_pipeline; // For load-use hazards
+    wire load_use_stall; // For load-use hazards
+    wire pipeline_stall;


This stall/flush refactoring is functionally independent of the memory unification. These are good changes but could be a separate PR for cleaner history.

spr4bhu · 2026-02-26T06:00:43Z

@5iri check pls

5iri

I have suggested some changes in the unified_memory.v file. Please fix them!

5iri · 2026-03-03T12:42:22Z

rtl/top.v

                          data_mem_access ? data_mem_read_data :
                          uart_access ? uart_read_data :
-                            instr_mem_access ? instr_read_data : 32'h00000000;
+                          instr_mem_access ? data_mem_read_data : 32'h00000000;


I feel the naming here feels incorrect;

Do change this to something generic like just mem_read_data so that people reading through the code understands.

5iri · 2026-03-03T13:28:24Z

rtl/unified_memory.v

+            $display("Loading unified memory from file: %s", `INSTR_HEX_FILE);
+
+            // Read program as words
+            for (i = 0; i < MEM_SIZE/4; i = i + 1) begin


uhhh why are you writing the entire memory space with NOPs?

I think this can be fixed alongside the INSTR_MEM_SIZE fix below.

5iri · 2026-03-03T13:31:19Z

rtl/unified_memory.v

+    assign aligned_instr_addr = {addr_instr[ADDR_WIDTH-1:2], 2'b00};
+
+    always @(*) begin
+        if (aligned_instr_addr < MEM_SIZE - 3) begin


MEM_SIZE is now the entire size of both instructions as well as data; and instr addr length should depend on INSTR_MEM_SIZE not MEM_SIZE even now

Replace separate memory modules with single unified memory module

8994e8f

spr4bhu force-pushed the unified_mem branch from c9974d8 to 8994e8f Compare February 12, 2026 11:09

fixed memory access

e20fd80

5iri requested a review from Copilot February 12, 2026 16:01

Copilot AI reviewed Feb 12, 2026

View reviewed changes

Shashvat Prabhu and others added 2 commits February 13, 2026 10:49

adding suggested changes

1e3b6d9

code cleanup

52e4c7d

5iri requested changes Feb 24, 2026

View reviewed changes

made requested changes

e2ab0a6

spr4bhu requested a review from 5iri March 3, 2026 12:04

5iri requested changes Mar 3, 2026

View reviewed changes

fix unified memory naming, instruction bounds check, and hex load size

c9d885e

spr4bhu requested a review from 5iri March 13, 2026 07:52

	.MEM_SIZE(1572864) // 1.5MB in bytes (instr + data)
	.MEM_SIZE(UNIFIED_MEM_SIZE) // unified size = instr + data

		reg [7:0] ram [0:MEM_SIZE-1];
		reg [31:0] word_mem [0:MEM_SIZE/4-1];

Conversation

spr4bhu commented Feb 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

5iri Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

5iri left a comment

Choose a reason for hiding this comment

Uh oh!

5iri Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

5iri Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

5iri Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

spr4bhu commented Feb 26, 2026

Uh oh!

5iri left a comment

Choose a reason for hiding this comment

Uh oh!

5iri Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5iri Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

5iri Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

5iri Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

5iri Mar 3, 2026 •

edited

Loading