ram: Cross platform testing and support by braydenlouie · Pull Request #9991 · The-OpenROAD-Project/OpenROAD

braydenlouie · 2026-03-30T04:30:07Z

Summary

Replaces hardcoded sky130 port names with a PDK-independent pin mapping system
Introduces 'PortRole' struct with 'PortRoleType' enum that is used to classify Liberty ports by role instead of name
Verified against both sky130hd and nangate45

Type of Change

New feature
Refactoring

Impact

RamGen is no longer tied to sky130 port naming conventions and allows using any PDK with a valid Liberty file, with pin names resolved automatically. The existing sky130 behavior is preserved.

Verification

I have verified that the local build succeeds (./etc/Build.sh).
I have run the relevant tests and they pass.
My code follows the repository's formatting guidelines.
I have signed my commits (DCO).

Related Issues

#9468
#9392

add test make_7x7_nangate45 exercising generate_ram with Nangate45 standard cells. Uses an intentionally odd 7-word x 1-byte (56-bit) configuration to stress the decoder logic on a non-power-of-2 count. also fix a latent bug in makeCellBit() where the clock-pin name probe only checked for CLK (sky130hd) or GATE (latch cells), silently missing CK which is the Nangate45 DFF_X1 clock pin name. The probe chain is extended to CLK -> CK -> GATE. Resolves The-OpenROAD-Project#9468. Signed-off-by: OpenROAD Contributor <contributor@openroad.org>

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

braydenlouie · 2026-03-30T04:33:41Z

@rovinski I haven't completed testing with generate_ramyet, but the generate_ram_netlist (just placing all cells, no routing, pdngen, etc.) for Nangate45 is working. Just wanted to put this draft PR up so you can take a look at let me know if this implementation is viable.

gemini-code-assist

Code Review

This pull request introduces a more robust mechanism for resolving technology-specific pin names in the RamGen class by utilizing a new PinRole system and a buildPinMap method. It also adds a new regression test for a 7x7 RAM configuration using the Nangate45 library. Several issues were identified in the review: a critical copy-paste error in the decoder logic, incorrect parameters in the new 7x7 test file and its golden Verilog output, leftover debug logging, and redundant code blocks that should be cleaned up before merging.

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/src/ram.cpp

+               {{and2_pins_[{PinRoleType::DataIn, 0}], addr_nets[i]},
+                {and2_pins_[{PinRoleType::DataIn, 0}], input_net},
+                {and2_pins_[{PinRoleType::DataIn, 0}], prev_net}});


There appears to be a bug in the connections for this makeInst call. All three connections are incorrectly mapped to PinRoleType::DataIn with index 0, and there is no output connection. This looks like a copy-paste error.

Based on the logic of the surrounding code, the connections should likely be two distinct inputs and one output.

{{and2_pins_[{PinRoleType::DataIn, 0}], addr_nets[i]}, {and2_pins_[{PinRoleType::DataIn, 1}], input_net}, {and2_pins_[{PinRoleType::DataOut, 0}], prev_net}};

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/test/make_7x7_nangate45_behavioral.vok

+module RAM7x8 (
+  clk,
+  D,
+  Q,
+  addr_rw,
+  we
+);
+  input clk;
+  input [7:0] D;
+  output reg [7:0] Q;
+  input [2:0] addr_rw;
+  input [0:0] we;
+
+  // memory array declaration
+  reg [7:0] mem[0:6];
+
+  // write logic
+  integer i;
+  always @(posedge clk) begin
+    for (i = 0; i < 1; i = i + 1) begin
+      if (we[i]) begin
+        mem[addr_rw][i*8 +:8] <= D[i*8 +:8];
+      end
+    end
+  end
+
+  // read logic
+  always @(*) begin
+    Q = mem[addr_rw];
+  end
+
+endmodule


The contents of this golden Verilog file seem incorrect for a 7x7 RAM. It appears to have been copied from an 8-bit wide RAM test and not fully updated.

Specifically:

The module name is RAM7x8, but should be RAM7x7.

The D and Q ports are 8 bits wide ([7:0]), but should be 7 bits ([6:0]).

The memory array mem is declared as reg [7:0], but should be reg [6:0].

The write logic uses an 8-bit part-select (i*8 +:8), but should use 7.

Please update this file to reflect the correct behavior for a 7-word by 7-bit RAM.

Gemini is correct here. Something not right with the behavioral model.

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/src/ram.cpp

