Add GatesInBasis analysis pass #6832

mtreinish · 2021-07-29T14:12:01Z

Summary

This commit adds a new analysis pass, GatesInBasis, which is used to
check if all the gates in the circuit are currently in the basis set or
not. The intent for this pass is to use this to conditionally trigger
the execution of basis translation (normally the BasisTranslator) in the
main optimization loop of the preset pass managers, however this is
blocked on #6830. So for now this just adds the pass so we can use it in
the future.

Details and comments

This commit adds a new analysis pass, GatesInBasis, which is used to check if all the gates in the circuit are currently in the basis set or not. The intent for this pass is to use this to conditionally trigger the execution of basis translation (normally the BasisTranslator) in the main optimization loop of the preset pass managers, however this is blocked on Qiskit#6830. So for now this just adds the pass so we can use it in the future.

kdk

On the whole, this looks good. This is another place where the Gate/Instruction distinction can be hard to reason about (it took longer than I would've expected to try and think if there were any edge cases between Instruction/Gate, op_nodes/gate_nodes, and basis_gates/supported_instructions, but I do think what's here, combined with where it will be run in the optimization loop, is correct.)

I would've expected though the BasisTranslator to effectively be a no-op if the circuit is already in the basis (and effectively have a version of this check implemented already). Is this not the case? Or does this pass perform that check more quickly?

qiskit/transpiler/passes/utils/gates_basis.py

This commit reworks the GatesInBasis pass to keep track of the instruction counts in the DAGCircuit object whenever we add or remove an op node from the DAG. Then the gates in basis pass can simply lookup against that counts dictionary whether or not all the gates are in the basis or not. This way we avoid a full dag traversal to determine the gates in the dag.

Co-authored-by: John Lapeyre <[email protected]>

qiskit/dagcircuit/dagcircuit.py

jlapeyre · 2021-10-05T21:19:17Z

LGTM. Thanks!

kdk

Thanks @mtreinish , this looks great! Can DAGCircuit.count_ops() use this now? Also, can this be added as a check in test.python.test_dagcircuit.raise_if_dagcircuit_invalid?

qiskit/dagcircuit/dagcircuit.py

mtreinish · 2021-10-05T21:59:37Z

Can DAGCircuit.count_ops() use this now?

Yeah, we can replace the implementation of that with return self._op_names. The only thing is the order might not be the same since we explicitly iterate over the DAG in topological order there while the self._op_names will be in node creation order. If you don't think that's critical I can update it in this PR.

mtreinish · 2021-10-05T22:07:42Z

Also, can this be added as a check in test.python.test_dagcircuit.raise_if_dagcircuit_invalid?

Done in: 215e008

qiskit/dagcircuit/dagcircuit.py

jakelishman · 2021-10-08T09:23:21Z

qiskit/transpiler/passes/utils/gates_basis.py

+    def run(self, dag):
+        """Run the GatesInBasis pass on `dag`."""
+        gates_out_of_basis = False
+        basic_instrs = {"measure", "reset", "barrier", "snapshot", "delay"}


What's the reason for having an extra set of "magic" instructions that always count as being in the basis? Are these ones absolutely guaranteed to always be removed by a later transpiler pass?

If you're looking to ignore directives, it might be better to keep track of the Instruction._directive parameter, but that doesn't fit super nicely with the current set. Alternatively, we could make this an __init__ kwarg, and default to this set if we're not given a value?

This is just mirroring what the basis_translator does https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/passes/basis/basis_translator.py#L85 it's a poor but prevalent assumption the transpiler makes everywhere that these instructions are universal for all backends. (It's something I'm trying to change in #5885 by making the instructions support explicit), but for the time being I couldn't think of any other way to do this. I can add a kwarg but I think it'll just be better to update this for backendv2 when things are ready

Ok cool, if it's a baked-in assumption elsewhere in Terra, we may as well leave it as it is for now.

Co-authored-by: itoko <[email protected]>

kdk · 2021-10-13T19:31:00Z

Can DAGCircuit.count_ops() use this now?

Yeah, we can replace the implementation of that with return self._op_names. The only thing is the order might not be the same since we explicitly iterate over the DAG in topological order there while the self._op_names will be in node creation order. If you don't think that's critical I can update it in this PR.

I'd advocate that the only place we should work to maintain dictionary ordering is where we accept or return an OrderedDict (or otherwise document an expected order). Otherwise, for these cases, we'd need to support (indefinitely?) ordering of returned dictionaries, even if the original ordering was arbitrary.

mtreinish · 2021-10-13T19:40:50Z

I'd advocate that the only place we should work to maintain dictionary ordering is where we accept or return an OrderedDict (or otherwise document an expected order). Otherwise, for these cases, we'd need to support (indefinitely?) ordering of returned dictionaries, even if the original ordering was arbitrary.

Ok sure, done in: dda9f59

mtreinish requested a review from a team as a code owner July 29, 2021 14:12

mtreinish force-pushed the gates-in-basis branch from ad01ec3 to 195e4e1 Compare July 29, 2021 14:34

kdk reviewed Aug 3, 2021

View reviewed changes