add control flow to CommutativeCancellation pass #9143

ewinston · 2022-11-16T14:48:44Z

Summary

This adds control flow handling to the CommutativeCancellation pass. In this version the body of the control flow instructions are treated independently of the containing circuit such that the commutivity of the control flow instruction itself is not considered. Also, commutivity across the instruction block boundary is left for a future improvement.

Details and comments

coveralls · 2022-11-16T15:26:19Z

Pull Request Test Coverage Report for Build 5548115012

15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.04%) to 86.036%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	2	91.65%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%

Totals
Change from base Build 5547947273:	0.04%
Covered Lines:	71934
Relevant Lines:	83609

💛 - Coveralls

jakelishman

This does actually seem much more straightforwards than I was worried about - recursing a PassManager itself isn't ideal, but it's not horrendous. We'll also need a release note for this at some point as well.

Can we also add some tests that the commutative cancellation will never try to "commute" some gates out of a control-flow scope, or between control-flow scopes? For example, in the circuit

qc = QuantumCircuit(2, 2)
qc.x(1)
with qc.if_test((0, False)):
    qc.cx(0, 1)
    qc.x(1)

we want to make sure that that's not reduced.

jakelishman · 2023-04-27T15:47:14Z

qiskit/transpiler/passes/optimization/commutative_cancellation.py

+        dag = self._handle_control_flow_ops(dag)
+
+        return dag
+
+    def _handle_control_flow_ops(self, dag):
+        """
+        This is similar to transpiler/passes/utils/control_flow.py except that the
+        commutation analysis is redone for the control flow blocks.
+        """
+        from qiskit.transpiler import PassManager
+
+        for node in dag.op_nodes(ControlFlowOp):
+            mapped_blocks = []
+            for block in node.op.blocks:
+                pass_manager = PassManager(self.__class__(self.basis))
+                new_circ = pass_manager.run(block)
+                mapped_blocks.append(new_circ)
+            node.op = node.op.replace_blocks(mapped_blocks)
        return dag


I think we can just build a single PassManager in the state of self when required here - the requires mechanism should then work on each call.

jlapeyre · 2023-06-23T03:03:09Z

I agree that this is ready to go with a release note. And the additional tests for not crossing a block boundary are not unreasonable.
And moving the creation of the new PassManager out of loop is more efficient.

In _handle_control_flow_ops, a new PassManager was created for each ControlFlowOp found. This commit moves the creation out of the loop over nodes. This should be a bit more efficient. I also considered mapped_blocks.clear() rather than reallocating. But in some simpler tests of clearing rather than reallocating, this is actually less performant. I think reallocating renders the code slightly more understandable.

These were requested in a review comment. The previous commit was also requested in a review comment.

jlapeyre · 2023-06-23T20:22:55Z

EDIT: See note below
@ewinston , @jakelishman , @mtreinish : This may be ready to go if it passes CI

jlapeyre · 2023-06-27T21:43:36Z

Looking closer at this, I don't see how the commutation analysis is run for the blocks in the control flow ops. The tests pass, so I assume it is in fact being run. ~~But, I'd prefer this not be merged until this is explained.~~
EDIT: It seems that this works because a CommutationAnalysis pass is added if not present:
https://github.com/Qiskit/qiskit-terra/blob/d9763523d45a747fd882a7e79cc44c02b5058916/qiskit/transpiler/passes/optimization/commutative_cancellation.py#L63

A slightly confusing circumstance is that in tests (and perhaps other code), the CommutationAnalysis is includes explicitly when constructing the pass manager. Having two ways to do this doesn't feel right. As far as I can tell CommutationAnalysis is only used for CommutativeCancellation, so it might make sense to require it explicitly. The requires mechanism is meant to support including passes that may be used by more than one subsequent pass.

The tests include this line PassManager([CommutationAnalysis(), CommutativeCancellation()]).... We should consider including CommutationAnalysis explicitly when constructing a pass manager in _handle_control_flow_ops. Or at any rate be consistent in terra code in how we construct pass managers that include CommutativeCancellation.

In any case, it seems that the approach used here obviates the need to add explicit support for control flow ops in CommutationAnalysis

jlapeyre · 2023-06-28T00:39:30Z

Arg. I somehow merged commits from main in this branch. resetting ...

…locks A new PassManager for CommutativeCancellation is constructed when descending into control flow blocks. This commit explicitly includes a CommutativeAnalysis pass in this construction rather than relying on `requires`. This change, while not necessary, is made for consistency with other explicit constructions of PassManagers containing these passes.

mtreinish

Overall this LGTM, the approach of using a nested pass manager is clever to handle running both passes in the nested context. Just a couple of small questions inline.

mtreinish · 2023-07-05T14:44:41Z

qiskit/transpiler/passes/optimization/commutative_cancellation.py

+        This is similar to transpiler/passes/utils/control_flow.py except that the
+        commutation analysis is redone for the control flow blocks.
+        """
+        from qiskit.transpiler import PassManager


Is there a cyclical import issue that requires us to do this at runtime?

We can improve this the same way it's done in #10355

EDIT: reverted the following and committed suggestion above.
~~Fixed in f8c2aaf.~~

qiskit/transpiler/passes/optimization/commutative_cancellation.py

A previous commit made a copy of the pass being used in the parent context to be used in the blocks of a control flow op. With this commit, we reuse the existing pass, as it is immutable.

Co-authored-by: Matthew Treinish <[email protected]>

mtreinish

This LGTM, I think this works as intended and the test coverage that @jakelishman asked for earlier is present now. That being said I left one inline comment, it's not necessarily a blocker and it's something we can experiment with later. But I'm curious your thoughts about it.

mtreinish · 2023-07-13T21:48:22Z

qiskit/transpiler/passes/optimization/commutative_cancellation.py

+        commutation analysis is redone for the control flow blocks.
+        """
+
+        pass_manager = PassManager([CommutationAnalysis(), self])


I was debating asking to move this to an instance scope, something like: self.control_flow_pm because CommutationAnalysis maintains a cache of commutation relationships which might be useful between multiple runs for >1 control flow block. But my hesitation is around whether sharing the cache between the blocks is sound or not.

It should be safe to reuse the cache between different blocks or even runs, since all it stores are gate identities like "CX(0, 1) commutes with Z(0)"; "CX(0, 1) does not commute with S(0)", etc.. I don't know if sharing this information would lead to a performance improvement, I guess it depends how many blocks there are and how similar these are in terms of gates.

Let's move forward here, we can always add this in a quick follow up without too much concern then

ewinston added 2 commits November 16, 2022 09:17

add initial tests

760fbfc

tests passing

aeb571e

ewinston requested a review from a team as a code owner November 16, 2022 14:48

This comment was marked as outdated.

Sign in to view

ewinston added this to the 0.24.0 milestone Feb 8, 2023

Merge branch 'main' into controlflow/commutative_cancellation

dfb8d31

mtreinish assigned jakelishman Apr 11, 2023

kdk assigned jlapeyre and unassigned jakelishman Apr 18, 2023

mtreinish modified the milestones: 0.24.0, 0.25.0 Apr 19, 2023

ewinston added 4 commits April 19, 2023 11:24

remove debug code

450e678

Merge branch 'main' into controlflow/commutative_cancellation

935d787

fix test for clbits

ad93783

formatting

292b0d7

jakelishman reviewed Apr 27, 2023

View reviewed changes

mtreinish mentioned this pull request Jun 8, 2023

Support control flow in CommutationAnalysis and CommutativeCancellation #9423

Closed

Merge branch 'main' into controlflow/commutative_cancellation

02036f0

jlapeyre added 4 commits June 23, 2023 09:41

Merge branch 'main' into controlflow/commutative_cancellation

6f30dcf

Test commutative cancellation does not cross block boundaries

68a8fce

These were requested in a review comment. The previous commit was also requested in a review comment.

Add release note for commutative cancellation in control flow blocks

b6cdab4

jlapeyre force-pushed the controlflow/commutative_cancellation branch from bf7e8e1 to bd78e5a Compare June 28, 2023 00:37

