Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Phase 3 hierarchical module swap #6240

Conversation

openroad-robot
Copy link
Contributor

  1. added a new test that does module swap in a multi-level design
  2. changed module search to do recursive search for all levels of hierarchy
  3. added an error check for the same module swap

1) added a new test that does module swap in a multi-level design
2) changed module search to do recursive search for all levels of hierarchy
3) added an error check for the same module swap

Signed-off-by: Cho Moon <[email protected]>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@precisionmoon
Copy link
Contributor

precisionmoon commented Nov 26, 2024

@andyfox-rushc, here is a 3 level hierarchical testcase you asked for.

Comment on lines 424 to 431
// Check if module names differ
if (strcmp(old_module_name, new_module_name) == 0) {
logger->warn(utl::ODB,
470,
"The modules cannot be swapped because the new module {} is "
"identical to the existing module",
new_module_name);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a return here if the swap can't continue or use error()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

Comment on lines 3113 to 3114
auto recursiveSearch
= [this](Instance* inst, const char* name, auto& self) -> dbModule* {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not essential but you could capture recursiveSearch rather than make it an argument, though you would have to declare its type rather than use auto.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API has been removed and replaced by dbBlock::findModule().

@maliberty
Copy link
Member

@andyfox-rushc any comments?

@andyfox-rushc
Copy link
Contributor

andyfox-rushc commented Nov 26, 2024

To find a module (anywhere in the hierarchy) use: dbBlock::findModule(const char* name).
So the method: dbModule* dbNetwork::findModule(const char* name) can be replaced by (or better, simply use) this call.
This is an example of where the block stuff is (for once) helpful !

@precisionmoon
Copy link
Contributor

To find a module (anywhere in the hierarchy) use: dbBlock::findModule(const char* name). So the method: dbModule* dbNetwork::findModule(const char* name) can be replaced by (or better, simply use) this call. This is an example of where the block stuff is (for once) helpful !

@andyfox-rushc , thanks much for the suggestion. I did the replacement. Initially, I thought this caused inconsistency between STA and ODB but this was not due to this change.

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Cho Moon <[email protected]>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Cho Moon <[email protected]>
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@maliberty maliberty merged commit 3501a4c into The-OpenROAD-Project:master Nov 27, 2024
11 checks passed
@maliberty maliberty deleted the secure-swap-module3 branch November 27, 2024 04:16
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.

4 participants