The-OpenROAD-Project / OpenROAD

OpenROAD's unified application implementing an RTL-to-GDS Flow. Documentation at https://openroad.readthedocs.io/en/latest/
https://theopenroadproject.org/
BSD 3-Clause "New" or "Revised" License
1.62k stars 561 forks source link

Operator module swap phase 1 #6163

Closed openroad-robot closed 2 days ago

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

precisionmoon commented 1 week ago

I think this still needs a c++ unit test.

Yes, C++ test can be added. It would be great if Verilog can be read in directly.

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

maliberty commented 1 week ago

@andyfox-rushc please help review

QuantamHD commented 1 week ago

I think this still needs a c++ unit test.

Yes, C++ test can be added. It would be great if Verilog can be read in directly.

Take a look at the ReadVerilog helper in RMP https://github.com/The-OpenROAD-Project/OpenROAD/blob/master/src/rmp/test/cpp/TestAbc.cc#L89-L125

precisionmoon commented 1 week ago

I think this still needs a c++ unit test.

Yes, C++ test can be added. It would be great if Verilog can be read in directly.

Take a look at the ReadVerilog helper in RMP https://github.com/The-OpenROAD-Project/OpenROAD/blob/master/src/rmp/test/cpp/TestAbc.cc#L89-L125

Thanks much for the link. I'll work on adding C++ test next.

precisionmoon commented 1 week ago

Yes, I will add a new test with lower level module swap.

On Thu, Nov 14, 2024 at 10:43 PM andy fox @.***> wrote:

@.**** commented on this pull request.

In src/odb/test/replace_design1.v https://github.com/The-OpenROAD-Project/OpenROAD/pull/6163#discussion_r1843261454 :

@@ -0,0 +1,62 @@ +module top (clk);

This looks like a very good test. Please could you also make a version in which the object to be swapped is an instance in a lower level of the hierarchy (not in the topLevel). This is to stress the code that does the findInstance in the hierarchy (I see that calls the childIterator and I see that does indeed go through hierarchy), but I think it a good idea to make sure that a module at any point in the hierarchy (not just the top level) can be swapped.

— Reply to this email directly, view it on GitHub https://github.com/The-OpenROAD-Project/OpenROAD/pull/6163#pullrequestreview-2437798833, or unsubscribe https://github.com/notifications/unsubscribe-auth/BBVEJRS2VICAMCR4XUO57V32AWJZFAVCNFSM6AAAAABRZRV64GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZXG44TQOBTGM . You are receiving this because you commented.Message ID: @.***>

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 6 days ago

clang-tidy review says "All clean, LGTM! :+1:"

precisionmoon commented 3 days ago

@maliberty, @andyfox-rushc, @QuantamHD, all suggested code changes have been completed. I'll work on C++ testcase and more complex testcase next.