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

HLS with coupling map #9250

Closed

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Dec 6, 2022

Summary

This PR adds the argument coupling_map to the HighLevelSynthesis transpiler pass and fully implements two high-level-synthesis plugins for LinearFunctions, PMHSynthesisLinearFunction and KMSSynthesisLinearFunction, both based on the already implemented Patel-Markov-Hayes (PMH) and Kutin-Moulton-Smithline (KMS) synthesis algorithms for linear functions.

The underlying PMH algorithm does not take the coupling map into account, thus the corresponding PMHSynthesisLinearFunction plugin does not do anything when the coupling map is not None. This allows to effectively use this method when synthesizing linear functions on the virtual quantum circuit (that is, before layout and routing take place), but not on physical quantum circuits (after layout and routing).

The underlying KMS algorithm synthesizes linear functions for the linear-nearest-neighbor (LNN) architecture. This allows to use this method on the virtual quantum circuit, and also on the physical quantum circuit, provided that the (placed and routed) linear function is defined over a set of qubit connected by a path. More precisely, this means a hamiltonian path for the coupling map reduced to the set of qubits involved in the linear function. When such a path does not exist, the KMS algorithm does not apply and the KMSSynthesisLinearFunction does not do anything, otherwise the linear function is synthesized over this hamiltonian path.

Each of the two plugins described above also have the option to choose the best implementation when several implementations are available, including picking the best circuit when considering A, A^t, A^{-1} and A^t^{-1} (where A is the matrix representing the linear function), and also to choose the original circuit describing the linear function when present (for instance, when LinearFunction is obtained via CollectLinearFunctions transpiler pass). For the LNN-based synthesis plugin, we can also consider multiple hamiltonian paths and choose the best definition.

This PR does not change the preset pass managers with the now available functionality to collect and to resynthesize linear functions. This requires more thorough benchmarking, but see the (simple) example below illustrating what one can currently do.

Example

This is a very simple example consisting of a single randomly-generated quantum circuit with 6 qubits and 40 CX-gates (and no other gates) that we want to transpile onto the 6-qubit LNN architecture.

[1] First, consider the default run. Initially: #cx = 40, depth = 28. After routing (Sabre): #cx = 40, #swaps = 28, depth = 54 (though depth here is a bit misleading as SWAPs are also counted as having depth 1). After basis translation, we have #cx = 124, depth = 108. The optimization loop (unitary resynthesis of 2-qubit blocks) manages to reduce the numbers a little bit, resulting in #cx = 118, #u = 4, depth = 106.

[2] Now let's modify the init plugin manager stage to additionally resynthesize linear functions using KMS, as follows

pm = generate_preset_pass_manager(
    optimization_level=3,
    coupling_map=coupling_map,
    basis_gates=basis_gates,
    seed_transpiler=0,
)
hls_config_unroll = HLSConfig(linear_function=[("kms", {"all_mats": 1, "max_paths": 100, "orig_circuit": 0})])
init_pm = pm.init
init_pm += PassManager([CollectLinearFunctions()])
init_pm += HighLevelSynthesis(hls_config=hls_config_unroll)
pm.init = init_pm

The idea is that we want to apply the LNN-based synthesis method on the abstract circuit, so that a priori VF2 can find a perfect layout, and no extra swaps would be inserted. Indeed, after resynthesis we have #cx = 57, depth = 27 (Kutin's method tends to introduce quite many gates). No extra swaps are inserted during routing. Optimization loop does not do anything.

[3] Let's instead modify the init plugin manager stage to additionally resynthesize linear functions using PMH. The idea is that the resynthesis algorithm may reduce the number of CX-gates, and hence routing would still be needed, but smaller circuits require fewer SWAPs. We have: after resynthesis #cx = 13, depth = 10 (we got lucky), after routing #cx = 13, #swap = 6, depth = 12, after basis translation #cx = 31, depth = 20.

For each of the above cases, we can also resynthesize linear functions after routing (making sure to keep the pre-synthesized definition not to make things worse). It is important to resynthesize linear functions before resynthesizng 2q blocks using unitary synthesis (or else the CX structure of the circuit will be messed up), this can be achieved by further modifying the pass manager's optimization stage:

