third-party: Import mockturtle sources#10670
Conversation
Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
There was a problem hiding this comment.
Code Review
This pull request integrates the mockturtle C++ logic network library into the third-party directory, adding various logic network representations, algorithms, and utilities. The review feedback highlights a critical initialization order bug in print_time that could lead to undefined behavior, a performance bottleneck in topo_view::node_to_index due to an O(N) linear scan, and an implicit double-to-float downcast in switching_activity.hpp.
| print_time() | ||
| : _t( new stopwatch<Clock>( _d ) ) | ||
| { | ||
| } | ||
|
|
||
| ~print_time() | ||
| { | ||
| delete _t; | ||
| std::cout << fmt::format( "[i] run-time: {:5.2f} secs\n", to_seconds( _d ) ); | ||
| } | ||
|
|
||
| private: | ||
| stopwatch<Clock>* _t{ nullptr }; | ||
| typename stopwatch<Clock>::duration _d{}; | ||
| }; |
There was a problem hiding this comment.
Lifetime and Initialization Order Issue
In C++, class members are initialized in the order of their declaration in the class, not the order in the constructor initializer list.
Here, _t is declared before _d:
stopwatch<Clock>* _t{ nullptr };
typename stopwatch<Clock>::duration _d{};Because of this, _t is initialized first. Its initializer new stopwatch<Clock>( _d ) passes _d by reference to the stopwatch constructor before _d itself has been initialized. This can lead to undefined behavior if the constructor or any of its sub-expressions attempt to access the uninitialized _d.
Recommendation
Declare _d before _t. Furthermore, we can completely avoid the raw pointer, dynamic allocation (new/delete), and manual memory management by making _t a direct member variable of type stopwatch<Clock> instead of a pointer.
print_time()
: _t( _d )
{
}
~print_time()
{
std::cout << fmt::format( "[i] run-time: {:5.2f} secs\n", to_seconds( _d ) );
}
private:
typename stopwatch<Clock>::duration _d{};
stopwatch<Clock> _t;
};| static_assert( has_foreach_po_v<Ntk>, "Ntk does not implement the foreach_po method" ); | ||
| static_assert( has_foreach_fanin_v<Ntk>, "Ntk does not implement the foreach_fanin method" ); | ||
| static_assert( has_incr_trav_id_v<Ntk>, "Ntk does not implement the incr_trav_id method" ); | ||
| static_assert( has_set_visited_v<Ntk>, "Ntk does not implement the set_visited method" ); | ||
| static_assert( has_trav_id_v<Ntk>, "Ntk does not implement the trav_id method" ); |
There was a problem hiding this comment.
Performance Bottleneck: $O(N)$ Node to Index Lookup
In topo_view, node_to_index is implemented using std::find on topo_order:
uint32_t node_to_index( node const& n ) const
{
return std::distance( std::begin( topo_order ), std::find( std::begin( topo_order ), std::end( topo_order ), n ) );
}This results in a linear scan (node_to_index frequently inside nested loops, this can degrade the overall complexity of those algorithms to
Recommendation
Consider caching the mapping from node to its index in a node_map<uint32_t, Ntk> or std::vector<uint32_t> during update_topo() to achieve
|
|
||
| ntk.foreach_node( [&]( auto const& n ) { | ||
| float ones = static_cast<float>( kitty::count_ones( tts[n] ) ); | ||
| float activity = 2.0 * ones / simulation_size * ( simulation_size - ones ) / simulation_size; |
There was a problem hiding this comment.
Double-Precision Promotion and Implicit Downcast
Using the double literal 2.0 promotes the entire expression to double precision, which is then implicitly downcast back to float when assigned to activity:
float activity = 2.0 * ones / simulation_size * ( simulation_size - ones ) / simulation_size;Recommendation
Use the float literal 2.0f to keep all calculations in single-precision float, avoiding unnecessary double-precision promotion and potential compiler warnings.
| float activity = 2.0 * ones / simulation_size * ( simulation_size - ones ) / simulation_size; | |
| float activity = 2.0f * ones / simulation_size * ( simulation_size - ones ) / simulation_size; |
|
Please add third-party/BUILD from your other PR so this can be compiled. At the same time please put that in third-party/mockturtle where it is properly scoped. |
Signed-off-by: Jakub Michalski <jmichalski@antmicro.com>
Signed-off-by: Bartłomiej Chmiel <bchmiel@antmicro.com>
Imported emap test, minimized to not include dependencies for functionality not used in OpenROAD and modified to use gtest Signed-off-by: Jakub Michalski <jmichalski@antmicro.com>
|
Added |
Signed-off-by: Jakub Michalski <jmichalski@antmicro.com>
|
@maliberty could you please take a look at this? The BUILD file is present and |
Summary
This PR contains extracted unmodified mockturtle sources from the #9097 in order to reduce diff size of the #9097 by separating original mockturtle sources from our changes on top of them that integrate mockturtle with OpenROAD
Type of Change
rmp#9097)Impact
No functional change to the OpenROAD, this is only an import of mockturtle sources into
third-party/mockturtleVerification
./etc/Build.sh)../etc/run-clang-tidy.shdoesn't seem to complain)