undo prep for module swap

[openroad-ci]

1. old mod inst is now deleted and new one is created for module swap
2. saved old module to a child block for future use after module swap
3. added two tests to test ODB journal save/restore
   odb/test/replace_hier_mod_undo.tcl leads to a fatal
   odb/test/replace_hier_mod_undo2.tcl produces an error:
   [CRITICAL ODB-0417] No undo_deleteObject support for type dbBTerm
4. fixed a few ODB bug fixes to avoid fatals with hierarchy
   a) dbModInst.cpp and dbModule.cc - save objects in vector before deleting them inside iterator loop
   b) dbNetwork.cc - add nullptr checks in visitConnectedPins
   c) EstimateWireParasitics.cc - skip mod pins and no net pins
5. replaced delimeter with delimiter

[precisionmoon]

@andyfox-rushc, this is a work in progress but it'd be great if you can review. I fixed all the fatals from regression tests but am not sure if the fixes are OK. dbModule::destroy() doesn't seem to remove the module from the block somehow.

[github-actions]

clang-tidy made some suggestions

[maliberty]

Why delete the dbModInst rather than bind it to the new master (like dbInst::swapMaster)?

[andyfox-rushc]

The intent here (I think) is to make "undo" work in journalling. Because the journalling only works on atomic database operations (eg create/delete/connect) the swapping of the master (a pointer swap) is not recorded so journalling wont allow undo.
For this reason (I believe) the dbModInst is being recreated.
Another approach is to journal a command rather than an action. For example we could have a new class of journalling called "command". This might usefully have a sub-command type eg "swap master" and then in this case swap the id of the "master" dbModule in the dbModInst.

[maliberty]

The high level operation is just composed of atomic operations. Why can't any "pointer swap" be recorded?

[andyfox-rushc]

On the point about dbModule::destroy. What happens is the the dbModule is removed from the table of dbModules from the block and marked as free. It can be brought back to life if the delete is undo.

[andyfox-rushc]

This looks very strange. During visitConnectedPins I think we require A dbModBTerm must always have a dbModule associated with it. How can modbterm ->getParent be null ? Likewise how can modInst not have a dbModule.

Am guessing this is possibly some intermediate state, but it looks a little odd.

[precisionmoon]

@andyfox-rushc , yes you are correct. This happens during dbModInst::swapMaster(). The old mod inst has been deleted but there are old bterms that remain. When old iterm is disconnected (line 632) via old_irterm->disconnectDbNet() API, the fatal happens because of intermediate nature of old modbterm, old module and old modinst. assert() won't work. We need null checks.

[andyfox-rushc]

Thank you for the explanation.

Please can we agree that the legal intermediate state is the dbModule might have been swapped out so the dbModInst might have a null dbModule "master". For example when killing the dbModInst after swapping. However, please note that this api is the visitConnectedPins iterator -- ie traversing the pins on a net. Doing that on an intermediate state seems strange.

Please can we also agree that the dbModBTerms must always have a dbModule (master). Likewise the dbModITerms must always have an owning dbModInst. Disconnecting the net from the dbModITerm of dbModBTerm should not corrupt the parent relationship of the pins/ports (ie a dbModBTerm must always have a parent dbModule and likewise a dbModITerm must always have a parent dbModInst). Disconnect merely removes the the dbModITerm/dbModBTerm from the corresponding dbModNet.

I understand that this is an intermediate state issue (thank you again for the explanation and forgive me I have misunderstood). I respectfully suggest issuing an error if the dbModBTerm does not have a parent dbModule and likewise if the dbModITerm does not have a parent dbModInst.

[precisionmoon]

Yes, I agree that in legal state mod bterm should have module and mod iterm, mod inst. However, we don't want to issue errors during transient state. I'm not sure why disconnectDbNet() API needs visitConnectedPins.

