Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/ram/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ generate_ram [-mask_size bits]
[-storage_cell name]
[-tristate_cell name]
[-inv_cell name]
-power_pin name
-ground_pin name
[-power_net_name name]
[-ground_net_name name]
-routing_layer config
Expand All @@ -66,8 +64,6 @@ generate_ram [-mask_size bits]
| `-storage_cell` | Name of the master to use for the storage device (i.e. a flip-flop). Must be positive-edge triggered. Default: auto-select from the loaded cell library. |
| `-tristate_cell` | Name of the master to use for the tristate device (i.e. a tristate inverter). It is currently assumed that the device is inverting. Default: auto-select from the loaded cell library. |
| `-inv_cell` | Name of the master to use for inverters. Default: auto-select from the loaded cell library. |
| `-power_pin` | Name of the power pin in each standard cell used. Only one name is currently supported. |
| `-ground_pin` | Name of the ground pin in each standard cell used. Only one name is currently supported. |
| `-routing_layer` | A list of the metal layer and metal width (in microns) for generating standard cell power tracks (followpins). Example: `{met1 0.48}`. |
| `-power_net_name` | Name of the power net to create. Default: `VDD`. |
| `-ground_net_name` | Name of the ground net to create. Default: `VSS`. |
Expand Down
8 changes: 5 additions & 3 deletions src/ram/include/ram/ram.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <functional>
#include <map>
#include <memory>
#include <set>
#include <string>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -99,9 +100,7 @@ class RamGen
odb::dbMaster* tapcell,
int max_tap_dist);

void ramPdngen(const char* power_pin,
const char* ground_pin,
const char* power_net_name,
void ramPdngen(const char* power_net_name,
const char* ground_net_name,
const char* route_name,
int route_width,
Expand Down Expand Up @@ -203,6 +202,9 @@ class RamGen
odb::dbMaster* latch_cell_{nullptr};
odb::dbMaster* tapcell_{nullptr};

std::set<std::string> power_pin_names_;
std::set<std::string> ground_pin_names_;

std::map<PortRole, std::string> storage_ports_;
std::map<PortRole, std::string> tristate_ports_;
std::map<PortRole, std::string> inv_ports_;
Expand Down
49 changes: 44 additions & 5 deletions src/ram/src/ram.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,42 @@ std::map<PortRole, std::string> RamGen::buildPortMap(dbMaster* master)
ground_count);
}

for (auto& [role, name] : pin_map) {
if (role.type == PortRoleType::Power) {
power_pin_names_.insert(name);
} else if (role.type == PortRoleType::Ground) {
ground_pin_names_.insert(name);
}
}

Comment on lines +585 to +592
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this guarantee that only one primary power is used? What if there are two different power pins found? We would want to error in that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a error check in baf136e, ready for review

if (power_pin_names_.size() > 1) {
std::string names;
for (const auto& name : power_pin_names_) {
if (!names.empty()) {
names += ", ";
}
names += name;
}
logger_->error(RAM,
42,
"Multiple primary power pin names detected across cells: {}",
names);
}
if (ground_pin_names_.size() > 1) {
std::string names;
for (const auto& name : ground_pin_names_) {
if (!names.empty()) {
names += ", ";
}
names += name;
}
logger_->error(
RAM,
43,
"Multiple primary ground pin names detected across cells: {}",
names);
}

Comment on lines +593 to +620
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misunderstand, you want to error if there are multiple power pins within the same cell, not across cells.

Supporting different pin names across cells is exactly the reason to use a set.

return pin_map;
}

Expand Down Expand Up @@ -846,9 +882,7 @@ void RamGen::findMasters()
}
}

void RamGen::ramPdngen(const char* power_pin,
const char* ground_pin,
const char* power_net_name,
void RamGen::ramPdngen(const char* power_net_name,
const char* ground_net_name,
const char* route_name,
int route_width,
Expand Down Expand Up @@ -913,8 +947,13 @@ void RamGen::ramPdngen(const char* power_pin,
ground_net->setSpecial();
ground_net->setSigType(odb::dbSigType::GROUND);

block_->addGlobalConnect(nullptr, ".*", power_pin, power_net, true);
block_->addGlobalConnect(nullptr, ".*", ground_pin, ground_net, true);
for (const auto& pin_name : power_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), power_net, true);
}

for (const auto& pin_name : ground_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), ground_net, true);
}

block_->globalConnect(false, false);
Comment on lines +950 to 958
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Since RamGen is a persistent object in the OpenROAD session (retrieved via ord::getRamGen()), the member variables power_pin_names_ and ground_pin_names_ will accumulate pin names across multiple runs of generate_ram in the same session. This can lead to incorrect global connections or errors if different libraries or designs are processed sequentially.

To prevent this state leakage, clear both sets at the end of ramPdngen once the global connections have been established.

Suggested change
for (const auto& pin_name : power_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), power_net, true);
}
for (const auto& pin_name : ground_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), ground_net, true);
}
block_->globalConnect(false, false);
for (const auto& pin_name : power_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), power_net, true);
}
for (const auto& pin_name : ground_pin_names_) {
block_->addGlobalConnect(nullptr, ".*", pin_name.c_str(), ground_net, true);
}
block_->globalConnect(false, false);
power_pin_names_.clear();
ground_pin_names_.clear();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is added in 382d8c7

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary because you would not be creating more than one RAM per session. OpenROAD doesn't support operating on more than one design and likely won't in the future.


Expand Down
5 changes: 2 additions & 3 deletions src/ram/src/ram.i
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,13 @@ generate_ram_netlist_cmd(int mask_size,
max_tap_dist);
}

void ram_pdngen(const char* power_pin, const char* ground_pin,
const char* power_net_name, const char* ground_net_name,
void ram_pdngen(const char* power_net_name, const char* ground_net_name,
const char* route_name, int route_width,
const char* ver_name, int ver_width, int ver_pitch,
const char* hor_name, int hor_width, int hor_pitch)
{
RamGen* ram_gen = ord::getRamGen();
ram_gen->ramPdngen(power_pin, ground_pin, power_net_name, ground_net_name,
ram_gen->ramPdngen(power_net_name, ground_net_name,
route_name, route_width,
ver_name, ver_width, ver_pitch,
hor_name, hor_width, hor_pitch);
Expand Down
22 changes: 4 additions & 18 deletions src/ram/src/ram.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ sta::define_cmd_args "generate_ram" {[-mask_size bits]
[-storage_cell name]
[-tristate_cell name]
[-inv_cell name]
-power_pin name
-ground_pin name
[-power_net_name name]
[-ground_net_name name]
-routing_layer config
Expand All @@ -120,9 +118,9 @@ proc generate_ram { args } {
sta::parse_key_args "generate_ram" args \
keys { -mask_size -word_size -num_words -column_mux_ratio
-storage_cell -tristate_cell -inv_cell -read_ports -use_latch
-power_pin -ground_pin -power_net_name -ground_net_name
-routing_layer -ver_layer -hor_layer -filler_cells
-tapcell -max_tap_dist -write_behavioral_verilog } flags {}
-power_net_name -ground_net_name -routing_layer -ver_layer
-hor_layer -filler_cells -tapcell -max_tap_dist
-write_behavioral_verilog } flags {}

sta::check_argc_eq0 "generate_ram" $args

Expand Down Expand Up @@ -180,18 +178,6 @@ proc generate_ram { args } {

ord::design_created

if { [info exists keys(-power_pin)] } {
set power_pin $keys(-power_pin)
} else {
utl::error RAM 5 "The -power_pin argument must be specified."
}

if { [info exists keys(-ground_pin)] } {
set ground_pin $keys(-ground_pin)
} else {
utl::error RAM 6 "The -ground_pin argument must be specified."
}

set power_net_name "VDD"
if { [info exists keys(-power_net_name)] } {
set power_net_name $keys(-power_net_name)
Expand Down Expand Up @@ -259,7 +245,7 @@ proc generate_ram { args } {
utl::error RAM 18 "The -filler_cells argument must be specified."
}

ram::ram_pdngen $power_pin $ground_pin $power_net_name $ground_net_name \
ram::ram_pdngen $power_net_name $ground_net_name \
$route_name $route_width \
$ver_name $ver_width $ver_pitch $hor_name $hor_width $hor_pitch

Expand Down
2 changes: 0 additions & 2 deletions src/ram/test/make_7x7_nangate45.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ generate_ram \
-num_words 7 \
-read_ports 1 \
-storage_cell DFF_X1 \
-power_pin VDD \
-ground_pin VSS \
-routing_layer {metal1 0.08} \
-ver_layer {metal4 0.14 9} \
-hor_layer {metal3 0.08 8} \
Expand Down
2 changes: 0 additions & 2 deletions src/ram/test/make_8x8_latch_sky130.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ generate_ram \
-num_words 8 \
-read_ports 1 \
-use_latch 1 \
-power_pin VPWR \
-ground_pin VGND \
-routing_layer {met1 0.48} \
-ver_layer {met2 0.48 40} \
-hor_layer {met3 0.48 20} \
Expand Down
2 changes: 0 additions & 2 deletions src/ram/test/make_8x8_mux2_sky130.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ generate_ram \
-column_mux_ratio 2 \
-read_ports 1 \
-storage_cell sky130_fd_sc_hd__dfxtp_1 \
-power_pin VPWR \
-ground_pin VGND \
-routing_layer {met1 0.48} \
-ver_layer {met2 0.48 20} \
-hor_layer {met3 0.48 10} \
Expand Down
2 changes: 0 additions & 2 deletions src/ram/test/make_8x8_mux4_sky130.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ generate_ram \
-column_mux_ratio 4 \
-read_ports 1 \
-storage_cell sky130_fd_sc_hd__dfxtp_1 \
-power_pin VPWR \
-ground_pin VGND \
-routing_layer {met1 0.48} \
-ver_layer {met2 0.48 10} \
-hor_layer {met3 0.48 7} \
Expand Down
2 changes: 0 additions & 2 deletions src/ram/test/make_8x8_sky130.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ generate_ram \
-num_words 8 \
-read_ports 1 \
-storage_cell sky130_fd_sc_hd__dfxtp_1 \
-power_pin VPWR \
-ground_pin VGND \
-routing_layer {met1 0.48} \
-ver_layer {met2 0.48 40} \
-hor_layer {met3 0.48 20} \
Expand Down
Loading