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

Introduce input sort #183

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

Introduce input sort #183

wants to merge 126 commits into from

Conversation

hibenj
Copy link
Collaborator

@hibenj hibenj commented Apr 14, 2023

Description

Network_ordering utilizes following files:

  • (NEW_FILE)ortho_ordering_network.hpp: implementation of the algorithm
  • (NEW_FILE)inverter_balancing.hpp: given a fanout node with two inverters as fanouts, the inverters are replaced by one inverter at the fanout-node's input
  • (NEW_FILE)input_ordering_view.hpp: orders the inputs of a network (taking into account the connection between PIs)
  • network_blueprints: included some test_cases relevant for the ordering_network

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • 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.

@marcelwa marcelwa self-requested a review April 14, 2023 14:11
@marcelwa marcelwa self-assigned this Apr 14, 2023
@marcelwa marcelwa added the enhancement New feature or request label Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #183 (eed2c32) into main (bfee388) will decrease coverage by 0.30%.
The diff coverage is 94.21%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   94.55%   94.25%   -0.30%     
==========================================
  Files          83       87       +4     
  Lines        7581     8183     +602     
==========================================
+ Hits         7168     7713     +545     
- Misses        413      470      +57     
Impacted Files Coverage Δ
...orithms/physical_design/ortho_ordering_network.hpp 92.64% <92.64%> (ø)
...ude/fiction/networks/views/input_ordering_view.hpp 95.65% <95.65%> (ø)
...s/network_transformation/inverter_substitution.hpp 96.51% <96.51%> (ø)

... and 4 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 bfee388...eed2c32. Read the comment docs.

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Many thanks for your effort with this contribution! Thus far, I have looked at the inverter balancing part in detail and had a quick look at the tests. Since my comments also point you toward more general topics that apply to all files, I would like to give you the opportunity to address them in all of your files, before I have a look at the remaining source code.

@hibenj
Copy link
Collaborator Author

hibenj commented Apr 20, 2023

GitHub told me that i have to fetch some differences and now there is also a commit in this branch called: [Merge branch 'Introduce_Input_Sort' of], where some changes from your repository (i think) are included. Maybe have a quick look at it if everything is fine there.

@marcelwa
Copy link
Collaborator

GitHub told me that i have to fetch some differences and now there is also a commit in this branch called: [Merge branch 'Introduce_Input_Sort' of], where some changes from your repository (i think) are included. Maybe have a quick look at it if everything is fine there.

That seems fine 👍

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Many thanks for your adjustments. Looks a lot better already!

As before, let's iterate a few files in detail while you can apply the lessons learned to the others as well.

cli/cmd/logic/inverterbalance.hpp Outdated Show resolved Hide resolved
cli/cmd/logic/inverterbalance.hpp Outdated Show resolved Hide resolved
cli/cmd/logic/inverterbalance.hpp Outdated Show resolved Hide resolved
cli/cmd/logic/inverterbalance.hpp Outdated Show resolved Hide resolved
cli/cmd/logic/inverterbalance.hpp Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #183 (a2edcc4) into main (07d5712) will decrease coverage by 0.17%.
Report is 5 commits behind head on main.
The diff coverage is 97.01%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   96.04%   95.87%   -0.17%     
==========================================
  Files         102      106       +4     
  Lines       10101    10720     +619     
==========================================
+ Hits         9701    10278     +577     
- Misses        400      442      +42     
Files Coverage Δ
...ude/fiction/networks/views/input_ordering_view.hpp 99.40% <99.40%> (ø)
...s/network_transformation/inverter_substitution.hpp 92.85% <92.85%> (ø)
...orithms/physical_design/ortho_ordering_network.hpp 97.04% <97.04%> (ø)

... and 5 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 4c66aa4...a2edcc4. 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

experiments/DN_experiments/DN_experiments.cpp Outdated Show resolved Hide resolved
experiments/DN_experiments/DN_experiments.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

@hibenj
Copy link
Collaborator Author

hibenj commented Nov 23, 2023

Do you want to have another look. Now all CIs pass.

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Many thanks for your hard work on this PR 🙏 We're getting closer. There are still some issues to resolve here and there, but they are mainly concerning documentation.

test/networks/views/input_ordering_view.cpp Outdated Show resolved Hide resolved
test/networks/views/input_ordering_view.cpp Outdated Show resolved Hide resolved
test/networks/views/input_ordering_view.cpp Outdated Show resolved Hide resolved
test/algorithms/physical_design/ortho_ordering_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

cli/cmd/physical_design/ortho_ordering.hpp Show resolved Hide resolved
cli/cmd/physical_design/ortho_ordering.hpp Show resolved Hide resolved
cli/cmd/physical_design/ortho_ordering.hpp Show resolved Hide resolved
cli/cmd/physical_design/ortho_ordering.hpp Show resolved Hide resolved
@hibenj hibenj requested a review from marcelwa November 28, 2023 08:12
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion: Instead of doing a whole new command, you could expose input ordering as a flag for ortho if you wanted. It would reduce code duplication. If you rather have it this way, I'm okay with that, too.

cli/cmd/physical_design/ortho_ordering.hpp Outdated Show resolved Hide resolved
cli/cmd/physical_design/ortho_ordering.hpp Outdated Show resolved Hide resolved
docs/publications.rst Outdated Show resolved Hide resolved
docs/publications.rst Outdated Show resolved Hide resolved
include/fiction/traits.hpp Outdated Show resolved Hide resolved
test/algorithms/physical_design/ortho_ordering_network.cpp Outdated Show resolved Hide resolved
test/networks/views/input_ordering_view.cpp Outdated Show resolved Hide resolved
CHECK(nodes_order == expected_nodes_order);
}

TEST_CASE("Input_ordering consider all pis aig network", "[input-ordering-view]")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about other network types? You could make this a TEMPLATE_TEST_CASE to test for multiple implementations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the network type changes also the nodes change, so also the ranking of pis can change.

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

Successfully merging this pull request may close these issues.

2 participants