+  // Resolve the clock-pin name for the storage cell by querying the
+  // Liberty data.  This avoids hard-coding technology-specific pin names
+  // (e.g. "CLK", "CK") and works with any technology node.
+  // const char* clk_pin = nullptr;
+  // const char* data_in_pin = nullptr;
+  // auto sta_cell = network_->dbToSta(storage_cell_);
+  // if (sta_cell) {
+  //   auto liberty = network_->libertyCell(sta_cell);
+  //   if (liberty) {
+  //     auto port_iter = liberty->portIterator();
+  //     while (port_iter->hasNext()) {
+  //       auto lib_port = port_iter->next();
+  //       if (lib_port->libertyPort() && lib_port->libertyPort()->isClock()) {
+  //         clk_pin = lib_port->name();
+  //       }
+  //       if (lib_port->libertyPort() &&
+  //       lib_port->libertyPort()->isLatchData()) {
+  //         data_in_pin = lib_port->name();
+  //       }
+  //     }
+  //     delete port_iter;
+  //   }
+  // }
+  // if (!clk_pin) {
+  //   logger_->error(RAM,
+  //                  12,
+  //                  "No clock pin found on storage cell {}.",
+  //                  storage_cell_->getName());
+  // }


This large block of commented-out code should be removed. It appears to be legacy logic for resolving pin names that has been replaced by the new buildPinMap and *_pins_ maps. Keeping dead code can be confusing for future maintenance.

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/src/ram.cpp

+    logger_->info(RAM,
+                  98,
+                  "port name:{} dir:{} isPwrGnd:{}",
+                  lib_port->name(),
+                  dir->name(),
+                  lib_port->isPwrGnd());


This info level log message inside a loop seems to be for debugging purposes. It can generate a lot of output and should be removed or changed to a more appropriate debug level before merging.

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/src/ram.cpp

+  for (const auto& [role, name] : clock_gate_pins_) {
+    logger_->info(RAM,
+                  99,
+                  "Clock gate pin - Role: {}/{}, Name: {}",
+                  static_cast<int>(role.type),
+                  role.index,
+                  name);
+  }


This loop for logging clock gate pins appears to be for debugging. It should be removed before merging.

gemini-code-assist · 2026-03-30T04:33:52Z

src/ram/test/make_7x7_nangate45.tcl

+  -word_size 7 \
+  -num_words 7 \
+  -read_ports 1 \
+  -read_ports 1 \


The -read_ports argument is specified twice. This line is a duplicate and should be removed.

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-30T04:34:54Z

src/ram/include/ram/ram.h

+  // for map so that keys are comparable
+  bool operator<(const PinRole& other) const
+  {
+    return std::tie(type, index) < std::tie(other.type, other.index);


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

src/ram/include/ram/ram.h:9:

- #include <utility> + #include <tuple> + #include <utility>

github-actions · 2026-03-30T04:34:54Z

src/ram/include/ram/ram.h


 private:
  void findMasters();
+  std::map<PinRole, std::string> buildPinMap(odb::dbMaster*);


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

src/ram/include/ram/ram.h:7:

- #include <memory> + #include <map> + #include <memory>

github-actions · 2026-03-30T04:34:54Z

src/ram/src/ram.cpp

  return best;
 }

+std::map<PinRole, std::string> RamGen::buildPinMap(dbMaster* master)


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

src/ram/src/ram.cpp:10:

- #include <memory> + #include <map> + #include <memory>

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

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-30T04:43:56Z

src/ram/include/ram/ram.h

+  // for map so that keys are comparable
+  bool operator<(const PinRole& other) const
+  {
+    return std::tie(type, index) < std::tie(other.type, other.index);


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

src/ram/include/ram/ram.h:10:

- #include <utility> + #include <tuple> + #include <utility>

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

github-actions · 2026-03-30T16:10:51Z

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

github-actions · 2026-03-30T16:28:06Z

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

rovinski · 2026-03-30T05:26:12Z

src/ram/include/ram/ram.h

+  Ground
+};
+
+struct PinRole


I would change Pin --> Port in all cases as technically you are dealing with the logical port here.

rovinski · 2026-03-30T05:30:22Z

src/ram/test/CMakeLists.txt

  "ram"
  TESTS
    make_8x8
+    make_7x7_nangate45


Can you also update make_8x8 to make_8x8_sky130?

rovinski · 2026-03-31T05:43:19Z

src/ram/src/ram.cpp

+  }
+  delete port_iter;
+  return pin_map;
+}


Should have some error checking to avoid using badly defined cells. e.g.

Exactly 1 primary power

Exactly 1 primary ground

Exactly the number of pins which are expected for the given cell type

Or I guess I suppose you could check those things after the cell map has been made, if easier.

