Skip to content

[CUS-517] - Improvements to clkgate register dumping#50

Merged
akashlevy merged 12 commits intomainfrom
icg
Apr 29, 2026
Merged

[CUS-517] - Improvements to clkgate register dumping#50
akashlevy merged 12 commits intomainfrom
icg

Conversation

@stanminlee
Copy link
Copy Markdown

@stanminlee stanminlee commented Apr 27, 2026

Previous implementation would accidentally flag flops that weren't actually gated

Now, on every vertex visit that is a clock edge (BFS)

  1. Look at all edges flowing into the vertex
  2. Skip any pins that aren't tagged by clock network
  3. Determine if the pin is gated (based on prev iterations)
  4. If at least one path to the node is gated, then we know the vertex is gated
  5. repeat for all nodes in the clock network, which stops at Flip-flops

Afterwards, we know if the flops are gated based on this attribute in a O(1) lookup.

I also changed how clock gates are identified because the liberty standard isn't always enforced. To be qualified as a clock gating cell, the cell must follow the following conditions:

  1. Pin must have a "clock_gate_enable_pin : true"
  2. Pin must have a "clock_gate_clock_pin : true"
  3. Have "clock_gating_integrated_cell : (something not none)"

Additionally, when determining if the ICG is logically gating something, we check if it is tied to a constant on the functional enable pin as a final check.

This prevents false flags from popping up in the case that a cell doesn't follow liberty standards

@linear
Copy link
Copy Markdown

linear Bot commented Apr 27, 2026

CUS-517

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 27, 2026

Greptile Summary

This PR replaces the previous on-the-fly fanout-traversal approach to clock-gated register detection with a BFS-propagated clk_gated_ cache, and tightens the isClockGate() predicate in Liberty to require both clock_gate_clock_pin and clock_gate_enable_pin attributes to be explicitly true. The refactor eliminates false-positive gated-register reports and reduces the per-call cost of clockGatedRegisters() to an O(1) cache lookup.

  • updateClkGates in Search.cc dereferences from->pin() for every predecessor (lines 1042–1048) without a null guard, while isClkGateInstance in the same file explicitly handles a null pin. If a vertex in the clock network has a null pin, this path will invoke UB.

Confidence Score: 4/5

Safe to merge after addressing the from->pin() null-dereference in updateClkGates; all other findings are style/P2.

One P1 finding: from->pin() used without a null check in updateClkGates while the sibling function isClkGateInstance explicitly guards against null pins. The Liberty tightening and O(1) cache approach are well-structured; remaining P2s are style and documentation concerns that do not affect correctness.

search/Search.cc — specifically updateClkGates (predecessor pin null-check) and isClkGateInstance (raw iterator pointer).

Important Files Changed

Filename Overview
search/Search.cc Adds isClkGateInstance, updateClkGates, and isClkGated for BFS-based clock-gate propagation; clk_gated_ vector initialized in findClkArrivals and findArrivalsSeed. Has a P1 missing null-check for from->pin() in updateClkGates, plus P2 style issues (raw pointer, eager debug-string computation, missing doc comment).
search/Sta.cc Replaces on-the-fly fanout traversal with O(1) clk_gated_ cache lookup in clockGatedRegisters; adds isClkGatedRegister. Logic is clean and consistent with new BFS scheme; std::unique_ptr used correctly for iterators.
liberty/LibertyReader.cc Reads the boolean value of clock_gate_clock_pin and clock_gate_enable_pin attributes before calling setHasClkGateClkPin/setHasClkGateEnablePin, so cells with explicit false values are no longer mis-classified.
include/sta/Search.hh Declares clk_gated_ as std::vector<char> and exposes isClkGated, updateClkGates, and isClkGateInstance. The char element comment could be clearer (not about unique addresses, but about avoiding vector<bool> bitpacking).
liberty/Liberty.cc Initializes has_clk_gate_clk_pin_ and has_clk_gate_enable_pin_ to false in constructor; tightens isClockGate() to require both flags. Clean change.
include/sta/Liberty.hh Adds has_clk_gate_clk_pin_ / has_clk_gate_enable_pin_ fields and inline setters to LibertyCell. Straightforward header-only additions.
include/sta/Sta.hh Adds isClkGatedRegister public method declaration. Trivial change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["findClkArrivals() / findArrivalsSeed()"] -->|"init clk_gated_ vector (size = vertexCount+1, all 0)"| B["BFS: ArrivalVisitor::visit(vertex)"]
    B -->|"isClock(vertex)?"| C{Clock vertex?}
    C -->|No| B
    C -->|Yes| D["updateClkGates(vertex)"]
    D --> E["isClkGateInstance(vertex)?"]
    E -->|"cell->isClockGate() && enable not tied to constant"| F["gated = true"]
    E -->|No| G["Iterate VertexInEdgeIterator"]
    G --> H{"isClock(from)?"}
    H -->|No - skip| G
    H -->|Yes| I{"clk_gated_[id(from)] != 0?"}
    I -->|Yes| F
    I -->|No| G
    F --> J["clk_gated_[id] = 1"]
    G -->|exhausted| K["clk_gated_[id] = 0"]
    J --> B
    K --> B
    B -->|BFS complete| L["clockGatedRegisters()"]
    L --> M["leafInstanceIterator - skip non-registers and clock gates"]
    M --> N["isClkGatedRegister(inst)"]
    N --> O["pinLoadVertex for each reg clk pin"]
    O --> P{"isClkGated(vertex)?"}
    P -->|Yes| Q["Add inst to result"]
    P -->|No| M
Loading

Reviews (3): Last reviewed commit: "check if the port is tied to a constant" | Re-trigger Greptile

Comment thread search/Sta.cc
Comment thread include/sta/Search.hh
Comment thread search/Search.cc
Comment thread search/Sta.cc
@stanminlee
Copy link
Copy Markdown
Author

@greptile please review again

@stanminlee stanminlee requested a review from akashlevy April 28, 2026 03:42
Comment thread liberty/LibertyReader.cc
@akashlevy
Copy link
Copy Markdown

@stanminlee can you look at the greptile comments?

@akashlevy
Copy link
Copy Markdown

Marking as draft since we are not sure this actually solves the issue...

@akashlevy akashlevy marked this pull request as draft April 28, 2026 19:21
@stanminlee stanminlee marked this pull request as ready for review April 29, 2026 01:08
@stanminlee
Copy link
Copy Markdown
Author

@greptile take another look

Comment thread search/Search.cc Outdated
@akashlevy akashlevy merged commit b27cbbb into main Apr 29, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants