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

Improve annotated plugin #13916

Open
wants to merge 86 commits into
base: main
Choose a base branch
from

Conversation

alexanderivrii
Copy link
Contributor

Summary

This PR significantly improves the high-level-synthesis plugin for synthesizing AnnotatedOperations and in particular the synthesis of hierarchical circuits involving controlled subcircuits. Addresses #13566. Should be rebased on top of #13813. Does not need #13870, but serves as a basis to that PR explaining why we want to change the default handling of controlled gates to annotated gates.

Details and comments

Here are the experiments for the following 9 simple quantum circuits:

# circuit1: 6-controlled X
qc = QuantumCircuit(15)
qc.append(XGate().control(6), [0, 1, 2, 3, 4, 5, 6])

# circuit2: 5-controlled CX
qc = QuantumCircuit(15)
qc.append(XGate().control(6), [0, 1, 2, 3, 4, 5, 6])

# circuit3: hierarchical circuit morally equivalent to 5-controlled CX
inner = QuantumCircuit(5)
inner.append(CXGate().control(3, annotated=True), [0, 1, 2, 3, 4])
controlled_inner_gate2 = inner.to_gate().control(2, annotated=True)
qc = QuantumCircuit(15)

# circuit4: hierarchical circuit morally equivalent to 5-controlled CX
inner = QuantumCircuit(5)
inner.append(CXGate().control(3, annotated=False), [0, 1, 2, 3, 4])
controlled_inner_gate2 = inner.to_gate().control(2, annotated= False)
qc = QuantumCircuit(15)

# circuits 5, 6, 7, 8: same as 1, 2, 3, 4 but with Z/CZ gates instead of X/CX

#circuit 9: our old friend controlled QFT-based adder
gate = HalfAdderGate(3).control(2, annotated=True)
qc = QuantumCircuit(gate.num_qubits)
qc.append(gate, qc.qubits)

We are going to transpile these circuits with HighLevelSynthesis(basis_gates=["cx", "u"]).

MAIN:

CIRCUIT1: {'u': 57, 'cx': 30}
CIRCUIT2: {'u': 57, 'cx': 30}
CIRCUIT3: {'u': 849, 'cx': 574}
CIRCUIT4: {'u': 1575, 'cx': 974}
CIRCUIT5: {'u': 59, 'cx': 30}
CIRCUIT6: {'u': 159, 'cx': 78}
CIRCUIT7: {'u': 5719, 'cx': 3626}
CIRCUIT8: {'u': 2549, 'cx': 1586}
CIRCUIT9: {'u': 1607, 'cx': 1310}

THIS PR:

CIRCUIT1: {'u': 57, 'cx': 30}
CIRCUIT2: ({'u': 57, 'cx': 30}
CIRCUIT3: {'u': 57, 'cx': 30}
CIRCUIT4: {'u': 1543, 'cx': 990}
CIRCUIT5: {'u': 59, 'cx': 30}
CIRCUIT6: {'u': 59, 'cx': 30}
CIRCUIT7: {'u': 59, 'cx': 30}
CIRCUIT8: {'u': 1573, 'cx': 1002}
CIRCUIT9: {'u': 539, 'cx': 450}

Takeaways: (1) this PR improves the synthesis for almost all of the examples, with especial gains for hierarchical circuits with controlled operations represented as annotated operations; (2) using annotated operations for representing controlled gates is the right way to go forward, as this easily enables reductions and optimizations not available otherwise.

With various conversions of DAGCircuit to QuantumCircuitData to CircuitData in Rust,
it's best to make sure that the global phases appearing in circuits/control-flow subcircuits
are tracked correctly
This leads to significantly improved synthesis results when adding multiple controls to a gate. Previously,
first controlling a CX with 2 controls, and then controlling the results with 3 controls, first resulted in expanding
the CCCX gates into single- and two-qubits gates, adding controls to each of these gates, and then expanding each
of these controlled gates. Now, a single MCX gate will be produced, which will then be expanded using the best MCX
synthesis algorithm available.
This is a simplified version from the unused code from the OptimizeAnnotated transpiler pass.
It's especially useful for controlled QFT-adder and similar circuits.
This allows to let the default QFT-based adder plugins to create the inverse QFTGate as an
AnnotatedOperation, allowing to exploit optimizations in the HighLevelSynthesis transpiler pass
@alexanderivrii alexanderivrii added this to the 2.0.0 milestone Feb 24, 2025
@alexanderivrii alexanderivrii requested a review from a team as a code owner February 24, 2025 14:12
@qiskit-bot
Copy link
Collaborator

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

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

…ed gates from add_control, but with the twist that some other high-level gate names are also supported; fixing failing tests that are synthesized differently (and actually better) using the new flow
…lier synthesis algorithm, and not the synthesis of the underlying QFT gate.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13516621530

Details

  • 732 of 783 (93.49%) changed or added relevant lines in 10 files are covered.
  • 33 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.03%) to 88.008%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/synthesis/plugin.py 1 2 50.0%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py 62 64 96.88%
qiskit/transpiler/passes/synthesis/annotated_synthesis.py 123 133 92.48%
crates/accelerate/src/high_level_synthesis.rs 513 551 93.1%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/plugin.py 1 84.34%
crates/circuit/src/dag_circuit.rs 3 86.07%
crates/qasm2/src/lex.rs 3 93.23%
crates/circuit/src/circuit_instruction.rs 4 90.16%
crates/qasm2/src/parse.rs 6 97.15%
crates/accelerate/src/high_level_synthesis.rs 7 90.27%
crates/circuit/src/dag_node.rs 9 74.55%
Totals Coverage Status
Change from base Build 13512719904: 0.03%
Covered Lines: 77980
Relevant Lines: 88606

💛 - Coveralls

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.

That's a great PR! see some comments and suggestions below.

@@ -109,10 +109,10 @@ def control(

global_phase = 0

basis = ["p", "u", "x", "z", "y", "h", "sx", "sxdg", "rx", "ry", "rz", "cx"]
basis = ["p", "u", "x", "z", "y", "h", "sx", "sxdg", "rx", "ry", "rz", "cx", "cz"]
Copy link
Member

Choose a reason for hiding this comment

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

we should check (in a follow-up PR) what happens with other controlled gates, such as:
cy, ch, cp, crx, cry, crz, cs, csdg, cswap, csx, csxdg, cu,
as well as ccx, ccz, c3x, c4x, c3sx.
Will adding them here improve the total CX cost?

Copy link
Member

Choose a reason for hiding this comment

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

btw, I really don't like the fact that we're copying this array in different part of the code.
I would suggest to set it somewhere and use it, see e.g. the CollectClifford transpiler pass:

@@ -187,7 +187,7 @@ def apply_basic_controlled_gate(circuit, gate, controls, target):
This implements multi-control operations for the following basis gates:
["p", "u", "x", "z", "y", "h", "sx", "sxdg", "rx", "ry", "rz", "cx"]
["p", "u", "x", "z", "y", "h", "sx", "sxdg", "rx", "ry", "rz", "cx", "cz"]
Copy link
Member

Choose a reason for hiding this comment

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

see my comment about this array above.

@@ -230,6 +230,14 @@ def apply_basic_controlled_gate(circuit, gate, controls, target):
controls[:] + [target[0]], # CX has two targets
target[1],
)
elif gate.name == "cz":
Copy link
Member

Choose a reason for hiding this comment

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

btw, what is the improvement in cx-cost of adding cz here?

if not isinstance(high_level_object, AnnotatedOperation):
return None

# Combine the modifiers. If the were no modifiers, or the modifiers magically canceled out,
Copy link
Member

Choose a reason for hiding this comment

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

the --> there

# (including annotated operations) as the HighLevelSynthesis transpiler pass will
# recursively re-synthesize this circuit, However, we should always guarantee that some
# progress is made.
basis = {
Copy link
Member

Choose a reason for hiding this comment

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

see my comment on the array above

"""
res = False

if inst1.qubits != inst2.qubits or inst1.clbits != inst2.clbits or inst1.params != inst2.params:
Copy link
Member

Choose a reason for hiding this comment

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

in standard gates, usually the params of the inverse would be different than the params of the original gate,
for example: the inverse of rz(t) is rz(-t).
it does make sense however to check that both params have the same number of params.

"""
Decomposes a circuit ``A`` into 3 sub-circuits ``P``, ``Q``, ``R`` such that
``A = P -- Q -- R`` and ``R = P^{-1}``.
Copy link
Member

Choose a reason for hiding this comment

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

can you do it more efficiently if you generate the circuit "P - R" and check if some transpiler passes return it to identity?
e.g. InverseCancellation, CommutativeInverseCancellation, CommutativeCancellation, RemoveIdentityEquivalent


class TestHighLevelSynthesisQuality(QiskitTestCase):
"""Test the "quality" of circuits produced by HighLevelSynthesis."""

Copy link
Member

Choose a reason for hiding this comment

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

Are there tests that use several modifiers? the QFT-adder should use both conjuage and control, right?

Copy link
Member

Choose a reason for hiding this comment

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

perhaps this example is also relevant for the conjugate modifier (since ccx gate is self-conjugate):
#6740

qc = QuantumCircuit(gate.num_qubits)
qc.append(gate, qc.qubits)
qct = HighLevelSynthesis(basis_gates=["cx", "u"], qubits_initially_zero=False)(qc)
self.assertLessEqual(qct.count_ops()["cx"], 450)
Copy link
Member

@ShellyGarion ShellyGarion Feb 26, 2025

Choose a reason for hiding this comment

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

is this bound optimal? (assuming that the QFT requires 198). what is the formula for the CX count?
is the bound of 198 for QFT optimal? is there a formula for the CX count?

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.

Improve synthesis of controlled circuits with controlled gates within
4 participants