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

Support swapping to uninstantiated modules #6616

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

openroad-ci
Copy link
Collaborator

  1. this requires OpenSTA changes to
    set top module name properly and
    return contents of unused modules
  2. each unused module is stored in a separate child block of top block
  3. swapMaster supports swapping to unused modules
  4. some regression failures exist (dbSta/test has 3 failures)

1) this requires OpenSTA changes to
   set top module name properly and
   return contents of unused modules
2) each unused module is stored in a separate child block of top block
3) swapMaster supports swapping to unused modules
4) some regression failures exist (dbSta/test has 3 failures)

Signed-off-by: Cho Moon <[email protected]>
1) fixed regression failures
2) fixed connectivity bug due to wrong bus member bit
3) fixed missing mod nets and internal flat nets

Signed-off-by: Cho Moon <[email protected]>
1) excluded black box cells from module swap candidates
2) this nedes OpenSTA update
3) fixed regression test failures

Signed-off-by: Cho Moon <[email protected]>
1) linkNetwork change to defer module deletion is still outstanding
2) additional OpenSTA change is still needed

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

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/dbSta/src/dbReadVerilog.cc Outdated Show resolved Hide resolved
src/dbSta/src/dbReadVerilog.cc Outdated Show resolved Hide resolved
src/dbSta/src/dbReadVerilog.cc Outdated Show resolved Hide resolved
src/odb/src/db/dbBlock.cpp Outdated Show resolved Hide resolved
src/odb/src/db/dbBlock.cpp Outdated Show resolved Hide resolved
src/odb/src/db/dbBlock.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

@precisionmoon
Copy link
Contributor

@andyfox-rushc, could you review the codes?

Copy link
Contributor

@andyfox-rushc andyfox-rushc left a comment

Choose a reason for hiding this comment

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

dbInst::makeUniqueInst.
Please check that dbInst::create really does return null if the named object exists (I don't believe it does). Perhaps another approach is to look in the hash on the dbModule (possibly getting the size of the hash and using that size as the id). See dbModule::findInst.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the printContent. it is very useful to have this exposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment to clarify the layout of the bus ports which is:

modbterm <-- busport type (the sentinel) has reference to size of bus port -- number and direction of elements
list of modbterms the actual bus content

So for a bus with N elements we see N + 1 modbterms. (The first one is the "bus port" sentinel).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a map for dbInsts. Perhaps this map is for something else (to help find undefined objects ?) As a dbInst is added it is added to the inst_tbl in the block. See the tables in the block (the tables are used by the hash functions). So it is unclear to me why there is a need for another hash.

_net_hash.setTable(_net_tbl);
_inst_hash.setTable(_inst_tbl);
_module_hash.setTable(_module_tbl);
_modinst_hash.setTable(_modinst_tbl);

The index for the has is the name. So this would appear at first blush to do what you want, but I might be wrong !

Copy link
Contributor

@andyfox-rushc andyfox-rushc Jan 31, 2025

Choose a reason for hiding this comment

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

This is a duplicate comment. Method makeUniqueInst dbInst::makeUniqueInst.
Please check that dbInst::create really does return null if the named object exists (I don't believe it does). Perhaps another approach is to look in the hash on the dbModule (possibly getting the size of the hash and using that size as the id). See dbModule::findInst.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the naming construction. Please can you add a "todo" item to get the hierarchy separator from the dbBlock (there is a field called hier_delimiter) in dbBlock.h. Likewise a todo item could be usefully added to state the array index delimiter on the dbBlock. I realize that I hardcoded those things ('/' and '[') myself and can now see it will proliferate. I think we need to get those characters from the same place (the dbBlock).

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