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

Replace Tweedledum classical (permutation) gate synthesis routines with native Lisp routines. #740

Merged
merged 12 commits into from
Oct 11, 2021

Conversation

karlosz
Copy link
Contributor

@karlosz karlosz commented Oct 8, 2021

This change reimplements the Tweedledum permutation gate synthesis algorithm using decomposition with a native Lisp one.
The specialized native Lisp routine offers the same 30x speedup in compilation time that Tweedledum gives over our generic routines on the Bernstein-Vazirani test program, incurring only a 5%-10% instruction increase over Tweedledum's synthesis code. This is due to lack of specialized synthesis routines for multiple control Toffoli gates. I believe that once specialized MCT synthesis algorithms are written (as Tweedledum has), we should be able to generate better code as well, since native quilc routines have access to more information, such as target chip architecture and more knowledge about the rest of the compilation pipeline.
A big plus is that we have one less external (foreign!) dependency, also notorious for constantly being rewritten.

@notmgsk
Copy link
Member

notmgsk commented Oct 8, 2021

nice

stylewarning
stylewarning previously approved these changes Oct 8, 2021
Copy link
Member

@stylewarning stylewarning left a comment

Choose a reason for hiding this comment

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

I can't believe my only comment is to use NRECONC.

This is a great addition!

src/build-gate.lisp Show resolved Hide resolved
src/compilers/permutation.lisp Outdated Show resolved Hide resolved
Copy link
Contributor

@braised-babbage braised-babbage 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 looks great. I think in a particular as a drop in replacement for Tweedledum it is super exciting. Essentially all of my comments are documentation or style related. Re: documentation, I think we should be liberal about citing references for specific synthesis techniques that we use. Also I have a strong preference for docstrings, at least in the context of cl-quil where they are the standard. I mentioned this in one specific spot, but as a suggestion I would consider it more broadly.

src/compilers/truth-table.lisp Show resolved Hide resolved
src/compilers/truth-table.lisp Outdated Show resolved Hide resolved
src/compilers/permutation.lisp Show resolved Hide resolved
src/compilers/linear-reversible-circuits.lisp Outdated Show resolved Hide resolved
src/compilers/permutation.lisp Show resolved Hide resolved
src/compilers/linear-reversible-circuits.lisp Outdated Show resolved Hide resolved
tests/permutation-tests.lisp Outdated Show resolved Hide resolved
stylewarning
stylewarning previously approved these changes Oct 11, 2021
@stylewarning
Copy link
Member

stylewarning commented Oct 11, 2021

@karlosz You can clean up commit history if you want. I'll merge this afternoon.

@karlosz
Copy link
Contributor Author

karlosz commented Oct 11, 2021

@stylewarning I just need to address the tests issue raised by Erik and then it should be ready for merge. I'll finish that later today.

@stylewarning
Copy link
Member

@karlosz You can clean up commit history if you want. I'll merge this afternoon.

Whoops, missed that. I agree with @kilimanjaro's comment about test style, too.

@karlosz
Copy link
Contributor Author

karlosz commented Oct 11, 2021

All comments have been addressed now.

tests/permutation-tests.lisp Outdated Show resolved Hide resolved
@stylewarning stylewarning merged commit b9a0eb0 into quil-lang:master Oct 11, 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.

4 participants