Legalization #476
Legalization #476
Conversation
…es have to be written as indicated in the ToDo)
…heck_validity functions
…equivalence checking inside the extended rank view.
…p as constructor.
There was a problem hiding this comment.
This function requires a stand-alone test file that extensively tests its different use cases (datapaths)
There was a problem hiding this comment.
As discussed with @marcelwa and you (@hibenj), I have done a review of the files that have not yet been reviewed by Marcel to assist Marcel since we have many open PRs at the moment.
I left a few tips and comments that Marcel taught me over time. So I would like to pass them on to you :)
I didn't go through the docstrings in detail because I thought it would be helpful to review the code first. Similar problems/issues/recommendations have only been addressed once. So please read all comments first to get a better understanding and overview.
Thanks for the implementation, I really enjoyed the review so far and learned a lot. I hope my feedback helps in some way! 😄
There was a problem hiding this comment.
After a closer look, I am confused by the fact that the class is called extended_rank_view, but it inherits from depth_view. As a result, a significant amount of code has been duplicated. Is there a reason for this?
There was a problem hiding this comment.
Since we are using a cloned version of mockturtle, we could also discuss whether it would make sense to integrate it directly into mockturtle, right? But I am not sure if this is a good idea. I just want to mention it.
| */ | ||
| signal create_virtual_pi(const signal& real_pi) | ||
| { | ||
| signal s = Ntk::create_pi(); |
There was a problem hiding this comment.
| signal s = Ntk::create_pi(); | |
| const signal s = Ntk::create_pi(); |
| */ | ||
| [[nodiscard]] auto get_real_pi(const node& v_pi) const | ||
| { | ||
| auto it = _storage->map_virt_to_real_pi.find(v_pi); |
There was a problem hiding this comment.
| auto it = _storage->map_virt_to_real_pi.find(v_pi); | |
| const auto it = _storage->map_virt_to_real_pi.find(v_pi); |
| /** | ||
| * Shared pointer if the virtual PI storage. | ||
| */ | ||
| std::shared_ptr<virtual_storage<Ntk>> _storage; |
There was a problem hiding this comment.
| std::shared_ptr<virtual_storage<Ntk>> _storage; | |
| storage strg; |
if you add using storage = std::shared_ptr<virtual_storage<Ntk>>; in the first line after public. This is how we do it in fiction.
There was a problem hiding this comment.
Please check which functions can be private or public.
| _storage->map_virt_to_real_pi.clear(); | ||
| } | ||
|
|
||
| protected: |
There was a problem hiding this comment.
| protected: | |
| private: |
?
# Conflicts: # bindings/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp
Signed-off-by: GitHub Actions <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #include "fiction/networks/views/extended_rank_view.hpp" | ||
| #include "fiction/networks/virtual_pi_network.hpp" | ||
| #include "fiction/traits.hpp" | ||
|
|
There was a problem hiding this comment.
warning: included header traits.hpp is not used directly [misc-include-cleaner]
|
|
||
| #include <mockturtle/traits.hpp> | ||
| #include <mockturtle/views/rank_view.hpp> | ||
|
|
There was a problem hiding this comment.
warning: included header rank_view.hpp is not used directly [misc-include-cleaner]
| #include <algorithm> | ||
| #include <cmath> | ||
| #include <deque> | ||
| #include <functional> |
There was a problem hiding this comment.
warning: included header deque is not used directly [misc-include-cleaner]
| #include <functional> | |
| #include <functional> |
| #include <cmath> | ||
| #include <deque> | ||
| #include <functional> | ||
| #include <queue> |
There was a problem hiding this comment.
warning: included header functional is not used directly [misc-include-cleaner]
| #include <queue> | |
| #include <queue> |
| #include <deque> | ||
| #include <functional> | ||
| #include <queue> | ||
| #include <random> |
There was a problem hiding this comment.
warning: included header queue is not used directly [misc-include-cleaner]
| #include <random> | |
| #include <random> |
| class extended_rank_view<Ntk, true> : public mockturtle::depth_view<Ntk> | ||
| { | ||
| public: | ||
| extended_rank_view(const Ntk& ntk) : mockturtle::depth_view<Ntk>(ntk) {} |
There was a problem hiding this comment.
warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
| extended_rank_view(const Ntk& ntk) : mockturtle::depth_view<Ntk>(ntk) {} | |
| explicit extended_rank_view(const Ntk& ntk) : mockturtle::depth_view<Ntk>(ntk) {} |
| * @tparam Ntk The network type. | ||
| */ | ||
| template <class Ntk> | ||
| class extended_rank_view<Ntk, false> : public mockturtle::depth_view<Ntk> |
There was a problem hiding this comment.
warning: class 'extended_rank_view' defines a non-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 extended_rank_view<Ntk, false> : public mockturtle::depth_view<Ntk>
^| class extended_rank_view<Ntk, false> : public mockturtle::depth_view<Ntk> | ||
| { | ||
| public: | ||
| static constexpr bool is_topologically_sorted = true; |
There was a problem hiding this comment.
warning: invalid case style for global constant 'is_topologically_sorted' [readability-identifier-naming]
| static constexpr bool is_topologically_sorted = true; | |
| static constexpr bool IS_TOPOLOGICALLY_SORTED = true; |
| static_assert(mockturtle::has_is_ci_v<Ntk>, "Ntk does not implement the is_ci method"); | ||
| static_assert(mockturtle::has_is_constant_v<Ntk>, "Ntk does not implement the is_constant method"); | ||
|
|
||
| add_event = Ntk::events().register_add_event([this](auto const& n) { on_add(n); }); |
There was a problem hiding this comment.
warning: 'add_event' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
include/fiction/networks/views/extended_rank_view.hpp:84:
- explicit extended_rank_view() : mockturtle::depth_view<Ntk>(), rank_pos{*this}, ranks{}, max_rank_width{0}
+ explicit extended_rank_view() : mockturtle::depth_view<Ntk>(), rank_pos{*this}, ranks{}, max_rank_width{0}, add_event(Ntk::events().register_add_event([this](auto const& n) { on_add(n); }))| add_event = Ntk::events().register_add_event([this](auto const& n) { on_add(n); }); | |
|
|
||
| init_ranks(); | ||
|
|
||
| add_event = Ntk::events().register_add_event([this](auto const& n) { on_add(n); }); |
There was a problem hiding this comment.
warning: 'add_event' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer]
include/fiction/networks/views/extended_rank_view.hpp:107:
- max_rank_width{0}
+ max_rank_width{0}, add_event(Ntk::events().register_add_event([this](auto const& n) { on_add(n); }))| add_event = Ntk::events().register_add_event([this](auto const& n) { on_add(n); }); | |
| #define FICTION_NODE_DUPLICATION_PLANARIZATION_HPP | ||
|
|
||
| #include "fiction/algorithms/network_transformation/network_balancing.hpp" | ||
| #include "fiction/algorithms/properties/check_planarity.hpp" |
There was a problem hiding this comment.
warning: included header network_balancing.hpp is not used directly [misc-include-cleaner]
| #include "fiction/algorithms/properties/check_planarity.hpp" | |
| #include "fiction/algorithms/properties/check_planarity.hpp" |
|
|
||
| #if (PROGRESS_BARS) | ||
| #include <mockturtle/utils/progress_bar.hpp> | ||
| #endif |
There was a problem hiding this comment.
warning: included header progress_bar.hpp is not used directly [misc-include-cleaner]
| #endif | |
| #endif |
| * @param node2 The second node of the fanin-edged node. | ||
| * @param delayValue The delay value for the node. | ||
| */ | ||
| node_pair(mockturtle::node<Ntk> node1, mockturtle::node<Ntk> node2, uint64_t delayValue) : |
There was a problem hiding this comment.
warning: invalid case style for parameter 'delayValue' [readability-identifier-naming]
| node_pair(mockturtle::node<Ntk> node1, mockturtle::node<Ntk> node2, uint64_t delayValue) : | |
| node_pair(mockturtle::node<Ntk> node1, mockturtle::node<Ntk> node2, uint64_t delay_value) : |
include/fiction/algorithms/network_transformation/node_duplication_planarization.hpp:87:
- delay(delayValue)
+ delay(delay_value)| * | ||
| */ | ||
| template <typename NtkSrc> | ||
| [[nodiscard]] extended_rank_view<virtual_pi_network<technology_network>> |
There was a problem hiding this comment.
warning: no header providing "fiction::technology_network" is directly included [misc-include-cleaner]
include/fiction/algorithms/network_transformation/node_duplication_planarization.hpp:9:
- #include "fiction/networks/views/extended_rank_view.hpp"
+ #include "fiction/networks/technology_network.hpp"
+ #include "fiction/networks/views/extended_rank_view.hpp"|
|
||
| if (!check_planarity(result)) | ||
| { | ||
| throw std::runtime_error("Error: Network should be planar"); |
There was a problem hiding this comment.
warning: no header providing "std::runtime_error" is directly included [misc-include-cleaner]
include/fiction/algorithms/network_transformation/node_duplication_planarization.hpp:21:
- #include <unordered_map>
+ #include <stdexcept>
+ #include <unordered_map>| * | ||
| * @return A boolean indicating whether the ranks and rank positions are valid (true) or not (false). | ||
| */ | ||
| bool check_validity() const noexcept |
There was a problem hiding this comment.
warning: function 'check_validity' should be marked [[nodiscard]] [modernize-use-nodiscard]
| bool check_validity() const noexcept | |
| [[nodiscard]] bool check_validity() const noexcept |
| */ | ||
| bool check_validity() const noexcept | ||
| { | ||
| for (std::size_t i = 0; i < ranks.size(); ++i) |
There was a problem hiding this comment.
warning: no header providing "std::size_t" is directly included [misc-include-cleaner]
include/fiction/networks/views/extended_rank_view.hpp:14:
- #include <cstdint>
+ #include <cstddef>
+ #include <cstdint>| * | ||
| * @return Width of the widest rank in the network. | ||
| */ | ||
| uint32_t width() const noexcept |
There was a problem hiding this comment.
warning: function 'width' should be marked [[nodiscard]] [modernize-use-nodiscard]
| uint32_t width() const noexcept | |
| [[nodiscard]] uint32_t width() const noexcept |
| { | ||
| for (auto l = 0; l < ranks.size(); ++l) | ||
| { | ||
| foreach_gate_in_rank(l, std::forward<Fn>(fn)); |
There was a problem hiding this comment.
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
include/fiction/algorithms/network_transformation/network_balancing.hpp:145: in instantiation of function template specialization 'fiction::extended_rank_view<fiction::virtual_pi_networkfiction::technology_network>::foreach_gate<(lambda at /github/workspace/include/fiction/algorithms/network_transformation/network_balancing.hpp:147:13)>' requested here
ntk.foreach_gate(
^include/fiction/algorithms/network_transformation/network_balancing.hpp:256: in instantiation of member function 'fiction::detail::is_balanced_impl<fiction::extended_rank_view<fiction::virtual_pi_networkfiction::technology_network>>::run' requested here
auto result = p.run();
^include/fiction/algorithms/properties/check_planarity.hpp:89: in instantiation of function template specialization 'fiction::is_balanced<fiction::extended_rank_view<fiction::virtual_pi_networkfiction::technology_network>>' requested here
if (!is_balanced(ntk))
^include/fiction/algorithms/network_transformation/node_duplication_planarization.hpp:452: in instantiation of function template specialization 'fiction::check_planarity<fiction::extended_rank_view<fiction::virtual_pi_networkfiction::technology_network>>' requested here
if (!check_planarity(result))
^test/algorithms/network_transformation/node_duplication_planarization.cpp:41: in instantiation of function template specialization 'fiction::node_duplication_planarizationfiction::technology_network' requested here
auto planarized_maj = node_duplication_planarization<technology_network>(tec_b);
^| std::vector<std::vector<node>> ranks; | ||
| uint32_t max_rank_width; | ||
|
|
||
| std::shared_ptr<typename mockturtle::network_events<Ntk>::add_event_type> add_event; |
There was a problem hiding this comment.
warning: no header providing "mockturtle::network_events" is directly included [misc-include-cleaner]
include/fiction/networks/views/extended_rank_view.hpp:8:
- #include <mockturtle/traits.hpp>
+ #include <mockturtle/networks/events.hpp>
+ #include <mockturtle/traits.hpp>Signed-off-by: GitHub Actions <[email protected]>
Signed-off-by: GitHub Actions <[email protected]>
…he virtual_pi generation needed for planarization.
…airs` gets a segmentation fault due to its size
…ation fault. Error: For big networks not the depth/view/rank_view needs to long to be created. (Therefore the size guards are still left in)
Description
Adds the following .hpp and .cpp files:
Molecular QCA Circuits") in order to duplicate nodes on a given PO order. The output is a extended_rank_view of a virtual_pi_network
Fixes # (issue)
Checklist: