Conversation
Greptile SummaryThis PR introduces an
Confidence Score: 4/5The change is safe to merge as-is; the fallback logic is correct for the cases it handles, and the new public accessor in Power.hh is straightforward. The core logic is sound and the fallback is correct for the cases it handles. The mixed indentation and missing comment reduce readability but do not affect correctness. The null-port exclusion is a design gap worth confirming as intentional. power/Power.cc deserves a second look: the indentation should be normalized to tabs, a comment should be added to the fallback block, and the null-port exclusion should be confirmed as intentional. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[visit vertex] --> B{hasUserActivity?}
B -->|yes| C[setActivityCheck userActivity]
B -->|no| D{isLoad pin?}
D -->|yes| E{Has incoming wire edge?}
E -->|yes| F[setActivityCheck propagated activity]
E -->|no| G[No activity set]
D -->|no| H{isDriver pin?}
H -->|yes| I{port not null?}
I -->|no| J[No activity set]
I -->|yes| K[annotated = false]
K --> L{func not null?}
L -->|yes| M[evalActivity from func, setActivityCheck pin, annotated = true]
L -->|no| N{isClockGateOut?}
M --> N
N -->|yes| O{enable and clk and gclk found?}
O -->|yes| P[setActivityCheck gclk with gate formula, annotated = true]
O -->|no| Q{annotated?}
P --> Q
N -->|no| Q
Q -->|no| R[setActivityCheck pin with inputActivity defaulted]
Q -->|yes| S[Done]
Reviews (1): Last reviewed commit: "better way" | Re-trigger Greptile |
| @@ -521,8 +523,16 @@ PropActivityVisitor::visit(Vertex *vertex) | |||
| network_->pathName(gclk), | |||
| activity.density(), | |||
| activity.duty()); | |||
| annotated = true; | |||
| } | |||
| } | |||
| if (!annotated) { | |||
| PwrActivity default_act = power_->inputActivity(); | |||
| PwrActivity activity(default_act.density(), | |||
| default_act.duty(), | |||
| PwrActivityOrigin::defaulted); | |||
| changed = setActivityCheck(pin, activity); | |||
| } | |||
There was a problem hiding this comment.
Inconsistent indentation mixes tabs and spaces
The new lines (bool annotated = false;, annotated = true; inside the func block, and the entire if (!annotated) block) use spaces, while the surrounding code consistently uses tabs. This will render incorrectly in any tab-width-aware viewer and conflicts with the project's existing style. All new lines should use tabs at the same nesting depth as their enclosing block.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
followed what is reflected in the code
There was a problem hiding this comment.
Yeah this is fine, we will refactor this when we update to OpenSTA 3.0 (which @donn is working on)
| @@ -521,8 +523,16 @@ PropActivityVisitor::visit(Vertex *vertex) | |||
| network_->pathName(gclk), | |||
| activity.density(), | |||
| activity.duty()); | |||
| annotated = true; | |||
| } | |||
| } | |||
| if (!annotated) { | |||
| PwrActivity default_act = power_->inputActivity(); | |||
| PwrActivity activity(default_act.density(), | |||
| default_act.duty(), | |||
| PwrActivityOrigin::defaulted); | |||
| changed = setActivityCheck(pin, activity); | |||
| } | |||
There was a problem hiding this comment.
Default fallback only applies when
port is non-null
annotated is declared and the if (!annotated) default is evaluated only inside the if (port) block. Driver pins for which network_->libertyPort(pin) returns null (no liberty port at all) receive neither a computed activity nor the inputActivity() default — the same as before this PR. If the goal is to ensure all unresolved driver pins get a default, the null-port case is silently excluded. If the exclusion is intentional (e.g., non-liberty pins are handled elsewhere), a comment stating that would help.
| @@ -521,8 +523,16 @@ PropActivityVisitor::visit(Vertex *vertex) | |||
| network_->pathName(gclk), | |||
| activity.density(), | |||
| activity.duty()); | |||
| annotated = true; | |||
| } | |||
| } | |||
| if (!annotated) { | |||
| PwrActivity default_act = power_->inputActivity(); | |||
| PwrActivity activity(default_act.density(), | |||
| default_act.duty(), | |||
| PwrActivityOrigin::defaulted); | |||
| changed = setActivityCheck(pin, activity); | |||
| } | |||
There was a problem hiding this comment.
Yeah this is fine, we will refactor this when we update to OpenSTA 3.0 (which @donn is working on)
| @@ -521,8 +523,16 @@ PropActivityVisitor::visit(Vertex *vertex) | |||
| network_->pathName(gclk), | |||
| activity.density(), | |||
| activity.duty()); | |||
| annotated = true; | |||
| } | |||
| } | |||
| if (!annotated) { | |||
| PwrActivity default_act = power_->inputActivity(); | |||
| PwrActivity activity(default_act.density(), | |||
| default_act.duty(), | |||
| PwrActivityOrigin::defaulted); | |||
| changed = setActivityCheck(pin, activity); | |||
| } | |||
Added boolean to check whether a pin was annotated during visit function on the Activity propagator.
If it was not annotated (by either branch) we will default the activity to the defaults calculated by library/setting from user