jlapeyre requested review from nonhermitian, eggerdj and wshanks as code owners June 28, 2023 00:37

jlapeyre requested a review from ikkoham as a code owner June 28, 2023 00:37

jlapeyre force-pushed the controlflow/commutative_cancellation branch from bd78e5a to b6cdab4 Compare June 28, 2023 00:40

jlapeyre removed request for wshanks, nonhermitian, ikkoham and eggerdj June 28, 2023 00:43

jlapeyre mentioned this pull request Jun 28, 2023

Support control flow in ConsolidateBlocks #10355

Merged

6 tasks

kevinhartman mentioned this pull request Jul 3, 2023

Allow circuits w/ control flow in O2 and O3 #10372

Merged

mtreinish reviewed Jul 5, 2023

View reviewed changes

kevinhartman mentioned this pull request Jul 5, 2023

[NFM] Integration testing for CF in O2 and O3 #10390

Closed

mtreinish self-assigned this Jul 10, 2023

jlapeyre and others added 7 commits July 12, 2023 16:57

Use existing CommutativeCancellation pass in control flow op blocks

f8c2aaf

A previous commit made a copy of the pass being used in the parent context to be used in the blocks of a control flow op. With this commit, we reuse the existing pass, as it is immutable.

Move import of PassManager from method to top of file

6cce375

Revert commit f8c2aaf because I missed a fix already to go

cdb6d60

Update qiskit/transpiler/passes/optimization/commutative_cancellation.py

074fb21

Co-authored-by: Matthew Treinish <[email protected]>

Merge branch 'main' into controlflow/commutative_cancellation

433ac38

Merge branch 'main' into controlflow/commutative_cancellation

eeab1c6

Merge branch 'main' into controlflow/commutative_cancellation

3906813

mtreinish approved these changes Jul 13, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Jul 18, 2023

Merged via the queue into Qiskit:main with commit 733f78c Jul 18, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add control flow to CommutativeCancellation pass #9143

add control flow to CommutativeCancellation pass #9143

ewinston commented Nov 16, 2022

This comment was marked as outdated.

coveralls commented Nov 16, 2022 •

edited

Loading

jakelishman left a comment

jakelishman Apr 27, 2023

jlapeyre commented Jun 23, 2023

jlapeyre commented Jun 23, 2023 •

edited

Loading

jlapeyre commented Jun 27, 2023 •

edited

Loading

jlapeyre commented Jun 28, 2023 •

edited

Loading

mtreinish left a comment

mtreinish Jul 5, 2023

jlapeyre Jul 12, 2023

jlapeyre Jul 12, 2023 •

edited

Loading

mtreinish left a comment

mtreinish Jul 13, 2023

alexanderivrii Jul 15, 2023

mtreinish Jul 18, 2023

add control flow to CommutativeCancellation pass #9143

add control flow to CommutativeCancellation pass #9143

Conversation

ewinston commented Nov 16, 2022

Summary

Details and comments

This comment was marked as outdated.

coveralls commented Nov 16, 2022 • edited Loading

Pull Request Test Coverage Report for Build 5548115012

💛 - Coveralls

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Apr 27, 2023

Choose a reason for hiding this comment

jlapeyre commented Jun 23, 2023

jlapeyre commented Jun 23, 2023 • edited Loading

jlapeyre commented Jun 27, 2023 • edited Loading

jlapeyre commented Jun 28, 2023 • edited Loading

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jul 5, 2023

Choose a reason for hiding this comment

jlapeyre Jul 12, 2023

Choose a reason for hiding this comment

jlapeyre Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Jul 13, 2023

Choose a reason for hiding this comment

alexanderivrii Jul 15, 2023

Choose a reason for hiding this comment

mtreinish Jul 18, 2023

Choose a reason for hiding this comment

coveralls commented Nov 16, 2022 •

edited

Loading

jlapeyre commented Jun 23, 2023 •

edited

Loading

jlapeyre commented Jun 27, 2023 •

edited

Loading

jlapeyre commented Jun 28, 2023 •

edited

Loading

jlapeyre Jul 12, 2023 •

edited

Loading