hls_config_opt = HLSConfig(linear_function=[("kms", {"all_mats": 1, "max_paths": 100, "orig_circuit": 1})])
opt_pm = PassManager([CollectLinearFunctions()])
opt_pm += HighLevelSynthesis(hls_config=hls_config_opt, coupling_map=coupling_map)
opt_pm += pm.optimization
pm.optimization = opt_pm

[4] Recall that after routing and basis translation in [1] we had #cx = 124, depth = 108. The first linear functions resynthesis in the optimization loop results in #cx = 49, depth = 23. At the very end we have #cx = 43, #u = 7, depth = 22.

[5] Recall that after routing and basis translation in [2] we had #cx = 57, depth = 27. The second time that we resynthesize this using KMS does not do anything, so at the end we have #cx = 57, depth = 27

[6] Recall that after routing and basis translation in [2] we had #cx = 31, depth = 20. The KMS-based resynthesis produce worse functions, so the pre-synthesized circuit is kept, and at the end we have #cx = 31, depth = 20.

Feedback wanted

  • Which other synthesis schemes should be considered now or in the long run?

  • What is the best way to properly benchmark various options, based on which possibly deciding to adjust preset pass managers?

  • What is the right way to compare two linear quantum circuits (and how to properly handle SWAP gates).

  • Different HLS plugins have their own set of kwargs (we actually wanted that from the start). But how to properly document this explaining which options exactly are available?

@qiskit-bot
Copy link
Collaborator

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:

@alexanderivrii alexanderivrii added this to the 0.23.0 milestone Dec 6, 2022
@alexanderivrii alexanderivrii added the mod: transpiler Issues and PRs related to Transpiler label Dec 6, 2022
Comment on lines 312 to 352
def _hamiltonian_paths(
coupling_map: CouplingMap, cutoff: Union[None | int] = None
) -> List[List[int]]:
"""Returns a list of all Hamiltonian paths in ``coupling_map`` (stopping the enumeration when
the number of already discovered paths exceeds the ``cutoff`` value, when specified).
In particular, returns an empty list if there are no Hamiltonian paths.
"""

# This is a temporary function, the plan is to move it to rustworkx

def should_stop():
return cutoff is not None and len(all_paths) >= cutoff

def _recurse(current_node):
current_path.append(current_node)
if len(current_path) == coupling_map.size():
# Discovered a new Hamiltonian path
all_paths.append(current_path.copy())

if should_stop():
return

unvisited_neighbors = [
node for node in coupling_map.neighbors(current_node) if node not in current_path
]
for node in unvisited_neighbors:
_recurse(node)
if should_stop():
return

current_path.pop()

all_paths = []
current_path = []
qubits = coupling_map.physical_qubits

for qubit in qubits:
_recurse(qubit)
if should_stop():
break
return all_paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more efficient to use rustworkx's dfs_search() method to do this traversal: https://qiskit.org/documentation/retworkx/apiref/rustworkx.dfs_search.html#rustworkx.dfs_search which lets you build this as an event driven iterator on the rust side. So you define a visitor class which has hook points in the dfs and rustworkx calls the visitor methods based on it's dfs traversal.

But we definitely should do this in rustworkx natively for the next release because this algorithm can be implemented in parallel fairly easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did not know about the rustworkx's dfs_search method. Though, here we need something akin to "DFS with backtracking", and I was not quite able to figure out if dfs_search can be used for that; that is, can this be implemented with some appropriate visitor? In any case, this function is only temporary as I am planning to follow your suggestion of adding it to rustworkx.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be something that can implement with a specially crafted visitor, but I'm not sure. It's at least not obvious to me quickly scanning the docs and the code. Maybe @georgios-ts knows (as he wrote the functions), but yeah as a temporary step this is fine while waiting on a dedicated function in rustworkx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dfs_search cannot be used since it does not support backtracking as @alexanderivrii correctly pointed out. But all_pairs_all_simple_paths might be a viable option:

res = rustworkx.all_pairs_all_simple_paths(graph, min_depth=graph.num_nodes())

all_hamiltonian_paths = []
for paths_from_node in res.values():
    for paths_from_node_to_target in paths_from_node.values():
        all_hamiltonian_paths.extend(map(list, paths_from_node_to_target))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @georgios-ts! I have already noticed the min-depth option, see my comment here: #9250 (comment). What scares me is what if there is an exponential number of hamiltonian paths, is there a way to limit rustworkx to compute only some fixed number of them?

@coveralls
Copy link

coveralls commented Dec 7, 2022

Pull Request Test Coverage Report for Build 5520254013

  • 156 of 162 (96.3%) changed or added relevant lines in 4 files are covered.
  • 19 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.03%) to 86.014%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/linear/linear_circuits_utils.py 47 49 95.92%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py 100 104 96.15%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/high_level_synthesis.py 1 87.68%
crates/qasm2/src/lex.rs 2 91.14%
crates/accelerate/src/sabre_swap/layer.rs 4 97.32%
crates/qasm2/src/parse.rs 12 97.11%
Totals Coverage Status
Change from base Build 5520034869: 0.03%
Covered Lines: 71877
Relevant Lines: 83564

💛 - Coveralls

@alexanderivrii
Copy link
Contributor Author

alexanderivrii commented Dec 7, 2022

Two relevant points that we discussed during the weekly meeting.

First, @ajavadia has suggested to support an alternative form of specifying synthesis methods for given high-level-objects. In addition to the already supported format

config = HLSConfig(linear_function=[("pmh", {"all_mats": 1})])
pm = PassManager(HighLevelSynthesis(hls_config=config))

we should also support the format

pmh = PMHSynthesisLinearFunction(all_mats=1)
config = HLSConfig(linear_function=[pmh])
pm = PassManager(HighLevelSynthesis(hls_config=config))

This is done in 243027c. Though, the implementation still uses generic **kwargs, so I am not sure whether a Python IDE would be able to automatically fill these.

Second, there was a very interesting suggestion to analyze the coupling map before layout/routing, and based on this analysis to automatically choose the best resynthesis algorithm. One concrete idea was to avoid using the LNN-based KMS method if the coupling map does not have a path of a sufficient length. I really like these suggestions, however I am afraid that it would require a large number of changes to make the init stage of the pass-manager "coupling-map-aware". So I would suggest to treat this in a follow-up PR.

@alexanderivrii alexanderivrii changed the title [WIP] HLS with coupling map HLS with coupling map Dec 7, 2022
@alexanderivrii alexanderivrii marked this pull request as ready for review December 7, 2022 13:33
@mtreinish mtreinish self-assigned this Jan 17, 2023
@mtreinish mtreinish added priority: medium Changelog: New Feature Include in the "Added" section of the changelog labels Jan 17, 2023
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a first pass through the code and had some inline comments. I feel like we need to split this PR into 2 pieces. One that adds the coupling map to the plugin interface and pass, the second which adds an alternative construction mechanism using the raw classes. They're really 2 separate logical changes and bundling makes it hard to review, bisect, and also test the features.

Other than that the only piece I'm a bit concerned about is the interface for how we specify the plugin interface around coupling map. I feel like it needs to be made a bit more explicit for plugin authors that coupling_map is something that will be available to all plugins (assuming the information is provided to the pass) and not something that needs to be manually included as part of the custom settings dictionary.

@@ -0,0 +1,50 @@
---
features:
- |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a separate entry here for the permute() method on the LinearFunction class too.

@@ -89,9 +96,20 @@ class HighLevelSynthesis(TransformationPass):
``default`` methods for all other high-level objects, including ``op_a``-objects.
"""

def __init__(self, hls_config=None):
def __init__(self, coupling_map: CouplingMap = None, hls_config=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility I would reverse these arguments. Python will let you specify keyword arguments positionally by default so putting coupling_map first will break users doing HighLevelSynthesis(HLSConfig(...)) which was valid way to instantiate the pass before.

qiskit/transpiler/passes/synthesis/high_level_synthesis.py Outdated Show resolved Hide resolved
Comment on lines +104 to +105
coupling_map (CouplingMap): the coupling map of the backend
in case synthesis is done on a physical circuit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a Target argument here, similar to what I do in #9263 ? Basically I'm trying to unify our internal transpiler model around the target. My plan for 0.24.0 is to hopefully have the preset pass manager only use a target internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtreinish, so should right now HighLevelSynthesis accept both coupling_map and target, or just target? I guess I am asking whether generate_translation_passmanager (or code further upstream) updates target with coupling_map information if only the latter is specified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it needs to accept both, there is still a code path in transpile where a target will not be generated (I was hoping to change this for 0.24, so it was only target, but it won't make it). You can do it with a second argument or what we did in #9263 is had a single argument take either a coupling map or a target.

Comment on lines +245 to +246
def __init__(self, **options):
self._options = options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this for things that are not directly user specified via the HLSConfig. I'm thinking coupling_map is different here because it's a target constraint and just bundling it via manually specified options by the user (in the HLSConfig) isn't the best interface for plugins. I feel like we really need something like what unitary synthesis is doing that defines the interface options vs the user specified configuration. Right now if I wanted to write a hardware aware synthesis plugin I wouldn't know what is a standard feature of the interface and might end up diverging from the standard interface.

exists a hamiltonian path through the qubits over which this linear function is
defined. When the coupling map is not ``None`` and the hamiltonian path does
not exist, this plugin returns ``None``.
upgrade:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This second note is really a feature note too, but as I mentioned above I feel like this needs to be a separate PR.

Suggested change
upgrade:

@alexanderivrii
Copy link
Contributor Author

Other than that the only piece I'm a bit concerned about is the interface for how we specify the plugin interface around coupling map. I feel like it needs to be made a bit more explicit for plugin authors that coupling_map is something that will be available to all plugins (assuming the information is provided to the pass) and not something that needs to be manually included as part of the custom settings dictionary.

I completely agree that coupling_map (or maybe target?) should be more explicitly passed to HighLevelSynthesisPlugins. Would it make sense to have the run method be of the form def run(self, high_level_object, coupling_map, **options), so that coupling_map would appear there explicitly?

A related question, in UnitarySynthesisPlugins the argument coupling_map actually represents a tuple consisting of the coupling map and qubit indices, would it make sense to do the same for HighLevelSynthesisPlugins, or let coupling_map be the coupling map and keep qubit indices in options?

But what about basis_gates or other things that a synthesis plugin might need to be aware about?

The interface of UnitarySynthesisPlugins is based around the concept that each plugin can return whether it supports basis_gates, coupling_map, etc.. However, this may not be enough for general synthesis algorithms. E.g., we already have synthesis algorithms that work for linear connectivity. That is, they support some coupling maps but not all coupling maps (depending whether a line of given size can be embedded into the connectivity graph). So far I was thinking that a synthesis algorithm is allowed to return None if for some reason it is unable to synthesize the given object (maybe because of connectivity map, because of basis gates, etc.). @mtreinish, what are your thoughts on all this?

@mtreinish
Copy link
Member

I completely agree that coupling_map (or maybe target?) should be more explicitly passed to HighLevelSynthesisPlugins. Would it make sense to have the run method be of the form def run(self, high_level_object, coupling_map, **options), so that coupling_map would appear there explicitly?

The only concern I have with doing it explicitly like that in run with a positional argument is that how does that work from a backwards compatibility PoV? The thing we have to balance in making these additions is to ensure that whatever changes we make to the signature of the run method doesn't force a plugin author to change their code when upgrading. I think we can get away with doing it via a kwarg though, coupling_map=None because for any old plugins out there it will get passed into **options.

A related question, in UnitarySynthesisPlugins the argument coupling_map actually represents a tuple consisting of the coupling map and qubit indices, would it make sense to do the same for HighLevelSynthesisPlugins, or let coupling_map be the coupling map and keep qubit indices in options?

I guess it depends on where/how we're running HighLevelSynthesis the qubit indices in unitary synthesis are to specify which qubits we're synthesizing for. If that is potentially important for HLS (which I think it would be) we probably should pass both. Although for some invocations of HLS we won't know the physical qubits being used for the synthesis. In those cases I think we can leave it blank. Maybe that means we should do it as separate fields instead of as a tuple like in unitary synthesis

But what about basis_gates or other things that a synthesis plugin might need to be aware about?

This is why I like the Target because it's all bundled in one object. :) Maybe we should just do HLS as target only and for backends that don't provide a target we just skip the target aware component? Hopefully for 0.25.0 we'll always have a target. The other option is we need to add a lot of individual fields for all the hardware constraints a synthesis plugin could potentially need (which is what unitary synthesis has).

The interface of UnitarySynthesisPlugins is based around the concept that each plugin can return whether it supports basis_gates, coupling_map, etc.. However, this may not be enough for general synthesis algorithms. E.g., we already have synthesis algorithms that work for linear connectivity. That is, they support some coupling maps but not all coupling maps (depending whether a line of given size can be embedded into the connectivity graph). So far I was thinking that a synthesis algorithm is allowed to return None if for some reason it is unable to synthesize the given object (maybe because of connectivity map, because of basis gates, etc.). @mtreinish, what are your thoughts on all this?

I think that's entirely sensible and a good idea. As long as there is a fallback to try other plugins if a synthesis plugin can return None to indicate it doesn't support running on the selected backend so we don't end up in case where we don't run synthesis when there are still plugins to try. I think we might want to look into a similar mechanism for unitary synthesis.

@kdk kdk modified the milestones: 0.24.0, 0.25.0 Apr 6, 2023
@mtreinish mtreinish modified the milestones: 0.25.0, 0.45.0 Jul 11, 2023
@alexanderivrii
Copy link
Contributor Author

After some delay, I am back thinking about this PR. The alternative form of specifying the HLS config has already been merged in #9413. Other than that, this PR consists of two parts: finalizing the high-level-synthesis plugins interface in the presence of coupling_map and target, and improving specific plugins for synthesizing or resynthesizing linear function, Cliffords, etc. Thus, I am thinking of further splitting this PR into two, which in practice means closing this PR and opening two new ones, but I would still like to use this PR for continuing the discussion.

The more important discussion is on how the plugin interface should look like. Note that @mtreinish has excellent points #9250 (comment) and #9250 (comment). So, a few thoughts:

Personally, I prefer passing target instead of a lot of individual fields. Though we temporarily need coupling_map as well, at least before the issue #9256 is solved.

I am not sure that the interface mechanism of unitary synthesis is sufficient for general synthesis methods. For instance, unitary synthesis has methods like supports_coupling_map, which I believe should return True when the synthesis algorithm can support an arbitrary coupling map. On the other hand, consider synthesis algorithms for Cliffords for dedicated architectures like line connectivity or grid connectivity. The decision whether the particular coupling map is supported will depend on the specific problem: the number of qubits in the Clifford, whether the line or the grid of the right size can be embedded inside the architecture (and passing through relevant qubits). Thinking further, I wouldn't like to restrict synthesis routines in any way, and would like to allow running a synthesis algorithm even if there is a chance that it may not succeed (but we will only know after trying to synthesize it), with the upshot that a plugin can return None if it was not able to synthesize. And we can indeed provide default fallback configurations.

Another thought is that given a huge variety of potential use-cases, we will not be able to choose a single set of options suitable for all possible synthesis routines, i.e. each synthesis algorithm may come with some of its dedicated options that make no sense in other cases. For instance, the options for synthesizing a Clifford might be very different from options for synthesizing an MCX-gate, and then some of specific MCX-synthesis algorithms will have their own set of options like dirty_ancillas (which makes sense for MCXVChain but not for MCXRecursive). Thus, we probably can't avoid passing **options in kwargs-like style.

I don't think that we should pass the whole HLSConfig as an option to a specific synthesis plugin. Currently HLSConfig manages which (lists of) plugins should run for which objects, i.e. which plugins should run for Cliffords, which plugins should run for linear functions, etc., I don't think this information should be available to a specific synthesis plugin for Cliffords.

I agree that it might be nice to separate plugin synthesis options into two sets, things like coupling_map or target (which might be considered the general interface for plugins) vs. plugin-specific options. @mtreinish, your suggestion is:

  • change HighLevelSynthesisPlugin::run from def run(self, high_level_object, **options) to def run(self, high_level_object, coupling_map=None, target=None, qubits=None, **options)
  • change the call from HighLevelSynthesis from decomposition = plugin_method.run(node.op,**plugin_args) to decomposition = plugin_method.run(node.op, coupling_map=self._coupling_map, target=self._target, qubits=[dag_bit_indices[x] for x in node.qargs], **plugin_args) ,
  • change some of the plugins run method to the new form
    This indeed seems to be backward-compatible, moreover more arguments can be exposed as needed.

@1ucian0
Copy link
Member

1ucian0 commented Aug 17, 2023

Is #10477 superseding this one?

@alexanderivrii
Copy link
Contributor Author

Is #10477 superseding this one?

To some extent, yes. There is still one piece left of extending some of the Clifford / LinearFunction / Permutation synthesis plugins to work with the coupling map (somewhat similarly to #10657), but at this point it's best to delegate this to a separate clean PR and to close this one.

@alexanderivrii alexanderivrii deleted the hls-with-coupling-map branch October 23, 2023 07:24
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 mod: transpiler Issues and PRs related to Transpiler priority: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants