new unroll for-loops transpilation pass #9670

1ucian0 · 2023-02-26T13:36:38Z

Summary

This transpilation pass unrolls for loops when possible. Things like for x in {0, 3, 4} {rx(x) qr[1];} will turn into rx(0) qr[1]; rx(3) qr[1]; rx(4) qr[1];.

Details and comments

Loop unrolling is a common optimization which is particularly easy in Qiskit for-loops. There are some exceptions though: when there is a ContinueLoopOp or a BreakLoopOp, which they might be inside a IfElseOp.

qiskit-bot · 2023-02-26T13:36:41Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core

coveralls · 2023-02-26T14:00:39Z

Pull Request Test Coverage Report for Build 4693010276

35 of 36 (97.22%) changed or added relevant lines in 3 files are covered.
151 unchanged lines in 14 files lost coverage.
Overall coverage increased (+0.3%) to 85.767%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/utils/unroll_forloops.py	33	34	97.06%

Files with Coverage Reduction	New Missed Lines	%
qiskit/primitives/backend_sampler.py	1	98.81%
qiskit/transpiler/passes/optimization/template_matching/template_substitution.py	1	93.2%
qiskit/primitives/sampler.py	2	97.1%
qiskit/circuit/quantumcircuit.py	3	93.98%
qiskit/primitives/base/base_sampler.py	3	92.68%
qiskit/primitives/estimator.py	3	96.05%
qiskit/algorithms/minimum_eigensolvers/diagonal_estimator.py	5	86.32%
qiskit/primitives/base/base_estimator.py	5	91.38%
qiskit/primitives/primitive_job.py	5	82.5%
qiskit/primitives/backend_estimator.py	7	95.77%

Totals
Change from base Build 4678110396:	0.3%
Covered Lines:	70319
Relevant Lines:	81988

💛 - Coveralls

jakelishman

This starts getting into questions that I'm not sure we need to be answering at the Qiskit level; it's a purely classical action, and we really don't have the context to know whether it's "better" at this level (given it's been put in optimisations). Unrolling loops might be better if it avoids a tight classical jump from needing to be synced, but equally it could easily blow out program memory on certain controllers. I'm not really sure it's something we should be trying to do at the Qiskit level in the guise of optimisation.

That said, it might not hurt for us to include it as a simple pass for backends that don't support for_loop; we can use it as part of the translation process for those, or even just expose it like we do ConvertCifToIfElse (or whatever it's called) to allow backends an easier upgrade route in their custom stages.

qiskit/transpiler/passes/optimization/unroll_forloops.py

1ucian0 · 2023-02-27T17:30:10Z

I'm not really sure it's something we should be trying to do at the Qiskit level in the guise of "optimisation".

I agree. I'm not advocating for having it in any default pipeline.

…-ref/unroll-forloops

1ucian0 · 2023-02-28T15:48:24Z

but equally it could easily blow out program memory on certain controllers.

Maybe the pass can be configurable for the amount of maximum repetitions that is allow to unroll given the size of the indexset to be known)?

1ucian0 · 2023-02-28T20:48:50Z

but equally it could easily blow out program memory on certain controllers.

Maybe the pass can be configurable for the amount of maximum repetitions that is allow to unroll given the size of the indexset to be known)?

done, with max_target_depth parameter.

jakelishman

Naming nitpicks: can we place this pass in passes.utils instead (next to convert_conditions_to_if_ops.py)? It feels wrong to call it an "optimisation" to me in Qiskit land - we're not a classical compiler nor should we be right now (if ever?), and I think this pass's place is more of a translation utility.

qiskit/transpiler/passes/optimization/unroll_forloops.py

jakelishman · 2023-03-02T00:40:34Z

qiskit/transpiler/passes/optimization/unroll_forloops.py

+
+    @classmethod
+    def body_contains_continue_or_break(cls, circuit):


There's no real reason for this to be a class method (doesn't need to access cls, and this transpiler pass isn't designed/safe for subclassing) - it could just be a stand alone function, but it's not a big deal.

done in b33e1fa

jakelishman · 2023-03-02T00:42:04Z

qiskit/transpiler/passes/optimization/unroll_forloops.py

