-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adding MCX synthesis plugins #12961
Adding MCX synthesis plugins #12961
Conversation
…m as HLS synthesis plugins.
…mcx but is not an MCXGate
For better or for worse, C3X and C4X gates have name 'mcx', and therefore are caught by mcx plugins. We need to avoid recursion with HLS calling an MCX-synthesis plugin for a C3X-gate, which in turns returns a C3X-gate.
One or more of the following people are relevant to this code:
|
One other thing: I would like to replace |
Pull Request Test Coverage Report for Build 10503316607Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
A quick experiment on benchpress multi_control_circuit benchmark (see #12729 for some prior numbers): Number of 2-qubit gates:
The improvement comes from better synthesis algorithms that use clean/dirty ancilla qubits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexanderivrii! I've made some preliminary comments (mainly on the functions names and API)
return qc | ||
|
||
|
||
def synth_mcx_mcphase(num_ctrl_qubits: int) -> QuantumCircuit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any better suggestions for this function name? the mcp synth is based on iteratively using mcrz synth.
I would suggest synth_mcx_basic
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't mcrz
use the _mcsu2
work? Maybe we can use that paper as reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking for a better name for this too, so ideas are welcome. I am not particularly fond of synth_mcx_basic
though, as the word "basic" is too generic. By the way, do we have a reference for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is a reference for the mcrz (mcsu2) paper, but this code is a variant that does not appear in the paper.
[1] R. Vale et al. Decomposition of Multi-controlled Special Unitary Single-Qubit Gates
arXiv:2302.06377 (2023) https://arxiv.org/abs/2302.06377
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference; adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that we should find a better name than " synth_mcx_mcphase"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After an offline discussion with @ShellyGarion and @Cryoris, we have settled on synth_v24
. I am not really fond of this or other names, but this is the best we could think of and consistent with the naming of other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should definitely introduce a human-readable alias for these methods, which users can call in the HLS 🙂
Thanks @Cryoris for the review. I have addressed of all your review comments (I think), so it's ready for review once again. The only remaining question (for the rest of the team) is what you think about the current plugin names |
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @alexanderivrii !
I only have one question, I generated the API docs, and could not find the hls_plugins.
Do you know where should they be located now?
@ShellyGarion, thanks for noticing! The docs now correctly appear in: html_docs/html/apidoc/synthesis.html |
@ShellyGarion , thanks for noticing that too. Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for all the effort! I'll wait with putting it the merge queue, if @ShellyGarion still has some comments 🙂
Summary
This PR builds on top of #12904 and #12911 and adds additional MCX synthesis methods and MCX synthesis plugins, some support clean and dirty auxiliary qubits.
The default plugin for MCX gates runs other plugins maximally trying to exploit the sets of clean and dirty ancilla qubits available: it first tries the 'clean-n' method, then the 'dirty-n' method, then the 'clean-1' method, and finally the reduction to mcphase.
The
MCXRecursive
gate and other custom variants are marked for pending deprecation (and will be removed some time in the future).Details and comments
The
high_level_synthesis.py
file was split into two: one containing the transpiler pass, and one containing the plugins.There were a few additional caveats:
MCXRecursive
(and other custom MCX variants) inherit fromMCXGate
but are not called"mcx"
C3XGate
andC4XGate
are called"mcx"
but do not inherit fromMCXGate
.The first point will be cleaned up by deprecating custom MCX variants. The second point might be cleaned up by renaming the names for
C3XGate
andC4XGate
, however might introduce some breaking changes and should be done for 2.0.