It doesn't need to be solved in this PR, but you should mark as a TODO item to check against various kinds of flip-flops to make sure invalid ones are flagged, e.g. scan flops, flip-flops with reset, etc. Anything that doesn't have only clk, D, and Q ports. There are occasionally some flip-flops that have a Qn (negated Q) output and those are ok, Qn is just left disconnected.

rovinski · 2026-03-31T05:48:52Z

src/ram/test/make_7x7_nangate45_behavioral.vok

+module RAM7x8 (
+  clk,
+  D,
+  Q,
+  addr_rw,
+  we
+);
+  input clk;
+  input [7:0] D;
+  output reg [7:0] Q;
+  input [2:0] addr_rw;
+  input [0:0] we;
+
+  // memory array declaration
+  reg [7:0] mem[0:6];
+
+  // write logic
+  integer i;
+  always @(posedge clk) begin
+    for (i = 0; i < 1; i = i + 1) begin
+      if (we[i]) begin
+        mem[addr_rw][i*8 +:8] <= D[i*8 +:8];
+      end
+    end
+  end
+
+  // read logic
+  always @(*) begin
+    Q = mem[addr_rw];
+  end
+
+endmodule


Gemini is correct here. Something not right with the behavioral model.

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

github-actions · 2026-03-31T23:19:14Z

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

github-actions · 2026-03-31T23:23:37Z

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

rovinski · 2026-04-01T00:05:35Z

src/ram/test/make_7x7_nangate45_behavioral.vok

It isn't sufficient to just change the vok file, if the verilog generator is wrong, then it needs to be fixed. How was the vok originally created?

The Verilog Generator was fine. The original file was generated before we added the bit masking, so it defaulted to 8 bits in the mask. The test has been changed so that it has 7 bits per mask; the dimensions have changed, but so have the input and output port lengths.

rovinski · 2026-04-01T00:07:52Z

src/ram/test/make_7x7_nangate45.tcl

+#
+# Test: ram/make_7x7_nangate45
+# Verifies that generate_ram works correctly with the Nangate45 technology
+# node using an intentionally odd 7-word x 1-byte (56-bit) configuration


Suggested change

# node using an intentionally odd 7-word x 1-byte (56-bit) configuration

# node using an intentionally odd 7-word x 7-bit (49-bit) configuration

rovinski · 2026-04-01T00:08:31Z

src/ram/test/make_7x7_nangate45.tcl

+# Verifies that generate_ram works correctly with the Nangate45 technology
+# node using an intentionally odd 7-word x 1-byte (56-bit) configuration
+# to exercise the decoder logic on a non-power-of-2 word count.
+# Addresses Issue #9468.


Don't need to include that here

Suggested change

# Addresses Issue #9468.

rovinski · 2026-04-01T00:09:38Z

src/ram/src/ram.cpp

  ground_net->setSigType(odb::dbSigType::GROUND);

-  // find a way to get the power and ground net names associated with cells used
+  // finds power and gorund of a cell if not given


Suggested change

// finds power and gorund of a cell if not given

// finds power and ground of a cell if not given

rovinski · 2026-04-01T00:13:25Z

src/ram/src/ram.cpp

+  if (!power_pin || power_pin[0] == '\0') {
+    power_pin = inv_ports_[{PortRoleType::Power, 0}].c_str();
+  }
+  if (!ground_pin || ground_pin[0] == '\0') {
+    ground_pin = inv_ports_[{PortRoleType::Ground, 0}].c_str();
+  }


This isn't technically sufficient. Although it would be really odd, it is still technically possible for other masters to have different power/ground names. A complete solution would be to iterate through all of the masters and collect the names into a std::set. Then, you would do a global connect on each member of the set.

99% of the time the set will only have 1 member, but it is good to handle all cases.

This is actually a separate item in #9392: "Add automatic detection of standard cell power/ground names"

It would be nice to keep that as a separate PR to only focus on one change.

Got it, I'll just keep the 'power_pin' and 'ground_pin' as required user-defined parameters now, and we can fix this in another PR.

github-actions · 2026-04-01T00:32:41Z

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

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

github-actions · 2026-04-01T05:48:52Z

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

rovinski · 2026-04-01T17:41:49Z

src/ram/test/make_7x7_nangate45.ok

+[WARNING ODB-0217] duplicate VIARULE (Via9Array-0) ignoring...
+[INFO ODB-0394] Duplicate site FreePDK45_38x28_10R_NP_162NW_34O in Nangate45 already seen in Nangate45_tech
+[INFO ODB-0227] LEF file: Nangate45/Nangate45.lef, created 135 library cells
+[WARNING RAM-0022] No tapcell is specified.


I think ng45 has a tapcell, right? If not, use an x1 fill cell as the tap cell.