+    """UnrollForLoops transpilation pass unrolls for-loops when possible. Things like
+    `for x in {0, 3, 4} {rx(x) qr[1];}` will turn into `rx(0) qr[1]; rx(3) qr[1]; rx(4) qr[1];`.
+    """
+
+    def __init__(self, max_target_depth=-1):
+        """UnrollForLoops transpilation pass unrolls for-loops when possible. Things like
+        `for x in {0, 3, 4} {rx(x) qr[1];}` will turn into `rx(0) qr[1]; rx(3) qr[1]; rx(4) qr[1];`.


No need to repeat documentation between the class docstring and __init__ - both get concatenated into the docs output. That said, we need to ensure that qiskit.transpiler.passes.<whichever category> and qiskit.transpiler.passes both import this into their namespace, and the qiskit.transpiler.passes docstring initiates documentation of the class.

Indeed! fixed in fbbcdb7

qiskit/transpiler/passes/optimization/unroll_forloops.py

test/python/transpiler/test_unroll_forloops.py

jakelishman · 2023-03-02T01:08:54Z

qiskit/transpiler/passes/optimization/unroll_forloops.py

+            # skip unrolling if it results in bigger than max_target_depth
+            if self.max_target_depth > 0 and len(indexset) * body.depth() > self.max_target_depth:
+                continue
+


QuantumCircuit.depth doesn't treat control-flow specially, i.e. a single containing for loop looks like it contributes just 1 to the depth.

I think that's probably actually the right choice here, but I think it's worth mentioning: if an inner for loop is not unrolled because of the max-depth constraint (during the depth-first nature of control_flow.trivial_recurse), then a containing for loop is still eligible for unrolling using this metric. I think this behaviour is the right choice, I just think it's worth pointing out. We may even want a test to enforce it.

I extended the docstrings with a ..note:: and added a test in c1966fe and 8039977

Co-authored-by: Jake Lishman <[email protected]>

…-ref/unroll-forloops

1ucian0 · 2023-03-03T14:56:09Z

oh! while showing this to my team, I noticed that parameterless loops (circuit.for_loop(indexset, None, body, [1, 2, 3], [1]) ) do not work. So, on hold. It should be an easy fix.

…-ref/unroll-forloops

1ucian0 · 2023-03-03T19:58:33Z

can we place this pass in passes.utils instead (next to convert_conditions_to_if_ops.py)?

Makes sense to me, done in 41312ad

Co-authored-by: Jake Lishman <[email protected]>

…iskit-terra into feature/no-ref/unroll-forloops

Co-authored-by: Jake Lishman <[email protected]>

…iskit-terra into feature/no-ref/unroll-forloops

1ucian0 · 2023-03-10T20:21:34Z

while showing this to my team, I noticed that parameterless loops (circuit.for_loop(indexset, None, body, [1, 2, 3], [1]) ) do not work. So, on hold. It should be an easy fix.

fixed in 9422e4f and on hold removed

It should be ready to go!

jakelishman

The code here looks good to me (minor debugging print aside). I think we agreed to add this pass but just not include it in default pass managers - it'd be good to get @1ucian0 and @kdk to confirm that I'm remembering that correctly.

test/python/transpiler/test_unroll_forloops.py

mtreinish · 2023-04-13T18:53:28Z

I think we agreed to add this pass but just not include it in default pass managers

This matches my recollection as well.

jakelishman

Ok, that's good enough for me.

* dynamic circuit optimization: unroll for loops * exceptions * check inside conditional blocks * docs * reno * Update qiskit/transpiler/passes/optimization/unroll_forloops.py Co-authored-by: Jake Lishman <[email protected]> * parameterless support * moved to utils * no classmethod, but function * docstring and __init__ * Update qiskit/transpiler/passes/optimization/unroll_forloops.py Co-authored-by: Jake Lishman <[email protected]> * Update test/python/transpiler/test_unroll_forloops.py Co-authored-by: Jake Lishman <[email protected]> * nested for-loops test * docstring note * new test with c_if * Remove commented-out code --------- Co-authored-by: Jake Lishman <[email protected]>

dynamic circuit optimization: unroll for loops

5158eed

jakelishman reviewed Feb 27, 2023

View reviewed changes

qiskit/transpiler/passes/optimization/unroll_forloops.py Outdated Show resolved Hide resolved

1ucian0 added 4 commits February 28, 2023 11:28

Merge branch 'main' of github.com:Qiskit/qiskit-terra into feature/no…

57313e0

…-ref/unroll-forloops

exceptions

eb9b3af

check inside conditional blocks

7f1e42a

docs

5a4e8ce

1ucian0 marked this pull request as ready for review February 28, 2023 20:50

1ucian0 requested a review from a team as a code owner February 28, 2023 20:50

1ucian0 added the Changelog: New Feature Include in the "Added" section of the changelog label Feb 28, 2023

1ucian0 added this to the 0.24.0 milestone Feb 28, 2023

1ucian0 added 2 commits February 28, 2023 22:02

reno

aed085e

Merge branch 'main' into feature/no-ref/unroll-forloops

9001559

jakelishman reviewed Mar 2, 2023

View reviewed changes

1ucian0 and others added 2 commits March 2, 2023 10:25

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

fd8d939

Co-authored-by: Jake Lishman <[email protected]>

Merge branch 'main' of github.com:Qiskit/qiskit-terra into feature/no…

b9f0a5f

…-ref/unroll-forloops

1ucian0 added the on hold Can not fix yet label Mar 3, 2023

1ucian0 added 3 commits March 3, 2023 16:44

Merge branch 'main' of github.com:Qiskit/qiskit-terra into feature/no…

4045b35

…-ref/unroll-forloops

parameterless support

9422e4f

moved to utils

41312ad

1ucian0 and others added 4 commits March 3, 2023 21:01

no classmethod, but function

b33e1fa

docstring and __init__

fbbcdb7

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

5b13707

Co-authored-by: Jake Lishman <[email protected]>

Merge branch 'feature/no-ref/unroll-forloops' of github.com:1ucian0/q…

925e6fc

…iskit-terra into feature/no-ref/unroll-forloops

1ucian0 and others added 5 commits March 3, 2023 21:09

Update test/python/transpiler/test_unroll_forloops.py

ea53541

Co-authored-by: Jake Lishman <[email protected]>

nested for-loops test

c1966fe

Merge branch 'feature/no-ref/unroll-forloops' of github.com:1ucian0/q…

87bc0b0

…iskit-terra into feature/no-ref/unroll-forloops

docstring note

8039977

new test with c_if

c20f236

1ucian0 removed the on hold Can not fix yet label Mar 3, 2023

1ucian0 changed the title ~~dynamic circuit optimization: unroll forloops~~ new unroll for-loops transpilation pass Mar 6, 2023

Merge branch 'main' into feature/no-ref/unroll-forloops

f85ae3c

mtreinish assigned jakelishman Apr 11, 2023

Merge branch 'main' into feature/no-ref/unroll-forloops

be0a50d

jakelishman reviewed Apr 13, 2023

View reviewed changes

test/python/transpiler/test_unroll_forloops.py Outdated Show resolved Hide resolved

Remove commented-out code

c445575

jakelishman approved these changes Apr 13, 2023

View reviewed changes

jakelishman enabled auto-merge April 13, 2023 19:18

jakelishman added this pull request to the merge queue Apr 13, 2023

Merged via the queue into Qiskit:main with commit 7b147a0 Apr 13, 2023

jakelishman mentioned this pull request Apr 20, 2023

[WIP] for-loop optimization pass #10003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new unroll for-loops transpilation pass #9670

new unroll for-loops transpilation pass #9670

1ucian0 commented Feb 26, 2023 •

edited

Loading

qiskit-bot commented Feb 26, 2023

coveralls commented Feb 26, 2023 •

edited

Loading

jakelishman left a comment •

edited

Loading

1ucian0 commented Feb 27, 2023

1ucian0 commented Feb 28, 2023

1ucian0 commented Feb 28, 2023

jakelishman left a comment

jakelishman Mar 2, 2023

1ucian0 Mar 3, 2023 •

edited

Loading

jakelishman Mar 2, 2023

1ucian0 Mar 3, 2023 •

edited

Loading

jakelishman Mar 2, 2023

1ucian0 Mar 3, 2023

1ucian0 commented Mar 3, 2023

1ucian0 commented Mar 3, 2023 •

edited

Loading

1ucian0 commented Mar 10, 2023

jakelishman left a comment

mtreinish commented Apr 13, 2023

jakelishman left a comment


		@classmethod
		def body_contains_continue_or_break(cls, circuit):

new unroll for-loops transpilation pass #9670

new unroll for-loops transpilation pass #9670

Conversation

1ucian0 commented Feb 26, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Feb 26, 2023

coveralls commented Feb 26, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4693010276

💛 - Coveralls

jakelishman left a comment • edited Loading

Choose a reason for hiding this comment

1ucian0 commented Feb 27, 2023

1ucian0 commented Feb 28, 2023

1ucian0 commented Feb 28, 2023

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Mar 2, 2023

Choose a reason for hiding this comment

1ucian0 Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

jakelishman Mar 2, 2023

Choose a reason for hiding this comment

1ucian0 Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

jakelishman Mar 2, 2023

Choose a reason for hiding this comment

1ucian0 Mar 3, 2023

Choose a reason for hiding this comment

1ucian0 commented Mar 3, 2023

1ucian0 commented Mar 3, 2023 • edited Loading

1ucian0 commented Mar 10, 2023

jakelishman left a comment

Choose a reason for hiding this comment

mtreinish commented Apr 13, 2023

jakelishman left a comment

Choose a reason for hiding this comment

1ucian0 commented Feb 26, 2023 •

edited

Loading

coveralls commented Feb 26, 2023 •

edited

Loading

jakelishman left a comment •

edited

Loading

1ucian0 Mar 3, 2023 •

edited

Loading

1ucian0 Mar 3, 2023 •

edited

Loading

1ucian0 commented Mar 3, 2023 •

edited

Loading