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

Adding MCX synthesis plugins #12961

Merged
merged 42 commits into from
Aug 22, 2024
Merged

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Aug 15, 2024

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:

  • The MCXRecursive (and other custom MCX variants) inherit from MCXGate but are not called "mcx"
  • The C3XGate and C4XGate are called "mcx" but do not inherit from MCXGate.

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 and C4XGate, however might introduce some breaking changes and should be done for 2.0.

@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@alexanderivrii
Copy link
Contributor Author

One other thing: I would like to replace _define for C3XGate and C4XGate in x.py by synthesis/multi-controlled/synth_c3x and synth_c4x, but this leads to circular imports (these are singleton gates so the imports seem to be happening on the top-level). Help appreciated.

@coveralls
Copy link

coveralls commented Aug 15, 2024

Pull Request Test Coverage Report for Build 10503316607

Warning: 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

  • 272 of 299 (90.97%) changed or added relevant lines in 7 files are covered.
  • 16 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.001%) to 89.574%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/synthesis/hls_plugins.py 187 214 87.38%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.48%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py 6 96.62%
crates/qasm2/src/parse.rs 6 96.69%
Totals Coverage Status
Change from base Build 10487858724: -0.001%
Covered Lines: 67750
Relevant Lines: 75636

💛 - Coveralls

@alexanderivrii
Copy link
Contributor Author

alexanderivrii commented Aug 15, 2024

A quick experiment on benchpress multi_control_circuit benchmark (see #12729 for some prior numbers):

Number of 2-qubit gates:

  • 10 qubits: tket: 937, before this PR: 1269, with this PR: 809
  • 15 qubits: tket: 3617, before this PR: 5849, with this PR: 2293

The improvement comes from better synthesis algorithms that use clean/dirty ancilla qubits

Copy link
Member

@ShellyGarion ShellyGarion left a 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)

qiskit/synthesis/__init__.py Outdated Show resolved Hide resolved
qiskit/synthesis/multi_controlled/mcx_synthesis.py Outdated Show resolved Hide resolved
return qc


def synth_mcx_mcphase(num_ctrl_qubits: int) -> QuantumCircuit:
Copy link
Member

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 ?

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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"

Copy link
Contributor Author

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.

Copy link
Contributor

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 🙂

qiskit/synthesis/multi_controlled/mcx_synthesis.py Outdated Show resolved Hide resolved
qiskit/circuit/library/standard_gates/x.py Show resolved Hide resolved
qiskit/synthesis/__init__.py Show resolved Hide resolved
@alexanderivrii
Copy link
Contributor Author

alexanderivrii commented Aug 20, 2024

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 "n_clean_m15", "v24", "1_clean_b95" -- these do look a bit ugly, but the best naming convention we could come up so far is to base the names off what they do (e.g., use n clean ancillas) and the name of the first author + the year of the paper describing the technique (e.g., Vale et al's 2024 paper).

Copy link
Member

@ShellyGarion ShellyGarion left a 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?

@alexanderivrii
Copy link
Contributor Author

@ShellyGarion, thanks for noticing! The docs now correctly appear in: html_docs/html/apidoc/synthesis.html

@ShellyGarion
Copy link
Member

Another small comment - the last table in transpiler_synthesis_plugins.html shows the following, so all the references are [1].
So either remove it, or specify first author and a year.
image

@alexanderivrii
Copy link
Contributor Author

@ShellyGarion , thanks for noticing that too. Fixed.

Copy link
Contributor

@Cryoris Cryoris left a 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 🙂

@ShellyGarion ShellyGarion added this pull request to the merge queue Aug 22, 2024
Merged via the queue into Qiskit:main with commit 072548f Aug 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: transpiler Issues and PRs related to Transpiler synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants