Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Virtual pi network #543

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

Virtual pi network #543

wants to merge 56 commits into from

Conversation

hibenj
Copy link
Collaborator

@hibenj hibenj commented Oct 16, 2024

Description

This PR includes the virtual_pi_network.hpp, virtual_pi_network.cpp(testing) and virtual_pi_network.rst(documentation), as well as the necessary changes in traits.hpp
The virtual PI network enhances all mockturtle network types to store duplicates of their PIs, called virtual PIs.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have added a changelog entry.
  • I have created/adjusted the Python bindings for any new or updated functionality.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@hibenj hibenj requested review from marcelwa and Drewniok October 16, 2024 11:01
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 98.68049% with 14 lines in your changes missing coverage. Please review.

Project coverage is 98.13%. Comparing base (df6b61e) to head (66c412f).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
include/fiction/networks/views/depth_view.hpp 93.70% 8 Missing ⚠️
...thms/network_transformation/delete_virtual_pis.hpp 93.65% 4 Missing ⚠️
...lude/fiction/networks/views/extended_rank_view.hpp 98.51% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             main     #543     +/-   ##
=========================================
  Coverage   98.13%   98.13%             
=========================================
  Files         232      242     +10     
  Lines       35410    36476   +1066     
  Branches     1691     1743     +52     
=========================================
+ Hits        34748    35797   +1049     
- Misses        660      677     +17     
  Partials        2        2             
Files with missing lines Coverage Δ
.../fiction/algorithms/verification/virtual_miter.hpp 100.00% <100.00%> (ø)
include/fiction/networks/virtual_pi_network.hpp 100.00% <100.00%> (ø)
...thms/network_transformation/delete_virtual_pis.cpp 100.00% <100.00%> (ø)
test/algorithms/verification/virtual_miter.cpp 100.00% <100.00%> (ø)
test/networks/views/depth_view.cpp 100.00% <100.00%> (ø)
test/networks/views/extended_rank_view.cpp 100.00% <100.00%> (ø)
test/networks/virtual_pi_network.cpp 100.00% <100.00%> (ø)
...lude/fiction/networks/views/extended_rank_view.hpp 98.51% <98.51%> (ø)
...thms/network_transformation/delete_virtual_pis.hpp 93.65% <93.65%> (ø)
include/fiction/networks/views/depth_view.hpp 93.70% <93.70%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37d27c7...66c412f. Read the comment docs.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/fiction/networks/virtual_pi_network.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
test/networks/virtual_pi_network.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

test/algorithms/verification/virtual_miter.cpp Outdated Show resolved Hide resolved
test/algorithms/verification/virtual_miter.cpp Outdated Show resolved Hide resolved
test/algorithms/verification/virtual_miter.cpp Outdated Show resolved Hide resolved
test/algorithms/verification/virtual_miter.cpp Outdated Show resolved Hide resolved
test/algorithms/verification/virtual_miter.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Drewniok Drewniok left a comment

Choose a reason for hiding this comment

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

It looks great already! I hope my comments will help you.
Thank you! 🙏

include/fiction/algorithms/verification/virtual_miter.hpp Outdated Show resolved Hide resolved
include/fiction/algorithms/verification/virtual_miter.hpp Outdated Show resolved Hide resolved
include/fiction/algorithms/verification/virtual_miter.hpp Outdated Show resolved Hide resolved
include/fiction/networks/virtual_pi_network.hpp Outdated Show resolved Hide resolved
include/fiction/networks/virtual_pi_network.hpp Outdated Show resolved Hide resolved
include/fiction/networks/virtual_pi_network.hpp Outdated Show resolved Hide resolved
include/fiction/networks/virtual_pi_network.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@hibenj hibenj requested a review from Drewniok December 10, 2024 16:41
hibenj and others added 2 commits December 13, 2024 11:47
# Conflicts:
#	bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp
Signed-off-by: GitHub Actions <[email protected]>
@Drewniok
Copy link
Collaborator

Drewniok commented Dec 16, 2024

I think it makes more sense that @marcelwa is doing a review now as he is more familiar with the topic and may suggest restructuring the code. Afterwards, I will review it again. What do you think?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

* @tparam NodeCostFn A function to compute the costs associated with nodes.
*/
template <typename Ntk, typename NodeCostFn>
class depth_view<Ntk, NodeCostFn, false> : public Ntk
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: class 'depth_view' defines a default destructor, a copy constructor and a copy assignment operator but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class depth_view<Ntk, NodeCostFn, false> : public Ntk
      ^

