rsz: exclude delay/clock-delay cells from data buffering (#10622)#10719
rsz: exclude delay/clock-delay cells from data buffering (#10622)#10719saurav-fermions wants to merge 1 commit into
Conversation
…D-Project#10622) buffer_ports and repair_design were selecting clock-delay buffers (e.g. sky130_fd_sc_hd__clkdlybuf4s25_1) and dedicated delay cells (*dlygate*, *dlymetal*, gf180 dly*) as general data buffers. getBufferUse() classified a cell as CLOCK only via the is_clock_cell attribute, a user pattern, or the default "CLKBUF" name substring, so clkdlybuf/delay cells fell through as DATA. In sky130 these carry the same "buf" footprint as real data buffers, so findBuffers() kept them and ranked them into the data-buffer candidate set. Add a BufferUse::DELAY classification and an isDelayCell() helper, and prune DELAY cells from the data-buffer candidate set in findBuffers(). getBufferList() still only excludes CLOCK, so delay cells remain available to hold fixing (RepairHold), which legitimately uses them. Add buffer_ports_data_only regression (CMake + Bazel) and update sky130hd/gf180 buffer reports and affected slew/fanout goldens. Signed-off-by: Saurav Singh <saurav.singh@fermions.co>
![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 prevents dedicated delay cells and clock-delay buffers from being selected for general-purpose data buffering. It introduces a new BufferUse::DELAY enum and the isDelayCell helper to identify these cells by name or footprint patterns, updating several tests and adding a new regression test. Feedback on the changes suggests replacing the substring checks for footprints with exact case-insensitive comparisons to avoid redundant checks and prevent false positives on common substrings like "del" matching "model" or "delta".
| const std::string& footprint = buffer->footprint(); | ||
| if (!footprint.empty() | ||
| && (containsIgnoreCase(footprint, "delay") | ||
| || containsIgnoreCase(footprint, "DEL") | ||
| || containsIgnoreCase(footprint, "DLY"))) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The check containsIgnoreCase(footprint, "delay") is completely redundant because "delay" contains "del" (case-insensitive), which is already matched by containsIgnoreCase(footprint, "DEL").
Furthermore, performing a generic substring match with "DEL" is highly risky and prone to false positives, as it will match any footprint containing "del" (such as "model", "delta", etc.).
Since standard cell footprints are typically exact generic names (e.g., "delay", "DEL", "DLY"), it is much safer and cleaner to use exact case-insensitive comparisons (e.g., using strcasecmp) to avoid these false positives.
const std::string& footprint = buffer->footprint();
if (!footprint.empty()
&& (strcasecmp(footprint.c_str(), "delay") == 0
|| strcasecmp(footprint.c_str(), "del") == 0
|| strcasecmp(footprint.c_str(), "dly") == 0)) {
return true;
}1 participant
Summary
buffer_ports/repair_designwere selecting clock-delay buffers (*clkdlybuf*) and dedicated delay cells (*dlygate*,*dlymetal*, gf180dly*) as general-purpose data buffers. This restores filtering so the data-buffer candidate set excludes delay/clock-delay cells.Type of Change
Impact
Data buffering no longer uses delay cells; affected golden outputs updated.
Verification
ctest -R '^rsz\.'238/238; real fail/pass.Related Issues
Fixes #10622
Developed with SAIGE, Fermions' autonomous RTL/EDA debugging agent; root-caused, tested, and signed off by the submitter (@saurav-fermions).