-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Make ConsolidateBlocks transpiler pass target aware #7778
Make ConsolidateBlocks transpiler pass target aware #7778
Conversation
This commit updates the consolidate blocks pass target aware so that at its instantiation a Target object can be specified to define the target device for compilation. When specified this target will be used for all device aware operations in the pass. When a target is specified the consolidate blocks pass will only consolidate blocks with instructions outside the target (both operation and qubits). Part of Qiskit#7113
Pull Request Test Coverage Report for Build 2000698215
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks very sensible to me. The new _check_not_in_basis
is a nice refactor of the pass, and Target.instruction_supported
looks useful overall.
releasenotes/notes/consolidate-blocks-target-aware-6482e65d6ee2d18c.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Jake Lishman <[email protected]>
This commit adds a check in the ideal operation case that the number of qargs is equal to the number of qubits the operation object supports. This way if you call instruction_supported() with a qargs parameter that has the wrong number of arguments even an ideally implemented gate will return false. For example, target.instruction_supported(cx, (0, 1, 2)) will return False since a CXGate object only supports 2 qubits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment in line, but it's just a comment not really a suggestion. I'm happy to merge and approve whichever way you decide to go.
if qargs in self._gate_map[operation_name]: | ||
return True | ||
if self._gate_map[operation_name] is None or None in self._gate_map[operation_name]: | ||
return self._gate_name_map[operation_name].num_qubits == len(qargs) and all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thoughts, perhaps checking for Instruction.num_qubits
is not ideal if we anticipate needing to handle any sort gate with variadic arguments, e.g. mcrx
or so. Instruction
lets you do that by having different num_qubits
for the different instances, so the instance attribute isn't necessarily applicable to all objects that share the same gate name.
We don't really have a formal statement of our assumptions about the properties of Instruction
, though, and I think other parts of Terra probably assume that name: num_qubits
is a many-to-one mapping as well, so I'm ok either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as far as the target goes this is fine. If an mcrx gate is in the target it will have a fixed number of qubits as it's set at instantiation, and the target only stores an instance of the operation object as that not the class is what represents the implementation on the backend. Even if this wasn't the case though and it was a class level thing the target and transpiler don't support transpiling to variable width operations yet (I'm thinking of something like global Mølmer–Sørensen gate which an ion system could implement as a native instruction and we'd need a fixed named variant for each number of qubits for the transpiler to work with it). This is something we discussed when creating the target and it's a known limitation with the current implementation, but the target was was a huge incremental step forward over the previous model and provides a great deal more flexibility and expressiveness than was previous available so even if we can't fully model every use case having just everything with fixed sizes for now is fine. Once we get the transpiler up to snuff and fully leveraging what we can express today I think we can look at adding variable width gates to the target model and the transpiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that all sounds plenty convincing to me. I'll merge this now so you can use the new method like you wanted in other PRs.
In Qiskit#7778 a new target method, instruction_supported(), was added to simplify checking if an instruction (operation + qubits) is supported on a target object. This commit switches the GatesInBasis pass to use this internally instead of manually querying the target.
* Make GatesInBasis transpiler pass Target aware This commit makes the GatesInBasis transpiler pass target aware. It adds a new kwarg, target, which optionally takes a Target object. If it's specified the basis_gates required field will be ignored and the pass will check that the gates and their qargs are valid in the target. Part of #7113 * Update qiskit/transpiler/passes/utils/gates_basis.py Co-authored-by: John Lapeyre <[email protected]> * No longer make basis_gates a required argument Now that we can specify a target or basis_gates it's probably best that we don't require basis_gates is always set, especially since a target will always take precedence over the basis_gates list. This commit makes basis_gates optional, but explicitly raises an error if neither basis_gates or target is set on the constructor. * Switch to op_nodes() for internal loop This commit tweaks the internal loop to be over dag.op_nodes() instead of dag.topological_op_nodes() which was used in earlier iterations. In local benchmarking dag.op_nodes() is on average 2x faster than dag.topological_op_nodes(). * Improve performance of basis_gates path This commit improves the performance of the basis_gates code path by combining the basis_gates set with the built-in directives and doing only one set lookup per iteration instead of two. Co-authored-by: John Lapeyre <[email protected]> * Fix support of ideal operations in target This commit fixes the handling of ideal operations in a target. If an operation is listed as ideal (where it applies on all qubits and doesn't have any error or duration information) in a target then the instruction properties inner dict is set to `{None: None}` to indicate this. For the gates in basis pass this just means it needs to check if the qubits for a particular instruction are None, and if it is we can treat it as a match. * Use target instruction_supported() method In #7778 a new target method, instruction_supported(), was added to simplify checking if an instruction (operation + qubits) is supported on a target object. This commit switches the GatesInBasis pass to use this internally instead of manually querying the target. Co-authored-by: John Lapeyre <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
This commit updates the consolidate blocks pass target aware so that at
its instantiation a Target object can be specified to define the target
device for compilation. When specified this target will be used for
all device aware operations in the pass. When a target is specified the
consolidate blocks pass will only consolidate blocks with instructions
outside the target (both operation and qubits).
Details and comments
Part of #7113
TODO: