Add behavioral verilog output by tnguy19 · Pull Request #9539 · The-OpenROAD-Project/OpenROAD

tnguy19 · 2026-02-24T23:54:38Z

add argument -write_behavioral_verilog [filename] to generate_ram allowing the user to generate a behavioral verilog file for RAM
update storage_cell in MakeCellInst (line 122) to allow use of CLK and GATE pin for flip flop and latch cells

@rovinski

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

gemini-code-assist

Code Review

This pull request introduces a valuable feature to generate a behavioral Verilog model for the RAM. The implementation is largely correct, but I've identified a critical bug in the address width calculation for an edge case that would lead to invalid Verilog. Additionally, I've provided suggestions to enhance code style consistency, aligning with repository guidelines, and to refine the formatting of the generated output.

gemini-code-assist · 2026-02-24T23:57:04Z

src/ram/src/ram.cpp

+    }
+
+    const int word_size_bit = bytes_per_word * 8; 
+    const int address_width = std::ceil(std::log2(word_count));


The calculation of address_width is incorrect when word_count is 1. In this case, std::log2(1) is 0, so address_width becomes 0. This leads to address_width - 1 being -1, resulting in an invalid Verilog port declaration like input [-1:0] addr;. The address width should be at least 1.

const int address_width = (word_count <= 1) ? 1 : std::ceil(std::log2(word_count));

updated in af4dd47

src/ram/src/ram.cpp

github-actions

clang-tidy made some suggestions

src/ram/src/ram.cpp

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions

clang-tidy made some suggestions

src/ram/src/ram.cpp

remove redundant initializer Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: tnguy19 <114105118+tnguy19@users.noreply.github.com>

github-actions

clang-tidy made some suggestions

src/ram/src/ram.cpp

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-25T00:40:05Z

clang-tidy review says "All clean, LGTM! 👍"

rovinski

Can you please add a test against the behavioral verilog file, e.g. adding the model for the 8x8 ram as a .vok file?

I also want to talk about making sure the structural model and behavioral model actually match expected behavior when we next meet.

rovinski · 2026-02-25T06:51:03Z

src/ram/src/ram.cpp

+  std::ofstream vf(filename);
+  if (!vf.is_open()) {
+    logger_->error(RAM, 23, "Unable to open file {}", filename);
+    return;
+  }
+
+  vf << verilog_code;
+  vf.close();


@maliberty isn't there a utility function for writing files? Should that be used here?

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-25T15:37:22Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-26T21:22:27Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-02-27T00:20:24Z

clang-tidy review says "All clean, LGTM! 👍"

rovinski · 2026-02-27T01:46:21Z

src/ram/test/make_8x8_behavioral.tcl

Does this need to be a separate test? Can't you just add the flag to the make8x8 test?

rovinski · 2026-02-27T01:47:19Z

test still requires fixing for CI

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-27T02:02:49Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-27T14:39:37Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-27T15:02:41Z

clang-tidy review says "All clean, LGTM! 👍"

rovinski

Go ahead and fix this last item then mark as ready for review.

@maliberty this looks good to me.

rovinski · 2026-02-27T18:40:12Z

src/ram/test/make_8x8_behavioral.vok

A few notes:

No indentation for module

Please name the module the exact same as the block. This will help developers seamlessly swap between the gate-level and behavioral versions.

Indent ports (D, Q0, etc.)

Please use 2-space indenting

Power and ground ports should not be included

Consistent indentation and spacing
** e.g. for (i=0; i< 1; i = i +1 ) --> for (i = 0; i < 1; i = i + 1)
** e.g. no empty line after // read logic

rovinski · 2026-02-27T18:43:11Z

CI is still failing though, please make sure everything is up to date.

…nd .vok files Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-27T19:13:06Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-02-27T19:17:46Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty · 2026-02-28T05:00:47Z

src/ram/src/ram.cpp

+  std::ofstream vf(filename);
+  if (!vf.is_open()) {
+    logger_->error(RAM, 23, "Unable to open file {}", filename);
+    return;


rm - return is unreachable as error will throw

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-02-28T05:22:16Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty · 2026-02-28T05:32:03Z

src/ram/test/make_8x8_behavioral.vok

+  always @(*) begin
+    Q0 = mem[addr];
+  end
+
+  always @(*) begin
+    Q1 = mem[addr];
+  end


Is this correct? You have two identical read ports using a single address?

Yes, because the structural verilog code for the ram that the behavioral code generator is based on uses the same address bus shared with all the read ports. So all read ports currently read from the same address

So a correct description of the hardware but not normal meaning to real ports afaik. @rovinski is this what you intend?

@tnguy yeah, that's not right. If that's what the structural netlist does, then it's wrong.

There should be an address port per read port. Fix up the behavioral model for now and we'll have to modify the netlist to be correct later.

@rovinski I have added the update in 190a188 please let me know if theres any changes needed in the new behavioral code

Hmmm, I think this raises an issue on the lack of clarity for the tool interface. It is true that there is one write port and two read ports. But it is unclear whether this means that it should be 3 ports total (2 read 1 write) or 2 ports (1 read-write 1 read).

Right now, we only implicitly support 1 write port. I think it should be that there is always 1 read-write port and then additional read ports. In other words, the first addr port is used for both reading and writing, and then any additional ports are only for reading.

If you make this change, then we can mark it done for now and leave an issue to come back to it when more write ports are supported.

I have added the updated code in 80c67ad, ready for review

@rovinski you are wanting to merge this with code the writes a behavioral model that doesn't match the hardware generated? That seems confusing/dangerous.

I am inclined to for now with the goal that the hardware is fixed to match. It didn't occur to me until now that essentially only 1rw memories actually work. Everything else is broken because there is no routine to implement multiple decoders.

Perhaps the ok test should be changed to be 1rw rather than having 2 broken read ports.

Then let's put in an error if the user selects a broken configuration. I don't want to give misleading results.

…ort per read port and new address write input Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-03-01T21:07:32Z

clang-tidy review says "All clean, LGTM! 👍"

… read-write port and additional read ports Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-03-02T17:14:24Z

clang-tidy review says "All clean, LGTM! 👍"

rovinski · 2026-03-03T20:39:56Z

@tnguy19 based on conversation, can you please do the following before merge:

Change the ram test and ok files to be based on -read_ports 1.
Add a check for read_ports != 1, and if so, error with "The ram generator currently only supports 1 read port."

…ted read_ports value !=1 Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

github-actions · 2026-03-04T00:36:39Z

clang-tidy review says "All clean, LGTM! 👍"

tnguy19 · 2026-03-05T20:40:29Z

@maliberty I have added the updates in a526f03 code is ready for review

rovinski · 2026-03-05T20:44:13Z

@maliberty CI looks green now, I think this is ready to merge. I added fixing muli-read/write ports as a top priority item in #9392

tnguy19 added 5 commits February 24, 2026 18:56

Add -write_behavioral_verilog argument to generate_ram

63d631c

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Fix duplicate message IDs - use 23 and 24

0994767

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Fix duplicate message IDs - use 23 and 24

154c2b5

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Add registered address for synchronous RAM reads

53445a0

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Add behavioral Verilog write with combinational read

5e6a90c

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

gemini-code-assist bot reviewed Feb 24, 2026

View reviewed changes

tnguy19 force-pushed the add-behavioral-verilog-output branch from e99a104 to 84c0f61 Compare February 24, 2026 23:57

github-actions bot reviewed Feb 24, 2026

View reviewed changes

src/ram/src/ram.cpp Outdated Show resolved Hide resolved

src/ram/src/ram.cpp Show resolved Hide resolved

Fix address width edge case, clang-tidy warnings, add fstream include

af4dd47

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

tnguy19 force-pushed the add-behavioral-verilog-output branch from 84c0f61 to af4dd47 Compare February 25, 2026 00:08

github-actions bot reviewed Feb 25, 2026

View reviewed changes

src/ram/src/ram.cpp Outdated Show resolved Hide resolved

Update src/ram/src/ram.cpp

f7dcb17

remove redundant initializer Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: tnguy19 <114105118+tnguy19@users.noreply.github.com>

github-actions bot reviewed Feb 25, 2026

View reviewed changes

src/ram/src/ram.cpp Show resolved Hide resolved

Fix constructor initializer list and apply clang-format

0ffc623

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

rovinski reviewed Feb 25, 2026

View reviewed changes

add behavioral Verilog test for 8x8 RAM

75e3c16

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

tnguy19 added 2 commits February 26, 2026 16:17

update make_8x8 test to use dfxtp_1 flip-flop storage cell

7df530c

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Update make_8x8.tcl and golden files for dfxtp_1 storage cell

49e5520

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Merge branch 'master' into add-behavioral-verilog-output

6bd7d62

rovinski reviewed Feb 27, 2026

View reviewed changes

Fix make_8x8.ok to include full log output

d0d38cf

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

merge behavioral test into make_8x8, update ok files

22f2e22

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Add clang format ram.cpp

5159174

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

merge upstream master into add-behavioral-verilog-output

b15bc4a

rovinski approved these changes Feb 27, 2026

View reviewed changes

update format for behavioral code in generator function, update .ok a…

576ef0e

…nd .vok files Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

tnguy19 force-pushed the add-behavioral-verilog-output branch from d4d2552 to 576ef0e Compare February 27, 2026 19:12

tnguy19 marked this pull request as ready for review February 27, 2026 20:23

maliberty reviewed Feb 28, 2026

View reviewed changes

tnguy19 marked this pull request as draft February 28, 2026 05:11

remove unreachable return after logger error

45a6126

Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

maliberty marked this pull request as ready for review February 28, 2026 05:30

maliberty reviewed Feb 28, 2026

View reviewed changes

update behavioral generator code and .vok file to allow one address p…

190a188

…ort per read port and new address write input Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

update behavioral verilog generator function and .vok file to use one…

80c67ad

… read-write port and additional read ports Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

Update golden files to 1 read port case, add error check for unsuppor…

a526f03

…ted read_ports value !=1 Signed-off-by: Thinh Nguyen <nguyenthinh19011@gmail.com>

maliberty mentioned this pull request Mar 5, 2026

ram: Generate behavioral models #9472

Closed

maliberty approved these changes Mar 5, 2026

View reviewed changes

maliberty merged commit 38fe88d into The-OpenROAD-Project:master Mar 5, 2026
13 checks passed

Conversation

tnguy19 commented Feb 24, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 25, 2026

Uh oh!

rovinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 25, 2026

Uh oh!

github-actions bot commented Feb 26, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rovinski commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

rovinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rovinski commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

github-actions bot commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rovinski Mar 3, 2026 •

edited

Loading