Use only discrete-basis translations in GateDirection

[jakelishman]

Summary

This makes it a little more likely that it'll be possible to use the transpiler when targetting a discrete basis, such as a Clifford simulator. We still in general do not offer full support for discrete-basis translation, and certainly not for optimal discrete-basis transpilation, but this is a relatively easy change that makes us marginally more reliable with them.

Details and comments

Fix #10785.

I'm vaguely concerned about a potential performance regression here by us making the basis translator and resynthesis routines do more work for real-world backends, but I couldn't immediately think what benchmarks, if any, we have to run about that.

edit: I'm not 100% convinced that this PR is something we should do, but it's a proof of principle.

[qiskit-bot]

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

• @Qiskit/terra-core

[jakelishman]

The tests here will actually have some problems because they'd need a global phase within control-flow blocks. It's not actually clear to me how much we physically need to track that (once there's control flow, it's not possible to get a unitary representation of the circuit anyway), but even if we were to, we currently don't have a way to do that via the control-flow builder interface.

[1ucian0]

That means this PR is blocked on #10800?

[jakelishman]

Yeah, or something like that. It's also not 100% clear that this PR is definitely the right direction in the general case - I might need to benchmark to see if there's any performance regression, and if so, make the pass dynamically choose which ecr template to use based on the target.

[kdk]

I'm vaguely concerned about a potential performance regression here by us making the basis translator and resynthesis routines do more work for real-world backends, ...

For the majority of active devices (maybe all that use ecr at least), wouldn't we already need to translate and re-synthesize away from ry? Is there somewhere this would lead to more effort on the part of the transpiler?

That means this PR is blocked on #10800?

Agree that's a bug, but it seems to be pre-existing. Does it need to block the PR here?

... make the pass dynamically choose which ecr template to use based on the target.

Agree this seems like the right long term direction.

[jakelishman]

[worried about resynthesis routines doing more work]

The trick here is the existing template inserts only two ry gates that need resynthesis for most ecr-supporting backends, whereas the new template inserts six gates of which the four s and sdgs will need resynthising, and the resulting 1q gate chain will be longer, which means the subsequent 1q optimiser will have to do more work to see if it can combine them. I don't know that that'll have a measurable performance impact or anything, I was just flagging it as something to check.

[Does global-phase in the control-flow builders need to block this?]

In practice it's hard and fiddly to update the tests in this PR to respect the new behaviour without it. Not impossible, but assuming the global-phase PR is not especially contentious, it's much more convenient just to merge that first.

[jakelishman]

Since #10800 is merged, I've updated the tests in this PR.

I ran a simple test for any performance regressions with this script:

from qiskit import transpile
from qiskit.circuit.library import EfficientSU2
from qiskit.providers.fake_provider import FakeSherbrooke

qc = EfficientSU2(100, reps=10)
backend = FakeSherbrooke()
transpile(qc, backend=backend, seed_transpiler=0)

%timeit [transpile(qc, backend=backend, seed_transpiler=i) for i in range(10)]

since O1 is the level we'd expect to see the largest different at anyway, and Sherbrooke is an ECR-based backend.

On main I found it took 38.7(3)s, and this branch took 38.9(1)s, so there's nothing really to mention between the two.

[mtreinish]

I'm fine with doing this, in the general case it won't fundamentally change the performance or output quality afaict. There might be some subtle changes/edge cases for the output for level 0 in some cases because we run gate direction followed by translation only. So it's using the equivalence library to translate from s, sdg, sx to -> target instead of from ry, which might return a different transform based on the exact rules in the equivalence library (the other opt levels run Optimize1qGatesDecomposition which will normalize to the same sequence based on the target and matrix of the sequence).

Longer term it'd be good if we could handle the translation of Ry(pi/2) to a discrete basis directly in the equivalence library since I don't think anything in there precludes us from adding it in theory (there are probably issues around handling discrete parameter values on matching, but we can address those). But in the short term I think this is fine.

[mtreinish]

From a performance PoV there isn't a huge difference either way. For each ECR gate we flip we end up doing two extra products here: https://github.com/Qiskit/qiskit/blob/main/qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py#L197 but they're all 2x2 so it'll be fast. There might actually be an immeasurable speed up because we don't have to build 2 matrices for the ry gates when to_matrix() is called. From a memory overhead perspective I think with #10314 this is actually more efficient because 2 Ry(..) is about 1kb and this ends up being 6 pointers (assuming SGate() and SdgGate() are instantiated somewhere already, SXGate should be because it's always in the default IBM basis with ECR).

[jakelishman]

oh, that's actually true about memory usage, the gate arrays and the singleton objects - I hadn't been thinking about that at all. I think I wrote this PR before we landed any of the singleton-gate stuff, so it wasn't on my mind at the time.