Add extended technology mapping from mockturtle to rmp #9097
Add extended technology mapping from mockturtle to rmp #9097m-torhan wants to merge 101 commits into
rmp #9097Conversation
Summary of ChangesHello @m-torhan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to OpenROAD's logic synthesis capabilities by integrating the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature, enabling extended technology mapping using the mockturtle library. The changes are extensive, adding mockturtle as a submodule and integrating it with the cut and rmp components. The core logic involves extracting a logic cone into a mockturtle AIG, performing technology mapping, and then re-inserting the result into the OpenROAD database. While this is a great addition, I've identified several critical issues, including potential crashes from null pointer dereferences and the use of C++20 features in a C++17 codebase. I've also noted some medium-severity concerns regarding code style, potential limitations, and API usage. Given the draft status of this PR, addressing these points will be crucial for hardening the implementation.
| if (sta_pin == nullptr) { | ||
| logger->error(utl::CUT, | ||
| 53, | ||
| "Cannot find pin {} on instance {}.", | ||
| p.name, | ||
| network->name(driver_inst)); | ||
| } | ||
|
|
||
| sta::Net* fanin_net = network->net(sta_pin); |
There was a problem hiding this comment.
Similar to the issue above, if network->findPin returns nullptr, an error is logged, but execution continues. The subsequent line sta::Net* fanin_net = network->net(sta_pin); will then dereference a null pointer, causing a crash. The function should terminate gracefully by throwing an exception.
| if (sta_pin == nullptr) { | |
| logger->error(utl::CUT, | |
| 53, | |
| "Cannot find pin {} on instance {}.", | |
| p.name, | |
| network->name(driver_inst)); | |
| } | |
| sta::Net* fanin_net = network->net(sta_pin); | |
| if (sta_pin == nullptr) { | |
| logger->error(utl::CUT, | |
| 53, | |
| "Cannot find pin {} on instance {}.", | |
| p.name, | |
| network->name(driver_inst)); | |
| throw std::runtime_error("Pin not found on instance"); | |
| } | |
| sta::Net* fanin_net = network->net(sta_pin); |
| if (drivers->size() != 1) { | ||
| logger->error(utl::CUT, | ||
| 51, | ||
| "Net {} has {} drivers; CUT expects exactly one.", | ||
| network->name(net), | ||
| drivers->size()); | ||
| } |
There was a problem hiding this comment.
If a net has multiple drivers, an error is logged, but execution continues by using the first driver found (*drivers->begin()). This could lead to incorrect logic being generated for the AIG network, as it silently ignores other drivers. This is a correctness issue. The function should abort more forcefully, for instance by throwing an exception, to prevent generating a potentially incorrect circuit, especially since a net with multiple drivers is a significant issue in a synthesized netlist.
There was a problem hiding this comment.
Error throws an exception so the control flow ends
| // want to add vertices from these cells. We really just | ||
| // want the ones in the fan-in set who are filtered by STA | ||
| // for one reason or another. | ||
| if (endpoint_set.contains(vertex)) { |
There was a problem hiding this comment.
Using contains is a C++20 feature. The project seems to be using C++17. This might cause compilation issues with some compilers. The C++17-compatible way to check for existence is endpoint_set.find(vertex) != endpoint_set.end(). This also applies to line 317.
| if (endpoint_set.contains(vertex)) { | |
| if (endpoint_set.find(vertex) != endpoint_set.end()) { |
| for (const sta::Instance* instance : cut.cut_instances()) { | ||
| sta->getDbNetwork()->deleteInstance(const_cast<sta::Instance*>(instance)); | ||
| } |
There was a problem hiding this comment.
The use of const_cast here is necessary because cut.cut_instances() returns a const reference to the instance set, and iterating over it yields const sta::Instance*, while deleteInstance requires a non-const sta::Instance*. While this works, it's a code smell.
For future refactoring, consider adding a non-const overload for cut_instances() in the cut::LogicCut class to provide non-const access to the underlying set, which would eliminate the need for const_cast.
// In cut::LogicCut
sta::InstanceSet& cut_instances() { return cut_instances_; }With such a change, this loop could be written more cleanly. Since this change is outside the diff, no code suggestion is provided here.
| for (const sta::Net* net : nets_to_be_deleted) { | ||
| sta->getDbNetwork()->deleteNet(const_cast<sta::Net*>(net)); | ||
| } |
There was a problem hiding this comment.
The use of const_cast here can be avoided. The nets_to_be_deleted set contains sta::Net* pointers. By declaring the loop variable as sta::Net* instead of const sta::Net*, you can pass it directly to deleteNet without casting away constness.
| for (const sta::Net* net : nets_to_be_deleted) { | |
| sta->getDbNetwork()->deleteNet(const_cast<sta::Net*>(net)); | |
| } | |
| for (sta::Net* net : nets_to_be_deleted) { | |
| sta->getDbNetwork()->deleteNet(net); | |
| } |
| } | ||
| } | ||
|
|
||
| mockturtle::tech_library<9u> tech_lib(gates); |
There was a problem hiding this comment.
The mockturtle::tech_library is instantiated with a hardcoded maximum number of inputs 9u. This could be a limitation if the provided genlib file contains cells with more than 9 inputs. Consider making this value more flexible, for example, by using a larger constant or making it configurable, to support a wider range of technology libraries.
|
|
||
| import_mockturtle_mapped_network(sta, mapped_ntk, libs, cut, logger); | ||
|
|
||
| db->triggerPostReadDb(); |
There was a problem hiding this comment.
The function db->triggerPostReadDb() is called after modifying the database. As its name suggests, this trigger is intended to be used after reading a database (e.g., from a DEF or LEF file), not after in-memory modifications. While it might have the desired side effect of updating some data structures, it's not clear if this is the correct and safe API to use here. Please verify if this is the intended usage or if there's a more appropriate way to signal that the database has been modified.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include "mockturtle/networks/aig.hpp" | ||
| #include "mockturtle/algorithms/cleanup.hpp" | ||
| #include "mockturtle/utils/tech_library.hpp" | ||
| #include "mockturtle/views/names_view.hpp" |
There was a problem hiding this comment.
warning: included header tech_library.hpp is not used directly [misc-include-cleaner]
| #include "mockturtle/views/names_view.hpp" | |
| #include "mockturtle/views/names_view.hpp" |
| @@ -60,4 +71,192 @@ class LogicCut | |||
| std::vector<sta::Net*> primary_outputs_; | |||
| sta::InstanceSet cut_instances_; | |||
| }; | |||
|
|
|||
| namespace { | |||
There was a problem hiding this comment.
warning: do not use unnamed namespaces in header files [google-build-namespaces]
namespace {
^|
|
||
| mockturtle::signal<mockturtle::aig_network> tt_to_aig( | ||
| mockturtle::names_view<mockturtle::aig_network>& ntk, | ||
| const kitty::dynamic_truth_table& tt, |
There was a problem hiding this comment.
warning: no header providing "kitty::dynamic_truth_table" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_cut.h:11:
- #include "mockturtle/networks/aig.hpp"
+ #include "kitty/dynamic_truth_table.hpp"
+ #include "mockturtle/networks/aig.hpp"| { | ||
| using aig_ntk = mockturtle::names_view<mockturtle::aig_network>; | ||
| using signal = mockturtle::signal<aig_ntk>; | ||
| using gate = mockturtle::gate; |
There was a problem hiding this comment.
warning: no header providing "mockturtle::gate" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_cut.h:11:
- #include "mockturtle/networks/aig.hpp"
+ #include "mockturtle/io/genlib_reader.hpp"
+ #include "mockturtle/networks/aig.hpp"| #include <unordered_set> | ||
| #include <vector> | ||
|
|
||
| #include "cut/abc_library_factory.h" | ||
| #include "cut/logic_cut.h" | ||
| #include "db_sta/dbSta.hh" | ||
| #include "mockturtle/utils/tech_library.hpp" | ||
| #include "sta/Bfs.hh" | ||
| #include "sta/Graph.hh" |
There was a problem hiding this comment.
warning: included header Bfs.hh is not used directly [misc-include-cleaner]
| #include "sta/Graph.hh" | |
| #include "sta/Graph.hh" |
| std::vector<std::vector<odb::dbNet*>> node_out_nets(num_nodes); | ||
|
|
||
| // Const nets (for constant fanins) | ||
| odb::dbNet* const0_net = nullptr; |
There was a problem hiding this comment.
warning: no header providing "odb::dbNet" is directly included [misc-include-cleaner]
odb::dbNet* const0_net = nullptr;
^| } | ||
|
|
||
| std::string inst_name = std::format("n_{}", idx); | ||
| odb::dbInst* inst = odb::dbInst::create(block, master, inst_name.c_str()); |
There was a problem hiding this comment.
warning: no header providing "odb::dbInst" is directly included [misc-include-cleaner]
odb::dbInst* inst = odb::dbInst::create(block, master, inst_name.c_str());
^| } | ||
|
|
||
| auto out_mterms = getSignalOutputs(master); | ||
| const uint32_t num_cell_outputs = static_cast<uint32_t>(out_mterms.size()); |
There was a problem hiding this comment.
warning: no header providing "uint32_t" is directly included [misc-include-cleaner]
src/rmp/src/extended_technology_mapping.cpp:9:
- #include "map/mio/mio.h"
+ #include <cstdint>
+ #include "map/mio/mio.h"| odb::dbMTerm* o_mterm = out_mterms[out_pin_idx]; | ||
| const std::string pin_name = o_mterm->getName(); | ||
|
|
||
| odb::dbITerm* o_iterm = inst->findITerm(pin_name.c_str()); |
There was a problem hiding this comment.
warning: no header providing "odb::dbITerm" is directly included [misc-include-cleaner]
odb::dbITerm* o_iterm = inst->findITerm(pin_name.c_str());
^| auto code = lorina::read_genlib(std::string(genlib_file_name), | ||
| mockturtle::genlib_reader(gates)); | ||
|
|
||
| if (code != lorina::return_code::success) { |
There was a problem hiding this comment.
warning: no header providing "lorina::return_code" is directly included [misc-include-cleaner]
src/rmp/src/extended_technology_mapping.cpp:9:
- #include "map/mio/mio.h"
+ #include "lorina/common.hpp"
+ #include "map/mio/mio.h"
| @@ -4,3 +4,6 @@ | |||
| [submodule "src/abc"] | |||
| path = third-party/abc | |||
| url = ../../The-OpenROAD-Project/abc.git | |||
| [submodule "third-party/mockturtle"] | |||
| path = third-party/mockturtle | |||
| url = https://github.com/antmicro/mockturtle | |||
There was a problem hiding this comment.
What are the changes in antmicro's fork vs upstream?
|
Given that mockturtle is not under active development, should we just copy over the relevant subset rather than taking a full dependency? |
Will this PR add that feature or is that future work?. What is actually enabled here? Are there any results to share? |
|
@maliberty could you trigger the CI again |
|
|
||
| mockturtle::signal<mockturtle::aig_network> tt_to_aig( | ||
| mockturtle::names_view<mockturtle::aig_network>& ntk, | ||
| const kitty::dynamic_truth_table& tt, |
There was a problem hiding this comment.
warning: no header providing "kitty::dynamic_truth_table" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_cut.h:11:
- #include "mockturtle/algorithms/cleanup.hpp"
+ #include "kitty/dynamic_truth_table.hpp"
+ #include "mockturtle/algorithms/cleanup.hpp"| { | ||
| using aig_ntk = mockturtle::names_view<mockturtle::aig_network>; | ||
| using signal = mockturtle::signal<aig_ntk>; | ||
| using gate = mockturtle::gate; |
There was a problem hiding this comment.
warning: no header providing "mockturtle::gate" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_cut.h:12:
- #include "mockturtle/networks/aig.hpp"
+ #include "mockturtle/io/genlib_reader.hpp"
+ #include "mockturtle/networks/aig.hpp"|
|
||
| sta::PinSet* drivers = network->drivers(net); | ||
| // Net driven from outside the cut handled by logic_extractor | ||
| assert(drivers && !drivers->empty()); |
There was a problem hiding this comment.
warning: no header providing "assert" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_cut.h:5:
- #include <utility>
+ #include <cassert>
+ #include <utility>| return nullptr; | ||
| } | ||
| // Okay if the cell we're looking for is actually a const cell add it. | ||
| sta::LibertyCell* liberty_cell = network->libertyCell(instance); |
There was a problem hiding this comment.
warning: no header providing "sta::LibertyCell" is directly included [misc-include-cleaner]
src/cut/include/cut/logic_extractor.h:15:
- #include "sta/NetworkClass.hh"
+ #include "sta/Liberty.hh"
+ #include "sta/NetworkClass.hh"| #include "utl/Logger.h" | ||
|
|
||
| namespace abc { | ||
| Vec_Str_t* Abc_SclProduceGenlibStrSimple(SC_Lib* p); |
There was a problem hiding this comment.
warning: function 'Abc_SclProduceGenlibStrSimple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| Vec_Str_t* Abc_SclProduceGenlibStrSimple(SC_Lib* p); | |
| static Vec_Str_t* Abc_SclProduceGenlibStrSimple(SC_Lib* p); |
| auto genlib_str = Vec_StrArray(genlib_vec); | ||
| std::istringstream genlib(genlib_str); | ||
|
|
||
| class diagnostic_consumer : public lorina::diagnostic_consumer |
There was a problem hiding this comment.
warning: no header providing "lorina::diagnostic_consumer" is directly included [misc-include-cleaner]
src/rmp/src/extended_technology_mapping.cpp:10:
- #include "map/mio/mio.h"
+ #include "lorina/diagnostics.hpp"
+ #include "map/mio/mio.h"| public: | ||
| diagnostic_consumer(utl::Logger* logger) : logger_(logger) {} | ||
|
|
||
| void handle_diagnostic(lorina::diagnostic_level level, |
There was a problem hiding this comment.
warning: no header providing "lorina::diagnostic_level" is directly included [misc-include-cleaner]
void handle_diagnostic(lorina::diagnostic_level level,
^| }; | ||
|
|
||
| diagnostic_consumer diag_consumer(logger); | ||
| lorina::diagnostic_engine diag_engine(&diag_consumer); |
There was a problem hiding this comment.
warning: no header providing "lorina::diagnostic_engine" is directly included [misc-include-cleaner]
lorina::diagnostic_engine diag_engine(&diag_consumer);
^| auto code = lorina::read_genlib( | ||
| genlib, mockturtle::genlib_reader(gates), &diag_engine); | ||
|
|
||
| if (code != lorina::return_code::success) { |
There was a problem hiding this comment.
warning: no header providing "lorina::return_code" is directly included [misc-include-cleaner]
src/rmp/src/extended_technology_mapping.cpp:10:
- #include "map/mio/mio.h"
+ #include "lorina/common.hpp"
+ #include "map/mio/mio.h"| namespace rmp { | ||
|
|
||
| void extended_technology_mapping(sta::dbSta* sta, | ||
| odb::dbDatabase *db, |
There was a problem hiding this comment.
warning: no header providing "odb::dbDatabase" is directly included [misc-include-cleaner]
src/rmp/src/extended_technology_mapping.h:4:
- #include "rsz/Resizer.hh"
+ #include "odb/db.h"
+ #include "rsz/Resizer.hh"
I'm open to that, it would make development easier. @QuantamHD thoughts?
It's supposed to be in this PR. What's enabled is |
|
I'm fine with just pulling over emap + the deps if they're reasonable in size. |
m-torhan
force-pushed
the
mockturtle-extended-technology-mapping
branch
6 times, most recently
from
b56ae22 to
b32fa17
Compare
m-torhan
force-pushed
the
mockturtle-extended-technology-mapping
branch
from
2fbf0d1 to
6bbf135
Compare
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
* removing emap_sub_aes as it was minimized version of AES * adding cell type reports before and after repair design Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
b-chmiel
force-pushed
the
mockturtle-extended-technology-mapping
branch
from
021bae8 to
90717c2
Compare
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
7 participants
This PR adds extended technology mapping from mockturtle to
rmptool asresynth_emapcommand. It adds possibility to map multioutput cells such as adders.It also adds support for exporting/importing design between OpenROAD and mockturtle, using mockturtle tech library in logic cut from
cutas it was required for this work.In the further steps, there will also be added possibility to read liberty into mockturtle tech library, as currently it reads it from genlib format and may cause issues if those are not compatible.
The mockturtle is currently supported only for CMake builds as it does not have configurations for Bazel. This will also be handled in the future.
Opening this as draft to get early feedback.