#7 0x000055555dcb4ed4 in sta::dbNetwork::visitConnectedPins (this=0x55556197dfc0, net=0x5555638dd7c0, visitor=..., visited_nets=...) at /home/cmoon/OR/OpenROAD/src/dbSta/src/dbNetwork.cc:1787
#8 0x000055555aa081e7 in sta::Network::visitConnectedPins (this=0x55556197dfc0, net=0x5555638dd7c0, visitor=...) at /home/cmoon/OR/OpenROAD/src/sta/network/Network.cc:1330
#9 0x000055555aa091d3 in sta::Network::drivers (this=0x55556197dfc0, net=0x5555638dd7c0) at /home/cmoon/OR/OpenROAD/src/sta/network/Network.cc:1627
#10 0x000055555aa090f9 in sta::Network::drivers (this=0x55556197dfc0, pin=0x5555638bc0e0) at /home/cmoon/OR/OpenROAD/src/sta/network/Network.cc:1609
#11 0x000055555ac4c4d4 in sta::ConcreteParasitics::deleteReducedParasitics (this=0x555561898df0, pin=0x5555638bc0e0) at /home/cmoon/OR/OpenROAD/src/sta/parasitics/ConcreteParasitics.cc:907
#12 0x000055555ac4c277 in sta::ConcreteParasitics::disconnectPinBefore (this=0x555561898df0, pin=0x5555638bc0e0, network=0x55556197dfc0) at /home/cmoon/OR/OpenROAD/src/sta/parasitics/ConcreteParasitics.cc:865
#13 0x000055555ab60b6a in sta::Sta::disconnectPinBefore (this=0x5555618f7610, pin=0x5555638bc0e0) at /home/cmoon/OR/OpenROAD/src/sta/search/Sta.cc:4456
#14 0x000055555dc575f6 in sta::dbStaCbk::inDbITermPreDisconnect (this=0x555561aaf060, iterm=0x5555638bc0e0) at /home/cmoon/OR/OpenROAD/src/dbSta/src/dbSta.cc:1010
#15 0x000055555e148e7f in odb::dbITerm::disconnectDbNet (this=0x5555638bc0e0) at /home/cmoon/OR/OpenROAD/src/odb/src/db/dbITerm.cpp:597
#16 0x000055555e336ed0 in odb::dbModInst::swapMaster (this=0x5555638af448, new_module=0x5555638a6b50) at /home/cmoon/OR/OpenROAD/src/odb/src/db/dbModInst.cpp:632
#17 0x000055555dcbbac1 in sta::dbNetwork::replaceHierModule (this=0x55556197dfc0, mod_inst=0x5555638af448, module=0x5555638a6b50) at /home/cmoon/OR/OpenROAD/src/dbSta/src/dbNetwork.cc:3642

[andyfox-rushc]

That is the update timing stuff. As pins are disconnected the backend sta stuff is updated (a good thing !).

One small thought. I see when you are swapping modules you are dealing with the modnets (at the edge) and the flat nets. Recall in our model the flat nets are the physical wires in the flat world and the modnets are at the edge of each hierarchical module. Your code shows that you are handling those.

If the flat nets are not being disconnected from the module instance being deleted then it would be the case that a pin accessed by the netPinIterator would indeed not have a dbModule (same for moditerm). So I respectfully suggest checking that the flat connections are being correctly disconnected in the module being deleted.

[precisionmoon]

On the point about dbModule::destroy. What happens is the the dbModule is removed from the table of dbModules from the block and marked as free. It can be brought back to life if the delete is undo.

My apologies... dbModule::destroy() does work as intended. My debugPrintContent was placed before destroy.

[github-actions]

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

[github-actions]

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

[github-actions]

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

[precisionmoon]

@andyfox-rushc @maliberty, I found the root cause of fatal in wire para estimation code EstimateWireParasitics.cc after module swap. This was due to stale net driver pin cache which stores driver pins of nets. This becomes stale after netlist change like module swap. Now I clear the cache at the beginning of each wire para estimation. This resolved the fatal, at the expense of slight increase in runtime.

Also, I modified one regression replace_hier_mod1.tcl to perform module swap, do incremental placement and legalization, and do reverse swap with another incremental place and legalization. The before and after QoR now matches up to 3 digits.

[maliberty]

@precisionmoon I'll waive the DCO but please remember to sign your commits.

[maliberty]

Ok for now but should be replaced with mod objects callbacks (@andyfox-rushc )

[maliberty]

In the future please be a non-functional but pervasive change like "replaced delimeter with delimiter" in a separate PR. It makes it harder to review the functional changes.

[maliberty]

Doing this is less efficient. If the debug isn't printing then the fmt::format is never executed if you use the normal debugPrint idiom.

[precisionmoon]

Correct. I was trading off readability with inefficiency. Let me think of some other scheme to make it more efficient.

[precisionmoon]

In the future please be a non-functional but pervasive change like "replaced delimeter with delimiter" in a separate PR. It makes it harder to review the functional changes.

Yes.

[github-actions]

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