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

klut to AIG/XAG/MIG #502

Merged
merged 9 commits into from Nov 6, 2021
Merged

klut to AIG/XAG/MIG #502

merged 9 commits into from Nov 6, 2021

Conversation

costamag
Copy link
Contributor

Work done:

  1. resynthesis function combining Disjoint Support Decomposition, shannon decomposition and NPN-based resynthesis.
    takes as input a k-LUT and returns a AIG, XAG or MIG network.
  2. tests considering 4 cases for the three networks:
    case 1: the function is fully DS-decomposable;
    case 2: one step of DSD and then the NPN mapping is performed;
    case 3: No DSD. One step of shannon and then NPN mapping;
    case 4: One step of DSD, one of Shannon and then NPN mapping.

Copy link
Member

@lee30sonia lee30sonia left a comment

Choose a reason for hiding this comment

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

Great job making so many tests! Just a few comments on little things...

@lee30sonia
Copy link
Member

(Just to clarify) After resolving all the comments, please let me know (explicitly) that I should review it again; otherwise I will take it as still work-in-progress.

@costamag
Copy link
Contributor Author

costamag commented Oct 29, 2021 via email

@lee30sonia
Copy link
Member

I saw there are some unresolved comments above so I thought you haven't completely finished. But now I see, they are outdated. Okay, I'll review again later.

@costamag
Copy link
Contributor Author

costamag commented Oct 30, 2021 via email

@lee30sonia lee30sonia self-requested a review November 5, 2021 15:04
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2021

Codecov Report

Merging #502 (97671a6) into master (1d133e3) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   84.17%   84.25%   +0.07%     
==========================================
  Files         140      142       +2     
  Lines       17336    17377      +41     
==========================================
+ Hits        14593    14641      +48     
+ Misses       2743     2736       -7     
Impacted Files Coverage Δ
include/mockturtle/algorithms/klut_to_graph.hpp 100.00% <100.00%> (ø)
include/mockturtle/networks/mig.hpp 91.04% <0.00%> (-0.92%) ⬇️
...ude/mockturtle/algorithms/node_resynthesis/dsd.hpp 95.45% <0.00%> (ø)
...mockturtle/algorithms/node_resynthesis/xmg_npn.hpp 93.16% <0.00%> (+0.85%) ⬆️
include/mockturtle/networks/xmg.hpp 88.11% <0.00%> (+1.01%) ⬆️
...mockturtle/algorithms/node_resynthesis/xag_npn.hpp 91.17% <0.00%> (+4.41%) ⬆️
...mockturtle/algorithms/node_resynthesis/shannon.hpp 100.00% <0.00%> (+30.76%) ⬆️

Continue to review full report at Codecov.

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

@lee30sonia
Copy link
Member

lee30sonia commented Nov 5, 2021

To speed up the process, I fixed a few things directly by myself:

  • I moved the file out of the node_resynthesis directory because it does not implement a new resynthesis method, but a wrapper to be used by higher-level users.
  • Updates in the documentation: Some are simply writing/wording style, some are syntax issues (e.g. the :math: decoration, tparam for template parameters, insert an empty line to change line, etc.), and also some to enhance understandability for outside users and to reduce repetitions.
  • Fixed inclusion style: Within mockturtle, we should use #include "<relative_path>.hpp". When including with #include <mockturtle/<some_algorithm>.hpp, we are using mockturtle as a system-level library. (See, e.g., here)
  • I don't understand why you need the always_false<T> template class. [Edit: okay, I understand now that this is a work-around that people on the internet suggest to deal with the "ill-formed static_assert". After trying to understand why the standards prohibit this, I decided to do it differently in a way that is even more readable. That is, we check at the beginning of the function that the base type of NtkDest is one of the supported ones. Then, we don't have to worry about the "else" case in the if constexprs.]
  • I removed the parameter and statistics objects because I suppose this function will be used by users who just want to quickly and easily convert the network and would not be interested in making it verbose or seeing the stats. This also simplifies the interface.
  • Finally, I ran clang-format -i <file> to automatically correct spaces, intentations, etc.

@lee30sonia
Copy link
Member

lee30sonia commented Nov 5, 2021

@costamag if you are fine with all the changes, this PR can be merged. Sorry for the delay.

@costamag
Copy link
Contributor Author

costamag commented Nov 6, 2021

dear @lee30sonia, everything is fine for me. Yesterday evening I saw that a typo in a header was giving troubles to the checks but if it is fixed I thank you for the adjustments and for the suggestions and I am fine for the merging.

@lee30sonia lee30sonia merged commit d64f6c0 into lsils:master Nov 6, 2021
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.

None yet

3 participants