/**
* Depth view parameters.
*/
depth_view_params _ps;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for member '_ps' [readability-identifier-naming]

include/fiction/networks/views/depth_view.hpp:96:

-             _ps(ps),
+             ps(ps),

include/fiction/networks/views/depth_view.hpp:123:

-             _ps(ps),
+             ps(ps),

include/fiction/networks/views/depth_view.hpp:146:

-             _ps(other._ps),
+             ps(other.ps),

include/fiction/networks/views/depth_view.hpp:169:

-             _ps        = other._ps;
+             ps        = other.ps;

include/fiction/networks/views/depth_view.hpp:257:

-             return _levels[n] = _ps.pi_cost ? _cost_fn(*this, n) - 1 : 0;
+             return _levels[n] = ps.pi_cost ? _cost_fn(*this, n) - 1 : 0;

include/fiction/networks/views/depth_view.hpp:265:

-                                 if (_ps.count_complements && this->is_complemented(f))
+                                 if (ps.count_complements && this->is_complemented(f))

include/fiction/networks/views/depth_view.hpp:285:

-                 if (_ps.count_complements && this->is_complemented(f))
+                 if (ps.count_complements && this->is_complemented(f))

include/fiction/networks/views/depth_view.hpp:298:

-                     if (_ps.count_complements && this->is_complemented(f))
+                     if (ps.count_complements && this->is_complemented(f))

include/fiction/networks/views/depth_view.hpp:336:

-         if (!this->is_constant(n) && !(_ps.pi_cost && this->is_pi(n)))
+         if (!this->is_constant(n) && !(ps.pi_cost && this->is_pi(n)))

include/fiction/networks/views/depth_view.hpp:344:

-                                     if (_ps.count_complements && this->is_complemented(f))
+                                     if (ps.count_complements && this->is_complemented(f))
Suggested change
depth_view_params _ps;
depth_view_params ps;

/**
* Hashmap assigning levels to nodes.
*/
phmap::flat_hash_map<node, uint32_t> _levels;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for member '_levels' [readability-identifier-naming]

include/fiction/networks/views/depth_view.hpp:108:

-         _levels.rehash(this->size());
+         levels.rehash(this->size());

include/fiction/networks/views/depth_view.hpp:135:

-         _levels.rehash(this->size());
+         levels.rehash(this->size());

include/fiction/networks/views/depth_view.hpp:147:

-             _levels(other._levels),
+             levels(other.levels),

include/fiction/networks/views/depth_view.hpp:170:

-             _levels    = other._levels;
+             levels    = other.levels;

include/fiction/networks/views/depth_view.hpp:197:

-         return _levels.at(n);
+         return levels.at(n);

include/fiction/networks/views/depth_view.hpp:213:

-         _levels[n] = level;
+         levels[n] = level;

include/fiction/networks/views/depth_view.hpp:229:

-         _levels.clear();
-         _levels.rehash(this->size());
+         levels.clear();
+         levels.rehash(this->size());

include/fiction/networks/views/depth_view.hpp:246:

-             return _levels[n];
+             return levels[n];

include/fiction/networks/views/depth_view.hpp:252:

-             return _levels[n] = 0;
+             return levels[n] = 0;

include/fiction/networks/views/depth_view.hpp:257:

-             return _levels[n] = _ps.pi_cost ? _cost_fn(*this, n) - 1 : 0;
+             return levels[n] = _ps.pi_cost ? _cost_fn(*this, n) - 1 : 0;

include/fiction/networks/views/depth_view.hpp:272:

-         return _levels[n] = level + _cost_fn(*this, n);
+         return levels[n] = level + _cost_fn(*this, n);

include/fiction/networks/views/depth_view.hpp:310:

-                 if (_levels[n] == _depth)
+                 if (levels[n] == _depth)

include/fiction/networks/views/depth_view.hpp:322:

-                     if (_levels[n] == _depth)
+                     if (levels[n] == _depth)

include/fiction/networks/views/depth_view.hpp:338:

-             const auto lvl = _levels[n];
+             const auto lvl = levels[n];

include/fiction/networks/views/depth_view.hpp:348:

-                                     if (_levels[cn] + offset == lvl && !_crit_path[cn])
+                                     if (levels[cn] + offset == lvl && !_crit_path[cn])
Suggested change
phmap::flat_hash_map<node, uint32_t> _levels;
phmap::flat_hash_map<node, uint32_t> levels;

