ram: simplification of word slice creation by braydenlouie · Pull Request #9690 · The-OpenROAD-Project/OpenROAD

braydenlouie · 2026-03-08T01:49:59Z

Changes in RAM creation order

Switched to building out each word slice out (row by row)
For simplifying all the indices used in the current implementation for readability

Signed-off-by: braydenl9988 <braydenl9988@gmail.com>

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

Signed-off-by: braydenlouie <braydenl9988@gmail.com>

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

gemini-code-assist

Code Review

This pull request refactors the RAM generation logic to build word slices row by row, which improves code readability and simplifies indexing. The changes are generally good and achieve the stated goal. I've found one bug in the layout generation logic that could lead to incorrect behavior, and a couple of minor issues related to code clarity and consistency. Once these are addressed, the changes will be a solid improvement.

_{Note: Security Review did not run due to the size of the PR.}

gemini-code-assist · 2026-03-08T01:52:31Z

src/ram/src/layout.cpp

+    for (int size = layouts_.size(); size <= tracks; ++size) {
+      if (orientation_ == odb::horizontal) {
+        layouts_.push_back(std::make_unique<Layout>(odb::vertical));
+      } else {
+        layouts_.push_back(std::make_unique<Layout>(odb::horizontal));
+      }
+    }


There is an off-by-one error in this loop condition. The loop for (int size = layouts_.size(); size <= tracks; ++size) will execute one more time than intended, creating an extra Layout object. For example, if layouts_ is empty and tracks is 10, this loop will run for size from 0 to 10 inclusive, creating 11 layouts instead of 10. The condition should be size < tracks to create the correct number of layouts.

Suggested change

for (int size = layouts_.size(); size <= tracks; ++size) {

if (orientation_ == odb::horizontal) {

layouts_.push_back(std::make_unique<Layout>(odb::vertical));

} else {

layouts_.push_back(std::make_unique<Layout>(odb::horizontal));

}

}

for (int size = layouts_.size(); size < tracks; ++size) {

if (orientation_ == odb::horizontal) {

layouts_.push_back(std::make_unique<Layout>(odb::vertical));

} else {

layouts_.push_back(std::make_unique<Layout>(odb::horizontal));

}

}

gemini-code-assist · 2026-03-08T01:52:31Z

src/ram/include/ram/ram.h

+  void makeCellByte(int start_idx,
+                    int row_idx,


The first parameter of makeCellByte is named start_idx here in the header, but it's named byte_idx in the implementation file (src/ram/src/ram.cpp). Using byte_idx is more descriptive as it represents the index of the byte within a word. For consistency and clarity, it would be best to use the same name in both the declaration and definition.

Suggested change

void makeCellByte(int start_idx,

int row_idx,

void makeCellByte(int byte_idx,

int row_idx,

gemini-code-assist · 2026-03-08T01:52:31Z

src/ram/src/ram.cpp

+    int start_idx
+        = byte * bits_per_byte;  // eventually replace with bits per byte


The comment // eventually replace with bits per byte is confusing because the variable bits_per_byte is already being used in the expression on the same line. This appears to be a stale comment from a previous stage of refactoring and should be removed to avoid confusion.

int start_idx = byte * bits_per_byte;

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-08T01:56:14Z

src/ram/include/ram/ram.h

-      int byte_number,
-      const std::string& prefix,
-      int read_ports,
+  void makeCellByte(int start_idx,


warning: function 'ram::RamGen::makeCellByte' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

void makeCellByte(int start_idx, ^

Additional context

src/ram/src/ram.cpp:139: the definition seen here

void RamGen::makeCellByte(const int byte_idx, ^

src/ram/include/ram/ram.h:121: differing parameters are named here: ('start_idx'), in definition: ('byte_idx')

void makeCellByte(int start_idx, ^

github-actions · 2026-03-08T01:56:14Z

src/ram/include/ram/ram.h

+                    const std::vector<std::vector<odb::dbBTerm*>>& data_output);
+
+  void makeWordSlice(
+      const int bytes_per_word,


warning: parameter 'bytes_per_word' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const int bytes_per_word,

int bytes_per_word,

github-actions · 2026-03-08T01:56:14Z

src/ram/include/ram/ram.h

+
+  void makeWordSlice(
+      const int bytes_per_word,
+      const int row_idx,


warning: parameter 'row_idx' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const int row_idx,

int row_idx,

github-actions · 2026-03-08T01:56:14Z

src/ram/include/ram/ram.h

+  void makeWordSlice(
+      const int bytes_per_word,
+      const int row_idx,
+      const int read_ports,


warning: parameter 'read_ports' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const int read_ports,

int read_ports,

github-actions · 2026-03-08T01:56:14Z

src/ram/src/ram.cpp

+    vector<vector<dbBTerm*>> byte_outputs(read_ports,
+                                          vector<dbBTerm*>(bits_per_byte));
+    for (int port = 0; port < read_ports; ++port) {
+      std::copy_n(data_output[port].begin() + start_idx,


warning: no header providing "std::copy_n" is directly included [misc-include-cleaner]

src/ram/src/ram.cpp:5:

- #include <array> + #include <algorithm> + #include <array>

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

braydenlouie · 2026-03-08T02:06:04Z

@rovinski Here are the code simplification changes. At the moment the ok files are still not lining up, even when I tested it with the cmake build so still need to look into that. The differences are related to two of the pins and I believe some routing. I don't know if that's an issue with the changes I made or if they are a product of any changes to the other tools. The rest of the source files should be fixed barring linting and formatting changes.

braydenlouie added 7 commits March 6, 2026 20:23

ram: start of code simplification + refactoring

2638c71

Signed-off-by: braydenl9988 <braydenl9988@gmail.com>

ram: edit to Bazel build and file includes

70e5515

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

ram: start of word slice creation

2b8e13e

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

commit before pulling behavioral verilog changes

46337bf

Signed-off-by: braydenlouie <braydenl9988@gmail.com>

Merge branch 'The-OpenROAD-Project:master' into simplification

8e7f1eb

ram: simplification changes without cmake build testing

f8c8845

Signed-off-by: braydenlouie <braydenl9988@gmail.com>

ram: change to net name, removal of debug log messages

7793d3a

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

gemini-code-assist bot reviewed Mar 8, 2026

View reviewed changes

github-actions bot reviewed Mar 8, 2026

View reviewed changes

ram: removed extra comments

d1d7a1d

Signed-off-by: Brayden Louie <braydenl9988@gmail.com>

braydenlouie force-pushed the simplification branch from 4bccd78 to d1d7a1d Compare March 8, 2026 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ram: simplification of word slice creation#9690

ram: simplification of word slice creation#9690
braydenlouie wants to merge 8 commits intoThe-OpenROAD-Project:masterfrom
braydenlouie:simplification

braydenlouie commented Mar 8, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Mar 8, 2026

Uh oh!

gemini-code-assist bot Mar 8, 2026

Uh oh!

gemini-code-assist bot Mar 8, 2026

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot Mar 8, 2026

Uh oh!

github-actions bot Mar 8, 2026

Uh oh!

github-actions bot Mar 8, 2026

Uh oh!

github-actions bot Mar 8, 2026

Uh oh!

github-actions bot Mar 8, 2026

Uh oh!

braydenlouie commented Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		int start_idx
		= byte * bits_per_byte; // eventually replace with bits per byte

Conversation

braydenlouie commented Mar 8, 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 Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

braydenlouie commented Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant