ram: add nangate45 cross-platform regression test#9680
ram: add nangate45 cross-platform regression test#9680DeepanshuDadhich wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in the RAM generator's clock pin probing logic and adds a new regression test for the Nangate45 technology. The fix correctly extends the probe chain to include the "CK" pin, which is necessary for Nangate45 cells. The new test case is well-structured, exercising the fix with a non-power-of-2 word count to stress the decoder logic. The changes are sound and enhance the tool's reliability. I have one suggestion to improve the maintainability of the clock pin probing logic.
src/ram/src/ram.cpp
Outdated
| const char* clk_pin = "GATE"; | ||
| if (storage_cell_->findMTerm("CLK")) { | ||
| clk_pin = "CLK"; | ||
| } else if (storage_cell_->findMTerm("CK")) { | ||
| clk_pin = "CK"; | ||
| } |
There was a problem hiding this comment.
For better maintainability and extensibility, you could use a data-driven approach to find the clock pin. This makes it easier to add more possible clock pin names in the future without adding more else if statements.
const char* clk_pin = "GATE";
for (const char* name : {"CLK", "CK"}) {
if (storage_cell_->findMTerm(name)) {
clk_pin = name;
break;
}
}
src/ram/src/ram.cpp
Outdated
| // Resolve the clock-pin name for the storage cell. | ||
| // sky130hd DFFs use "CLK", Nangate45 DFFs use "CK", and some latch-type | ||
| // storage cells use "GATE". Check in priority order so the correct pin | ||
| // is always found regardless of the technology node. | ||
| const char* clk_pin = "GATE"; | ||
| if (storage_cell_->findMTerm("CLK")) { | ||
| clk_pin = "CLK"; | ||
| } else if (storage_cell_->findMTerm("CK")) { | ||
| clk_pin = "CK"; | ||
| } |
There was a problem hiding this comment.
Instead of setting a fixed pin name you should use the OpenSTA APIs to query for the clock pin. Which is populated by the liberty data.
You can turn the cell name into a LibertyCell then iterate all of its ports then check which pin is the clock pin.
https://github.com/parallaxsw/OpenSTA/blob/master/include/sta/Liberty.hh#L832
There was a problem hiding this comment.
got it...thanks for the suggestion..i look into it...
DeepanshuDadhich
force-pushed
the
fix/9468-nangate45-ram-test
branch
from
9d5f2c9 to
329659e
Compare
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>
DeepanshuDadhich
force-pushed
the
fix/9468-nangate45-ram-test
branch
from
329659e to
15b0301
Compare
|
Thanks for the helpful suggestions! I’ve updated the implementation based on the feedback. Instead of relying on hard-coded clock pin names, the code now queries the Liberty data through the OpenSTA API to determine the clock pin for the storage cell. This should make the detection logic more robust and technology-independent, without assuming specific pin names like I also reran the regression tests, including the Nangate45 test added in this PR, and verified that the RAM generation flow still works as expected. Please let me know if there’s anything else I should adjust or improve. |
|
|
||
| set def_file [make_result_file make_7x7_nangate45.def] | ||
| write_def $def_file | ||
| diff_files make_7x7_nangate45.defok $def_file |
rovinski
left a comment
rovinski
left a comment
There was a problem hiding this comment.
Please include layout screenshots to verify that the output looks correct
| -routing_layer {metal1 0.07} \ | ||
| -ver_layer {metal2 0.07 0.42} \ | ||
| -hor_layer {metal3 0.07 0.42} \ |
There was a problem hiding this comment.
How were these values determined?
| -ver_layer {metal2 0.07 0.42} \ | ||
| -hor_layer {metal3 0.07 0.42} \ | ||
| -filler_cells {FILLCELL_X1 FILLCELL_X2 FILLCELL_X4 FILLCELL_X8} \ | ||
| -write_behavioral_verilog $behavioral_file |
There was a problem hiding this comment.
I don't think we need to test the behavioral flag. This really isn't significantly different from the 8x8 test.
|
|
||
| set def_file [make_result_file make_7x7_nangate45.def] | ||
| write_def $def_file | ||
| diff_files make_7x7_nangate45.defok $def_file |
There was a problem hiding this comment.
I don't think .vok is needed for this test
|
@DeepanshuDadhich I would kindly ask that you not take issues which are already assigned to others as mentioned in #9468. There are many many other issues not yet being worked on in #9392 which you could request assignment on. One of the main reasons is that @braydenlouie and I discussed this and we decided to pause this while waiting on #9690 which will probably affect this PR significantly. |
4 participants
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 #9468.