rovinski · 2026-04-01T17:43:51Z

src/ram/test/make_7x7_nangate45.tcl

+  -ver_layer {metal4 0.28 8} \
+  -hor_layer {metal5 0.28 8} \


Does this work? I thought the metals needed to be the first 2 above the first metal layer.

We generally don't want to use above M3 if it can be helped.

I think Nangate45's density is just too high for us to route on metal2 and metal3. Any valid combination of pitch and width has resulted in violations with the detailed router. I will note that these pitch and width dimensions don't work when tapcells are inserted, so this test will probably change.

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

github-actions · 2026-04-03T22:24:49Z

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

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

github-actions · 2026-04-03T22:33:51Z

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

github-actions · 2026-04-04T14:46:57Z

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

braydenlouie · 2026-04-04T15:47:03Z

@maliberty @rovinski PR is ready for review

github-actions · 2026-04-09T16:36:53Z

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

maliberty · 2026-04-10T14:50:53Z

src/ram/include/ram/ram.h

+  // for map so that keys are comparable
+  bool operator<(const PortRole& other) const
+  {
+    return std::tie(type, index) < std::tie(other.type, other.index);
+  }


In c++20 you can use

auto operator<=>(const PortRole&) const = default;

and get all six operators

maliberty · 2026-04-10T14:58:56Z

src/ram/src/ram.cpp

+  if (!tri_enable_name.empty()) {
+    // find and remove it from DataIn


Why would a tristate enable appear as a DataIn? That doesn't appear possible in the prior loop.

maliberty · 2026-04-10T15:03:10Z

src/ram/src/ram.cpp

+                   master->getName(),
+                   ground_count);
+  }
+  delete port_iter;


avoid manual memory management with a smart pointer and no delete

std::unique_ptr port_iter = liberty->portIterator();

OpenROAD Contributor and others added 5 commits March 30, 2026 00:20

ram: changes to test files after word simplification

62711fe

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

ram: begin removal of hard-coded pin names

3d95c43

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

ram: removed hard-coded names and tested sky130

6a8b7a8

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

ram: preliminary testing with nangate testing complete

582763a

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

gemini-code-assist bot reviewed Mar 30, 2026

View reviewed changes

github-actions bot reviewed Mar 30, 2026

View reviewed changes

ram: fixes to initial gemini advice

e9d4890

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

github-actions bot reviewed Mar 30, 2026

View reviewed changes

ram: clang-tidy change

30cc938

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

Merge branch 'The-OpenROAD-Project:master' into pdk-support

be3543f

rovinski reviewed Mar 31, 2026

View reviewed changes

braydenlouie added 4 commits March 31, 2026 19:10

ram: review changes

d495b3b

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

ram: change to vok file

7c8125a

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

ram: cmake list change

0dcdb68

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

ram: tclint change

132725e

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

rovinski reviewed Apr 1, 2026

View reviewed changes

Merge branch 'The-OpenROAD-Project:master' into pdk-support

3ca123e

braydenlouie added 2 commits April 1, 2026 00:34

ram: changes to nangate7x7 test

813287e

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

ram: reversion to user provided pwr/gnd pins

eb55fa6

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

rovinski reviewed Apr 1, 2026

View reviewed changes

ram: clang-format and ng45 test changes

1e6c537

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

ram: updated ng45 ok files and tclfmt

4ca740a

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

braydenlouie marked this pull request as ready for review April 4, 2026 01:06

Merge branch 'The-OpenROAD-Project:master' into pdk-support

57631b6

rovinski approved these changes Apr 4, 2026

View reviewed changes

Merge branch 'The-OpenROAD-Project:master' into pdk-support

9d02db1

github-actions bot added the size/XL label Apr 9, 2026

rovinski requested a review from maliberty April 9, 2026 18:35

maliberty requested changes Apr 10, 2026

View reviewed changes

	# node using an intentionally odd 7-word x 1-byte (56-bit) configuration
	# node using an intentionally odd 7-word x 7-bit (49-bit) configuration

	// finds power and gorund of a cell if not given
	// finds power and ground of a cell if not given

		if (!tri_enable_name.empty()) {
		// find and remove it from DataIn

Conversation

braydenlouie commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change

Impact

Verification

Related Issues

Uh oh!

braydenlouie commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 30, 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 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 30, 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 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rovinski Mar 31, 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!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

github-actions bot commented Mar 31, 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!

braydenlouie commented Mar 30, 2026 •

edited

Loading

braydenlouie commented Mar 30, 2026 •

edited

Loading

rovinski Mar 31, 2026 •

edited

Loading