/**
* Hashmap assigning nodes on the critical path.
*/
phmap::flat_hash_map<node, uint32_t> _crit_path;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for member '_crit_path' [readability-identifier-naming]

include/fiction/networks/views/depth_view.hpp:148:

-             _crit_path(other._crit_path),
+             crit_path(other.crit_path),

include/fiction/networks/views/depth_view.hpp:171:

-             _crit_path = other._crit_path;
+             crit_path = other.crit_path;

include/fiction/networks/views/depth_view.hpp:205:

-         return _crit_path.contains(n);
+         return crit_path.contains(n);

include/fiction/networks/views/depth_view.hpp:231:

-         _crit_path.clear();
-         _crit_path.rehash(this->size());
+         crit_path.clear();
+         crit_path.rehash(this->size());

include/fiction/networks/views/depth_view.hpp:335:

-         _crit_path[n] = true;
+         crit_path[n] = true;

include/fiction/networks/views/depth_view.hpp:348:

-                                     if (_levels[cn] + offset == lvl && !_crit_path[cn])
+                                     if (_levels[cn] + offset == lvl && !crit_path[cn])
Suggested change
phmap::flat_hash_map<node, uint32_t> _crit_path;
phmap::flat_hash_map<node, uint32_t> crit_path;

/**
* The depth of the network.
*/
uint32_t _depth{};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for member '_depth' [readability-identifier-naming]

include/fiction/networks/views/depth_view.hpp:149:

-             _depth(other._depth),
+             depth(other.depth),

include/fiction/networks/views/depth_view.hpp:172:

-             _depth     = other._depth;
+             depth     = other.depth;

include/fiction/networks/views/depth_view.hpp:189:

-         return _depth;
+         return depth;

include/fiction/networks/views/depth_view.hpp:221:

-         _depth = level;
+         depth = level;

include/fiction/networks/views/depth_view.hpp:280:

-         _depth = 0;
+         depth = 0;

include/fiction/networks/views/depth_view.hpp:289:

-                 _depth = std::max(_depth, clevel);
+                 depth = std::max(depth, clevel);

include/fiction/networks/views/depth_view.hpp:302:

-                     _depth = std::max(_depth, clevel);
+                     depth = std::max(depth, clevel);

include/fiction/networks/views/depth_view.hpp:310:

-                 if (_levels[n] == _depth)
+                 if (_levels[n] == depth)

include/fiction/networks/views/depth_view.hpp:322:

-                     if (_levels[n] == _depth)
+                     if (_levels[n] == depth)
Suggested change
uint32_t _depth{};
uint32_t depth{};

{
for (auto l = 0; l < ranks.size(); ++l)
{
foreach_gate_in_rank(l, std::forward<Fn>(fn));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion changes signedness: 'int' to 'uint32_t' (aka 'unsigned int') [clang-diagnostic-sign-conversion]

            foreach_gate_in_rank(l, std::forward<Fn>(fn));
                                 ^
Additional context

test/networks/views/extended_rank_view.cpp:130: in instantiation of function template specialization 'fiction::extended_rank_viewfiction::technology_network::foreach_gate<(lambda at /github/workspace/test/networks/views/extended_rank_view.cpp:131:24)>' requested here

    tec_r.foreach_gate([&node_vector](const auto& n) { node_vector.push_back(n); });
          ^


mockturtle::equivalence_checking_stats st;
const auto maybe_cec_m =
mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(ntk, ntk_r), {}, &st);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

        mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(ntk, ntk_r), {}, &st);
                                          ^

const auto maybe_cec_m =
mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(ntk, ntk_r), {}, &st);
REQUIRE(maybe_cec_m.has_value());
const bool cec_m = *maybe_cec_m;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

    const bool cec_m = *maybe_cec_m;
                        ^


mockturtle::equivalence_checking_stats st;
const auto maybe_cec_m =
mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(tec, vpi), {}, &st);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

        mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(tec, vpi), {}, &st);
                                          ^

const auto maybe_cec_m =
mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(tec, vpi), {}, &st);
REQUIRE(maybe_cec_m.has_value());
const bool cec_m = *maybe_cec_m;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

    const bool cec_m = *maybe_cec_m;
                        ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


mockturtle::equivalence_checking_stats st;
const auto maybe_cec_m =
mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(tec, vpi_r), {}, &st);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

        mockturtle::equivalence_checking(*fiction::virtual_miter<technology_network>(tec, vpi_r), {}, &st);
                                          ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants