Skip to content
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

Merged
merged 11 commits into from
Mar 17, 2022

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Mar 14, 2022

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:

  • Add release note
  • Add tests for new target method
  • Add tests for consolidate block with target

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
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Mar 14, 2022
@mtreinish mtreinish added this to the 0.20 milestone Mar 14, 2022
@mtreinish mtreinish requested a review from a team as a code owner March 14, 2022 22:16
@mtreinish mtreinish added the on hold Can not fix yet label Mar 14, 2022
@coveralls
Copy link

coveralls commented Mar 15, 2022

Pull Request Test Coverage Report for Build 2000698215

  • 17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 83.487%

Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/optimization/consolidate_blocks.py 1 89.66%
Totals Coverage Status
Change from base Build 2000083058: 0.02%
Covered Lines: 52397
Relevant Lines: 62761

💛 - Coveralls

@mtreinish mtreinish changed the title [WIP] Make ConsolidateBlocks transpiler pass target aware Make ConsolidateBlocks transpiler pass target aware Mar 16, 2022
@mtreinish mtreinish removed the on hold Can not fix yet label Mar 16, 2022
Copy link
Member

@jakelishman jakelishman left a 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.

mtreinish and others added 4 commits March 17, 2022 08:02
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.
Copy link
Member

@jakelishman jakelishman left a 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(
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@mergify mergify bot merged commit 79c332e into Qiskit:main Mar 17, 2022
@mtreinish mtreinish deleted the consolidate-blocks-target-aware branch March 17, 2022 22:07
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Mar 18, 2022
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.
mergify bot added a commit that referenced this pull request Mar 30, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants