web: add a Clusters panel and a cluster coloring overlay - #11122
web: add a Clusters panel and a cluster coloring overlay#11122jorge-ferreira-pii wants to merge 4 commits into
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 introduces a new 'Clusters' panel in the web viewer to visualize the dbGroup hierarchy, along with a 'Find' dialog for batch selecting and highlighting objects by name pattern. It adds the ClustersWidget frontend component, the _clusters synthetic tile layer, and backend support via GroupReport and new WebSocket endpoints, while refactoring the module hierarchy coloring logic to share a common effective-color rule. The review feedback suggests optimizing GroupReport::getReport by reserving capacity for result.nodes to avoid multiple reallocations, and adding validation in find_objects to ensure the highlight_group index is not less than -1.
| if (!block_) { | ||
| return result; | ||
| } | ||
|
|
||
| // dbBlock::getGroups() is the flat group table, so child groups appear | ||
| // in it too — recurse only from the roots to build the tree once. | ||
| int next_id = 0; |
There was a problem hiding this comment.
Reserving capacity for result.nodes based on the total number of groups in the block avoids multiple reallocations and overhead during the recursive addGroup calls.
if (!block_) {
return result;
}
result.nodes.reserve(block_->getGroups().size());
// dbBlock::getGroups() is the flat group table, so child groups appear
// in it too — recurse only from the roots to build the tree once.
int next_id = 0;| if (highlight_group >= gui::kNumHighlightSet) { | ||
| throw std::runtime_error("Invalid highlight group: " | ||
| + std::to_string(highlight_group)); | ||
| } |
There was a problem hiding this comment.
Validate that highlight_group is not less than -1. Negative values other than -1 (which represents selecting without highlighting) are invalid and should be rejected to prevent unexpected behavior.
| if (highlight_group >= gui::kNumHighlightSet) { | |
| throw std::runtime_error("Invalid highlight group: " | |
| + std::to_string(highlight_group)); | |
| } | |
| if (highlight_group < -1 || highlight_group >= gui::kNumHighlightSet) { | |
| throw std::runtime_error("Invalid highlight group: " | |
| + std::to_string(highlight_group)); | |
| } |
4c10c30 to
e3eba62
Compare
MPL writes its physical hierarchy into the database as nested dbGroups
when `rtl_macro_placer -keep_clustering_data` runs, but nothing could
read it back: the soft-macro partitioning was only visible in the
annealer's own debug graphics, while it ran.
This adds the viewer side of it:
- `_clusters` tile layer, which colors every instance by the dbGroup it
belongs to, with optional bounding-box outlines per cluster. It is
driven by the `cluster_view`/`cluster_outlines` display options, so
`save_image -web` and `web_save_report` plot it headless too.
- A Clusters panel listing the tree, with a checkbox and a color swatch
per cluster. Collapsing a cluster paints its whole subtree in its own
color; clicking a row isolates that cluster — only its instances, and
those of its nested clusters, stay colored, which is how a cluster is
located in a design too large for the selection highlight (that path
coarsens past kMaxHighlightShapes; the tile layer has no such cap, so
`select_group` takes a `no_highlight` flag for it).
- `group_report.{h,cpp}`, mirroring hierarchy_report: it walks the group
tree once and both the panel and the headless renderers read from it,
so the saved image cannot disagree with the viewer. The collapse ->
color rule lives in one place (computeEffectiveOwnerColors, shared
with the module hierarchy; color-tree.js on the client).
Along the way the two color overlays (`_modules`, `_clusters`) became a
table (ColorOverlaySpec) instead of two hardcoded paths, and the tile
layers are now gated on the visibility flag they already send in the
payload rather than on being mounted in the map — mounting was a second
record of the flag, and once the two drifted the toggle stopped working
until a page reload.
Writing the clustering data no longer takes instances away from groups
somebody else owns. A dbInst belongs to exactly one dbGroup and
dbGroup::addInst silently moves it, so a UPF power domain or a placement
region would have lost its members to the clusters -- and dpl builds its
regions from dbGroup::getInsts, so the placer would simply stop seeing
them. Those instances now stay where they are and are reported once
(MPL-0078); the assert that used to stand for this precondition only held
in debug builds.
Also adds a Find dialog (`find_objects`) over the descriptor registry,
which is what makes a named cluster reachable without the Tcl console.
Part of The-OpenROAD-Project#7959
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
e3eba62 to
dfb7cc1
Compare
|
Before digging into the code, I think that there are some conceptual things to address here:
@mguthaus Would you agree? |
|
On #2, I agree with @AcKoucher suggestion. |
|
The tree views now share one tab, picked from a dropdown in the toolbar, and the cluster bounding boxes are gone from the layout viewer: the per-instance coloring already shows where a cluster is. Also shortens this feature's code comments. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
@mguthaus For 1, just beware that this is not the bbox of the std cell cluster that MPL produced, it's just the bbox made based on the instances placed by GPL. That's why I think it's not that useful. |
|
Oh, in that case, definitely don't include it. |
|
@AcKoucher @mguthaus I added the bounding boxes to visually encapsulate where all instances of a given cluster end up. Without them, it's easy to miss instances that drifted away from the cluster during global placement. That said, I could consider adding an option to toggle the bounding boxes on or off. Or just take them off. |
|
If the box just encloses the instances then it doesn't really show "instances that drifted away". To know that you would be the box that clustering used. |
makeResizableHeaders measured the headers of a table that is not laid out, so re-rendering the inactive view locked every column at zero width. Discard a measurement that comes out all zeros, and let the panel defer the re-render of a hidden view until it is shown. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Drop the sentinel in makeResizableHeaders and test it directly instead of through the panel, and remove app.hierarchyBrowser, which no longer has any reader. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
AcKoucher
left a comment
AcKoucher
left a comment
There was a problem hiding this comment.
I think the names in the drop-down menu are a bit ambiguous.
As I said here, I think it'd be clearer to have "Verilog" (or "Verilog Modules") and "Instance Groups" as the names of the entries in the menu.
Apart from that, as a style nit, I also think it'd be nice to have a field "Source: " before the menu, as we have on the Charts widget:
| if (instances_kept_by_others > 0) { | ||
| logger_->warn( | ||
| MPL, | ||
| 78, | ||
| "{} instances are missing from the clustering data because " | ||
| "they already belong to another group (a power domain, a " | ||
| "region). They keep it: an instance can only be in one group, " | ||
| "and taking it would change what the placer sees.", | ||
| instances_kept_by_others); | ||
| } |
There was a problem hiding this comment.
I disagree with mixing VISUAL_DEBUG with other groups. If we want to make this MPL feature work when other groups exist in ODB, it's probably a separate scope and should have a separate PR for the new approach. Please, keep this PR WEB-only.
5 participants
rtl_macro_placer can run with the -keep_clustering_data flag (which is off by default), so the physical hierarchy that its clustering engine builds is kept in the database as dbGroups and stays inspectable after the run: The Web GUI Hierarchy Browser panel lists the clusters, and save_image -web -display_option {cluster_view true} plots it headless.
Part of issue #7959