Skip to content

third-party: Import mockturtle sources#10670

Open
jmichalski-ant wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
antmicro:mockturtle-sources
Open

third-party: Import mockturtle sources#10670
jmichalski-ant wants to merge 5 commits into
The-OpenROAD-Project:masterfrom
antmicro:mockturtle-sources

Conversation

@jmichalski-ant

@jmichalski-ant jmichalski-ant commented Jun 16, 2026

Copy link
Copy Markdown

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

Impact

No functional change to the OpenROAD, this is only an import of mockturtle sources into third-party/mockturtle

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines. (its a third-party code, but ./etc/run-clang-tidy.sh doesn't seem to complain)
  • I have signed my commits (DCO).

Signed-off-by: Maciej Torhan <mtorhan@antmicro.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:

Please ensure:

  • CI passes
  • Code is properly formatted
  • Tests are included where applicable
    A maintainer will review shortly!

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +164 to +178
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{};
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
};

Comment on lines +139 to +143
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" );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 ($O(N)$ complexity) for every lookup. Since many algorithms (such as cut enumeration, mapping, etc.) call node_to_index frequently inside nested loops, this can degrade the overall complexity of those algorithms to $O(N^2)$ or worse.

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 $O(1)$ lookups.


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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
float activity = 2.0 * ones / simulation_size * ( simulation_size - ones ) / simulation_size;
float activity = 2.0f * ones / simulation_size * ( simulation_size - ones ) / simulation_size;

@maliberty

Copy link
Copy Markdown
Member

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.

jmichalski-ant and others added 3 commits June 17, 2026 14:41
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>
@jmichalski-ant

Copy link
Copy Markdown
Author

Added third-party/mockturtle/BUILD and some test to have something to compile mockturtle with (the part we are importing here is header-only). I also had to pull a single change from the other PR, because we modified mockturtle to not bring its bundled dependencies where possible in order to reduce diff size (by e.g replacing phmap::flat_hash_map with absl::flat_hash_map already present in OpenROAD dependencies)

Signed-off-by: Jakub Michalski <jmichalski@antmicro.com>
@jmichalski-ant

jmichalski-ant commented Jun 19, 2026

Copy link
Copy Markdown
Author

@maliberty could you please take a look at this? The BUILD file is present and //third-party/mockturtle:mockturtle_emap_test test compiles and runs mockturtle code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants