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

DD synthesis of irreversible functions (Encoded to reversible without additional line) #115

Merged

Conversation

SmaranTum
Copy link
Contributor

@SmaranTum SmaranTum commented Nov 25, 2022

Description

This PR addresses the dd synthesis of irreversible functions which are encoded to reversible without any need of an additional line. For the synthesis, don't care conditions must be taken into consideration. This PR introduces the DC node condition based on which the synthesis can be carried forward. Appropriate tests and benchmarks are included as well.

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.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #115 (1ef4b39) into main (87959a2) will increase coverage by 0.0%.
The diff coverage is 95.5%.

@@          Coverage Diff          @@
##            main    #115   +/-   ##
=====================================
  Coverage   92.2%   92.2%           
=====================================
  Files         32      33    +1     
  Lines       2233    2287   +54     
=====================================
+ Hits        2059    2109   +50     
- Misses       174     178    +4     
Impacted Files Coverage Δ
include/algorithms/synthesis/dd_synthesis.hpp 100.0% <ø> (ø)
src/algorithms/simulation/simple_simulation.cpp 82.7% <ø> (ø)
src/algorithms/synthesis/encoding.cpp 98.3% <ø> (+<0.1%) ⬆️
src/core/truthTable/truth_table.cpp 90.9% <80.0%> (-9.1%) ⬇️
include/algorithms/synthesis/encoding.hpp 100.0% <100.0%> (ø)
include/core/truthTable/truth_table.hpp 97.7% <100.0%> (+0.2%) ⬆️
...rc/algorithms/simulation/circuit_to_truthtable.cpp 100.0% <100.0%> (ø)
src/algorithms/synthesis/dd_synthesis.cpp 99.1% <100.0%> (-0.5%) ⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@SmaranTum SmaranTum requested a review from burgholzer November 25, 2022 17:35
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Overall this LGTM. Just a handful suggestions of how to slightly improve this PR.

To be honest I haven't 100% thought through whether this is guaranteed to work. It would be great to have some more tests that ensure it does.

As far as I got it, you are representing a don't care node by having all successors point to the same node. Sounds like a reasonable approach that's fairly easy to check. And I think normalization does not destroy that structure as it just divides every entry by the left-Most maximum value (which is 1).
Please correct me if I got some of that wrong.

src/algorithms/synthesis/dd_synthesis.cpp Outdated Show resolved Hide resolved
src/algorithms/synthesis/dd_synthesis.cpp Outdated Show resolved Hide resolved
src/algorithms/synthesis/dd_synthesis.cpp Outdated Show resolved Hide resolved
test/unittests/test_dd_synthesis_dc.cpp Outdated Show resolved Hide resolved
@SmaranTum SmaranTum requested a review from burgholzer November 26, 2022 15:13
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

I am not quite satisfied with the structure here. You can find detailed comments down below.

include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
src/algorithms/synthesis/dd_synthesis.cpp Outdated Show resolved Hide resolved
src/algorithms/synthesis/dd_synthesis.cpp Outdated Show resolved Hide resolved
test/unittests/test_dd_synthesis_dc.cpp Outdated Show resolved Hide resolved
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Still not happy, but close to.

include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
include/algorithms/simulation/simple_simulation.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
src/algorithms/simulation/simple_simulation.cpp Outdated Show resolved Hide resolved
src/algorithms/simulation/simple_simulation.cpp Outdated Show resolved Hide resolved
@SmaranTum SmaranTum requested a review from burgholzer November 29, 2022 20:50
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Hope this will be the final one...

include/algorithms/simulation/circuit_to_truthtable.hpp Outdated Show resolved Hide resolved
include/core/truthTable/truth_table.hpp Outdated Show resolved Hide resolved
test/unittests/test_dd_synthesis_dc.cpp Outdated Show resolved Hide resolved
@SmaranTum SmaranTum requested a review from burgholzer November 29, 2022 23:26
@burgholzer burgholzer merged commit d0b9631 into cda-tum:main Nov 30, 2022
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.

2 participants