rsz: Add bottleneck analysis developer command#9464
rsz: Add bottleneck analysis developer command#9464openroad-ci wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: Martin Povišer <povik@cutebit.org>
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request introduces a new bottleneck analysis feature, which is a valuable addition for performance tuning. The implementation is well-structured, and the use of a simplified timing model is a clever approach. I've identified a critical bug that could lead to a crash, along with a couple of other high-severity issues in the core mathematical logic that should be addressed.
| Edge* prev_edge = prevEdge(edge->from(graph_)); | ||
| Vertex* prev_drvr = prev_edge->from(graph_); |
There was a problem hiding this comment.
prevEdge(edge->from(graph_)) can return nullptr if the fanin vertex edge->from(graph_) is not driven (e.g., a floating input pin on an instance). In that case, the subsequent line Vertex* prev_drvr = prev_edge->from(graph_); will cause a crash due to null pointer dereferencing. You should add a check to ensure prev_edge is not null before using it.
For example:
Edge* prev_edge = prevEdge(edge->from(graph_));
if (!prev_edge) {
continue;
}
Vertex* prev_drvr = prev_edge->from(graph_);| } | ||
| } | ||
|
|
||
| alpha /= std::numbers::ln2; |
There was a problem hiding this comment.
The scaling of alpha appears to be incorrect. The logsumexp function uses base 2 (log2/exp2), while the bottleneck formula log[ sum exp(-p_slack / alpha) ] uses the natural logarithm (log/exp). To convert exp(x / alpha) to base 2, it becomes exp2(x / (alpha * ln(2))). Therefore, the alpha used in the base 2 calculations should be alpha_user * ln(2). The current code divides by ln(2) instead of multiplying.
alpha *= std::numbers::ln2;| pin.fwd_accumulator = -INF; | ||
| for (auto [fi, arc_delay] : pin.fanins) { | ||
| double fanin_fwd_accumulator | ||
| = fi >= 0 ? driver_pins_[fi].fwd_accumulator : 1.0; |
There was a problem hiding this comment.
The fwd_accumulator values are in log-space. For a path startpoint, the forward path count is 1, and its log-space value should be log2(1) = 0. The fallback value of 1.0 for fanin_fwd_accumulator is incorrect as it corresponds to a path count of exp2(1.0) = 2. This should be 0.0 to correctly represent a startpoint.
= fi >= 0 ? driver_pins_[fi].fwd_accumulator : 0.0;| - _6_/Z (metric 0.713) | ||
| - _5_/Z (metric 0.711) | ||
| - _10_/ZN (metric 0.639) | ||
| - _8_/ZN (metric 0.639) |

Adds a
rsz::report_bottlenecks -alpha <value> -npins <value>developer command to exercise bottleneck analysis.See the added test and code for more detail