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

Complete support for swapping to uninstantiated modules #6685

Conversation

openroad-ci
Copy link
Collaborator

@openroad-ci openroad-ci commented Feb 11, 2025

  1. Enabled OpenSTA linkNetwork change to defer module deletion
  2. Enabled replace_design4 test
  3. Incorporated code review comments from Andy Fox and Matt Liberty

1. Enabled OpenSTA linkNetwork change to defer module deletion
2. Fixed bogus Coverity memory leaks
3. Incorporated code review comments from Andy Fox and Matt Liberty

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

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

@oharboe
Copy link
Collaborator

oharboe commented Feb 11, 2025

This sounds interesting, what is the use-case?

Is there a test-case(doubles as documentation)?

Will there be a test in ORFS to demonstrate the use-case for this?

@QuantamHD
Copy link
Collaborator

QuantamHD commented Feb 11, 2025

It's part of a feature to swap different operator implementations. Yosys will provide OR N implementations of an add operator and OR will choose the best one depending on PPA requirements.

@oharboe
Copy link
Collaborator

oharboe commented Feb 11, 2025

It's part of a feature to swap different operator implementations. Yosys will provide OR N implementations of an add operator and OR will choose the best one depending on PPA requirements.

So if I understand:

late instantiation of addition algorithms and standard cell elements when there is information available as to which one is best.

The alternative is impractical: to specify addition algorithms manually. It can vary across addition usage in the design and there can be a great many additions performed and no single solution is best everywhere.

@QuantamHD
Copy link
Collaborator

QuantamHD commented Feb 11, 2025

It's part of a feature to swap different operator implementations. Yosys will provide OR N implementations of an add operator and OR will choose the best one depending on PPA requirements.

So if I understand:

late instantiation of addition algorithms and standard cell elements when there is information available as to which one is best.

The alternative is impractical: to specify addition algorithms manually. It can vary across addition usage in the design and there can be a great many additions performed and no single solution is best everywhere.

You got it! There's no best adder, just the best tradeoff. It's essentially just another dimension resizer has to play with, but instead of single cells you get entire modules.

@maliberty
Copy link
Member

There will eventually be usage in ORFS when this is fully automated in timing optimization.

Copy link
Contributor

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

@precisionmoon
Copy link
Contributor

Small comments on the swapMaster which I was asked to review (not sure if my comments on that pull request will still exist as pull request accepted).

@andyfox-rushc , yes I took care of your comments in the previous PR.

Please check that after swapping the master the modbterms reference the moditerms in the instance. (SwapMaster)
This is done using setParentModITerm on each of the new master ModBTerms.

I'm already using setParentModITerm in dbModule::copyModuleBoundaryIO()
if (new_mod_bterm) {
old_mod_iterm->setChildModBTerm(new_mod_bterm);
new_mod_bterm->setParentModITerm(old_mod_iterm);

Is there more places where I should be using the API?:

I read through your code on the SwapMaster and see that you correctly update the flat connections (traverse iterms for each modNet pair (new and old), get the original flat net disconnect from original iterm and then connect to the new iterm). Good work. I respectfully suggest adding a test case in which new ModNet has (a) Different number of iterms. (b) Possibly none (a wire running through the new module or an output resolved to a constant). I see no reason why (a) and (b) won't work but I know that sometimes when replacing IP blocks we see "through wires" and "constants"

andy

OK. I'll add the new test cases, perhaps as a separate PR later.

Thanks.

@maliberty maliberty enabled auto-merge February 20, 2025 17:25
Copy link
Contributor

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

@maliberty maliberty merged commit 9312fb4 into The-OpenROAD-Project:master Feb 20, 2025
11 checks passed
@maliberty maliberty deleted the secure-module-swap-6 branch February 20, 2025 19:31
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.

5 participants