sta: ensure correct connectivity in modnets#10322
sta: ensure correct connectivity in modnets#10322gadfort wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: Peter Gadfort <gadfort@zeroasic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Peter Gadfort <gadfort@zeroasic.com>
![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 fixes a bug in dbReadVerilog where deep-descendant pins were incorrectly attached to ancestor module ports by name and includes regression tests. It also introduces a sanity check, checkSanityModNetPortAliasing, to detect structural integrity violations where a module boundary aliases distinct external nets. Feedback suggests optimizing performance by moving container declarations outside of loops in the sanity check and refining the warning message to only include ports that contribute to the aliasing issue.
| // collect the distinct ids encountered. >= 2 distinct ids | ||
| // means the boundary aliases two unrelated external nets. |
There was a problem hiding this comment.
Declaring std::set and std::vector inside the nested loops results in frequent allocations and deallocations. For designs with a large number of hierarchical nets, this can impact performance. Consider moving these declarations outside the loops and using .clear() at the start of each iteration. Additionally, ensure the containers are cleared at the end of the function's scope to maintain consistency with repository practices.
References
- When checking for container clearing before population, verify if the container is cleared at the end of the function's scope, not just at the beginning.
| std::vector<std::string> port_names; | ||
| for (odb::dbModBTerm* mbt : mbts) { | ||
| port_names.emplace_back(mbt->getName()); | ||
| odb::dbModITerm* pmi = mbt->getParentModITerm(); | ||
| if (pmi == nullptr) { | ||
| continue; | ||
| } | ||
| odb::dbModNet* pmn = pmi->getModNet(); | ||
| if (pmn == nullptr) { | ||
| continue; | ||
| } | ||
| odb::dbNet* pflat = pmn->findRelatedNet(); | ||
| if (pflat != nullptr) { |
There was a problem hiding this comment.
Currently, all port names are added to port_names, but the warning message specifically highlights ports that resolve to distinct flat nets. If a port is unconnected (i.e., pflat is null), including it in the warning list might be confusing to the user. It is better to only include ports that actually contribute to the aliasing issue.
odb::dbNet* pflat = pmn->findRelatedNet();
if (pflat != nullptr) {
parent_flat_ids.insert(pflat->getId());
port_names.emplace_back(mbt->getName());
}
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
ensure correct connectivity in verilog read in with hier turned on. I noticed an issue where multiple clocks that could not have any paths between them where getting connected in openroad, this was the fix Claude suggested with the associated testcases.
I checked this on a much larger circuit and it appears to work correctly, but this should be verified.
Type of Change
Impact
Avoids creating incorrect connections.
Verification
./etc/Build.sh).Related Issues
[Link issues here]