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

Repair setup4 hier case 1 buffer #6048

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

Conversation

andyfox-rushc
Copy link
Contributor

Changes to resizer to support hierarchy for rebuffer and split load operations only

New test cases: split_load_hier.tcl (shows split load across hierarchy with factoring of nets to minimize punch through), resize1_hier.tcl (shows rebuffering and resizing with hierarchical design).

Key changes:

  1. Journalling (dbJournal.cpp) to support hierarchical net connection/disconnection.
  2. Propagation of modnets in rebuffering. (eg rebuffer.cc, RepairSetup.cc)
  3. Wherever parasitics updated make sure a modnet is not used.

This is a substantial change so marked as draft until Matt and I have gone through changes.

@andyfox-rushc andyfox-rushc marked this pull request as draft October 29, 2024 13:29
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/dbNetwork.cc Outdated Show resolved Hide resolved
src/rsz/src/Rebuffer.cc Outdated Show resolved Hide resolved
src/rsz/src/Rebuffer.cc Outdated Show resolved Hide resolved
src/rsz/src/Rebuffer.cc Outdated Show resolved Hide resolved
Signed-off-by: andyfox-rushc <[email protected]>
Signed-off-by: andyfox-rushc <[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/rsz/src/Rebuffer.cc Show resolved Hide resolved
Signed-off-by: andyfox-rushc <[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/dbNetwork.cc Outdated Show resolved Hide resolved
Copy link
Contributor

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

Copy link
Contributor

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

@andyfox-rushc andyfox-rushc marked this pull request as ready for review October 29, 2024 22:10
@andyfox-rushc
Copy link
Contributor Author

Matt, ready for review. This one just has split_loads and rebuffer support for hierarchy.

Copy link
Contributor

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

@maliberty
Copy link
Member

secure CI testing started

Copy link
Member

@maliberty maliberty left a comment

Choose a reason for hiding this comment

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

partial review

Comment on lines 236 to 237
delete (obj->_name);
obj->_name = strdup(new_name);
Copy link
Member

Choose a reason for hiding this comment

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

free is the opposite of strdup not delete.

I also don't see a free call in dbModNet::destroy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.
Note the tbldestroy goes off and calls the destructor ~_dbModNet which inturn calls free on the name .
block->_modnet_tbl->destroy(_modnet);

//
// Support for renaming hierarchical nets
//
void dbModNet::reName(const char* new_name)
Copy link
Member

Choose a reason for hiding this comment

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

rename is one word so no need a capital in reName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, done

obj->_name = strdup(new_name);
_dbBlock* block = (_dbBlock*) obj->getOwner();
_dbModule* parent = block->_module_tbl->getPtr(obj->_parent);
parent->_modnet_hash[new_name] = obj->getOID();
Copy link
Member

Choose a reason for hiding this comment

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

You haven't removed the old name from the hash.

@@ -1618,6 +1634,7 @@ void dbJournal::undo_connectObject()
dbITerm* iterm = dbITerm::getITerm(_block, iterm_id);
uint net_id;
_log.pop(net_id);
// disconnects everything modnet and bnet)
Copy link
Member

Choose a reason for hiding this comment

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

stray )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Poor comment, cleaned up.

Comment on lines 2854 to 2858
std::string port_name_str = moditerm->getName();
size_t last_idx = port_name_str.find_last_of('/');
if (last_idx != string::npos) {
port_name_str = port_name_str.substr(last_idx + 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this done by string manipulation? Is this different from getChildModBTerm()->getName()? Why does moditerm even need to store its name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I have put in the getChildModBTerm and remove the string manipulation.
I am sure not sure why but in the opensta both pins and port names are stored. So we do indeed store names in the modIterm and modBterm. I have updated the code and put in a comment about this in the dbSta/dbReadVerilog.cc. We can always get the ModBTerm for a modIterm using the getChildModBTerm api.

Comment on lines 746 to 748
odb::dbNet* db_net = nullptr;
odb::dbModNet* db_modnet = nullptr;
db_network_->staToDb(net, db_net, db_modnet);
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose? Nothing seems to use the results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant code, removed.

@@ -150,7 +150,11 @@ void OdbCallBack::inDbInstSwapMasterAfter(dbInst* inst)
while (pin_iter->hasNext()) {
Pin* pin = pin_iter->next();
Net* net = network_->net(pin);
resizer_->parasiticsInvalid(net);
// we can only update parasitics for low level net
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow the comment. There is only one dbNet attached to the pin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another poor comment I am afraid. Cleaned up to call the new flatNet api, so it is clear that the parasiticsInvalid is applied to the dbNet only

Comment on lines 79 to 81
odb::dbNet* db_net;
odb::dbModNet* db_mod_net;
db_network_->net(drvr_pin, db_net, db_mod_net);
Copy link
Member

Choose a reason for hiding this comment

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

This pattern repeats a lot. It seems better to just make an api like odb::dbNet* db_net = db_network_->flat_net(drvr_pin);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I called the api flatNet. Two versions: one for net and one for pin.

Comment on lines 59 to 65
/*void
writeVerilog(const char *filename,
bool sort,
bool include_pwr_gnd,
sta::CellSeq *remove_cells,
sta::Network *network);
*/
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sorry some old debug code (was looking at verilog at various points in flow).

Signed-off-by: andyfox-rushc <[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

@@ -132,6 +132,7 @@ class dbNetwork : public ConcreteNetwork

dbNet* staToDb(const Net* net) const;
void staToDb(const Net* net, dbNet*& dnet, dbModNet*& modnet) const;
dbNet* flatNet(const Net* pin) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'sta::dbNetwork::flatNet' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

  dbNet* flatNet(const Net* pin) const;
         ^
Additional context

src/dbSta/src/dbNetwork.cc:2198: the definition seen here

dbNet* dbNetwork::flatNet(const Net* net) const
                  ^

src/dbSta/include/db_sta/dbNetwork.hh:134: differing parameters are named here: ('pin'), in definition: ('net')

  dbNet* flatNet(const Net* pin) const;
         ^

Copy link
Contributor

github-actions bot commented Nov 1, 2024

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

Signed-off-by: andyfox-rushc <[email protected]>
Copy link
Contributor

github-actions bot commented Nov 1, 2024

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

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.

2 participants