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

Optimization based on same sequence of Toffoli gates #113

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

SmaranTum
Copy link
Contributor

@SmaranTum SmaranTum commented Nov 22, 2022

Description

This PR adds an addition optimization technique which considers nodes that require the same sequence of Toffoli gates in order to get transformed to the identity structure. Hence, reducing the overall control bits and gate count

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.

… the same sequence of Toffoli gates in order to get transformed to the identity structure.
@SmaranTum SmaranTum requested a review from burgholzer November 22, 2022 08:18
@SmaranTum
Copy link
Contributor Author

SmaranTum commented Nov 22, 2022

I think this question is suitable for this PR.

We have considered Cube::Vector in many places. However, we don't require a Cube::Vector in the dd synthesis functionality (since path signature emplaced are always unique). Do we add a Cube::Set? By adding this we can also reduce the complexity of the function introduced in the above PR.

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #113 (df3d0d6) into main (2723f6c) will increase coverage by 0.2%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main    #113     +/-   ##
=======================================
+ Coverage   91.9%   92.2%   +0.2%     
=======================================
  Files         32      32             
  Lines       2225    2233      +8     
=======================================
+ Hits        2045    2059     +14     
+ Misses       180     174      -6     
Impacted Files Coverage Δ
...lude/algorithms/optimization/esop_minimization.hpp 100.0% <ø> (ø)
include/algorithms/synthesis/dd_synthesis.hpp 100.0% <ø> (ø)
include/core/truthTable/truth_table.hpp 97.5% <100.0%> (+<0.1%) ⬆️
src/algorithms/optimization/esop_minimization.cpp 98.5% <100.0%> (-0.1%) ⬇️
src/algorithms/synthesis/dd_synthesis.cpp 99.5% <100.0%> (+2.8%) ⬆️

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

@burgholzer
Copy link
Member

I think this question is suitable for this PR.

We have considered Cube::Vector in many places. However, we don't require a Cube::Vector in the dd synthesis functionality (since path signature emplaced are always unique). Do we add a Cube::Set? By adding this we can also reduce the complexity of the function introduced in the above PR.

Just went through the code and the corresponding paper. I think I only have minor comments/improvements on the code itself. Those can wait until later. As for the suggestion above: That would actually make a lot of sense. This is sort of similar to what the MinTerm data structure in the logic minimization is providing. I'd even say that the MinTerm class could be used throughout, but that would restrict us to 64bits without further modifications and I guess that could be a problem.
I assume you want the path signatures to be unordered_set's which would eliminate the std::is_permutation calls here.
Feel free to go ahead with this.

Reading this, I can only wonder if it would make sense to adopt
kitty's quaternary_truth_table instead of our own TT structure and work based off that. But that might be out of scope for this PR anyway.

@SmaranTum
Copy link
Contributor Author

I think this question is suitable for this PR.
We have considered Cube::Vector in many places. However, we don't require a Cube::Vector in the dd synthesis functionality (since path signature emplaced are always unique). Do we add a Cube::Set? By adding this we can also reduce the complexity of the function introduced in the above PR.

Just went through the code and the corresponding paper. I think I only have minor comments/improvements on the code itself. Those can wait until later. As for the suggestion above: That would actually make a lot of sense. This is sort of similar to what the MinTerm data structure in the logic minimization is providing. I'd even say that the MinTerm class could be used throughout, but that would restrict us to 64bits without further modifications and I guess that could be a problem. I assume you want the path signatures to be unordered_set's which would eliminate the std::is_permutation calls here. Feel free to go ahead with this.

Reading this, I can only wonder if it would make sense to adopt kitty's quaternary_truth_table instead of our own TT structure and work based off that. But that might be out of scope for this PR anyway.

The unordered set stores the signatures in random order right? Does the hash function affect the ordering in the unordered set? What type of hash function would be good for the TruthTable::Cube? just returning the cube.toInterger() be enough?

@burgholzer
Copy link
Member

I think this question is suitable for this PR.
We have considered Cube::Vector in many places. However, we don't require a Cube::Vector in the dd synthesis functionality (since path signature emplaced are always unique). Do we add a Cube::Set? By adding this we can also reduce the complexity of the function introduced in the above PR.

Just went through the code and the corresponding paper. I think I only have minor comments/improvements on the code itself. Those can wait until later. As for the suggestion above: That would actually make a lot of sense. This is sort of similar to what the MinTerm data structure in the logic minimization is providing. I'd even say that the MinTerm class could be used throughout, but that would restrict us to 64bits without further modifications and I guess that could be a problem. I assume you want the path signatures to be unordered_set's which would eliminate the std::is_permutation calls here. Feel free to go ahead with this.
Reading this, I can only wonder if it would make sense to adopt kitty's quaternary_truth_table instead of our own TT structure and work based off that. But that might be out of scope for this PR anyway.

The unordered set stores the signatures in random order right? Does the hash function affect the ordering in the unordered set? What type of hash function would be good for the TruthTable::Cube? just returning the cube.toInterger() be enough?

It does store the elements as a hash table, so yes, the order is random. However, according to https://cplusplus.com/reference/unordered_set/unordered_set/operators/ the complexity of operator== on two unordered sets is linear in the average and quadratic in the worst case.
There is a template specialization for the hash of std::vector<bool>. So, if don't cares are not required for the path signatures, you could go for that (this would also be scalable beyond 64 bits)
cube.toInteger only works up to 64 bits and does not treat don't cares.

@SmaranTum
Copy link
Contributor Author

SmaranTum commented Nov 22, 2022

there is no template for std::vectorstd::optional right? Although the path signatures don't require don't care we end up using the member functions of Truthtable::Cube. Therefore, a lot of changes will have to be incorporated. That is reason why I asked for a suitable hash function for cube.

No, there is no such template specialization and it's not that easy to define a hash function for std::vector<std::optional<bool>>. Technically, you could also opt for using std::set. That should work out of the box, since operator< is defined for Cubes.

@burgholzer
Copy link
Member

No, there is no such template specialization and it's not that easy to define a hash function for std::vector<std::optional>. Technically, you could also opt for using std::set. That should work out of the box, since operator< is defined for Cubes.

@SmaranTum
Copy link
Contributor Author

No, there is no such template specialization and it's not that easy to define a hash function for std::vectorstd::optional. Technically, you could also opt for using std::set. That should work out of the box, since operator< is defined for Cubes.

I will go with this then

@SmaranTum SmaranTum requested a review from burgholzer November 22, 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.

LGTM 👍🏻

@burgholzer burgholzer merged commit 2165b88 into cda-tum:main Nov 25, 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