parallaxsw / OpenSTA

GNU General Public License v3.0
52 stars 23 forks source link

Hierarchical Verilog writer repairs #110

Closed andyfox-rushc closed 1 month ago

andyfox-rushc commented 1 month ago

Hi, We observed some strange behaviour with the iterators on pendingchildren during writeModule (we saw a seg fault). Matt suggested replacing the vector with a deque which seems to do the trick. I am not quite sure why the original code failed (looked ok to me) but the problem seemed to occur when at the last element of the pendingchildren and then the recursive call adding another element. The fix includes the replacement of the pendingchildren Vector -> deque in writeModule in VerilogWriter.cc Also, I added a sort for deque in Vector.hh (am guessing there might usefully be a new header file wrapper for deque).

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

parallaxsw commented 1 month ago

The failure was caused by the range iteration on the pendingchildren with recursive calls that added to the vector invalidating the iterator. There is another issue that sorting and collect the modules as they are being written doesn't make much sense. I pushed

1dbcb329 write_verilog separate cell search from write

that separates the search from the writing that should fix your issue.

PS, it also doesn't make any sense to add a sort template for deque to Vector.hh.

maliberty commented 1 month ago

@andyfox-rushc is the above commit sufficient to resolve your issue and close this PR? If so I can update sta in OR.

andyfox-rushc commented 1 month ago

While James is very kindly looking at his code (I note he caught the weird sorting issue -- where at every level of recursion all the children were being sorted), please can he also clean up the findUnconnectedNetCount ? This is also another little issue. Specifically it is being called by writeWireDcls (called by the recursive call writeModule) for every instance and it traverses the whole hierarchy.

VerilogWriter::findUnconnectedNetCount() { return findNCcount(network_->topInstance()); <-- no need to do the whole hierarchy !! }

Probably best to to keep this PR open (now changed to be called "Hierarchical Verilog Writer Repairs") until Cho has checked out a few more hierarchical cases and validated them. For example the uses of bus ports at intermediate levels may have some weirdness.

My understanding is that the OpenROAD verilog writer was primarily used on flat designs after the flow has been run so it is quite reasonable to expect there to be a few more little things in the hierarchical writing.

Thank you James for looking into these things. Rather than piecemeal deal with them, probably best just to keep this PR open a little longer to catch a few more things.

parallaxsw commented 1 month ago

Independent of what further testing reveals this PR is no longer relevant so I am going to close it. The issue with findUnconnectedNetCount is actually that the unconnected pins are always counted for the the top level module (there is no recursive call). I addressed that issue with a2d445b0 write_verilog unconnected wire dcls for non-top level modules