Merging UnrollCustomDefinitions into HighLevelSynthesis as a single pass
#10965
Conversation
|
One or more of the the following people are requested to review this:
|
…tting copy_operations to False
Pull Request Test Coverage Report for Build 6545469464
💛 - Coveralls |
|
On a second thought, the number of changes is not that big. Here is the summary:
|
| # If there are no high level operations / annotated gates to synthesize, return fast | ||
| hls_names = set(self.hls_plugin_manager.plugins_by_op) | ||
| node_names = dag.count_ops() | ||
| if "annotated" not in node_names and not hls_names.intersection(node_names): | ||
| return dag |
There was a problem hiding this comment.
I realize there isn't a quick lookup anymore because the equivalence libraries' lookup keys are not pure strings anymore. But it's a real shame to lose this, because it makes the pass much faster in the case when everything is in standard library gates (especially for really large circuits). I think if we did:
gates = {node[0] for node in self._equiv_lib.graph.nodes()}
or
gates = {node[0] for node in self._equiv_lib._key_to_node_index}
That will give us a list of gate names in the equivalence library and we could retain this (along with a corresponding target and/or basis gates lookup). What do you think?
There was a problem hiding this comment.
This is a nice idea, let me experiment with this. I believe that the previous code was unnecessary slow because I was calling dag.substitute_node(node, decomposition) even when node wasn't changed, but now this no longer happens since we are explicitly tracking whether the node was modified.
There was a problem hiding this comment.
An update after thinking about this: intuitively, to restore the previous optimization we need to compute
- A = list of names
dag.count_ops() - B = list of names in equivalence library (the second suggested command works)
- C = list of names supported by the target. Looking at the code, I believe it should be something like
self._target._gate_map.keys() - D = list of names in
self.hls_plugin_manager.plugins_by_op
and check if A is a subset of (B | C) \ D.
However I believe we have the problem for open-controlled gates (as per the code adapted from UnrollCustomDefinitions): a gate's name might be supported by the target or belong to equivalence library, yet because it's open-control we actually can't skip synthesis and use the definition. Any suggestions?
If this is not a trivial task, maybe we can postpone restoring this optimization to a follow-up PR?
There was a problem hiding this comment.
Yeah, I think it's fine to defer it, I didn't have a good answer for this either in UnrollCustomDefinitions for #10788 we could potentially implement this a performance fix for 0.45.0 post rc1 if needed.
| if isinstance(synthesized_op, DAGCircuit): | ||
| synthesized_op = dag_to_circuit(synthesized_op, copy_operations=False) |
There was a problem hiding this comment.
I look forward to the day when the modifier synthesis can operate on dags and we're not using the circuit and or gate methods. The worst case here we are essentially doing:
circuit_to_dag(dag_to_circuit(synthesized_op).to_gate().control().definition)I don't see a way around it for now, but longer term it'd be good if we didn't do this.
Summary
This PR is implemented on top of #9846, following Matthew's review comment below and offline discussions:
Details and comments
In a nutshell, HLS is now combined with UCD and supports recursion. (Not supporting recursion was an oversight in the original implementation).
The constructor for
HighLevelSynthesishas additional argumentsequivalence_library,basis_gates, andmin_qubits,similarly to
UnrollCustomDefinitions.Consider the general case that a quantum circuit may have high-level-objects and annotated gates in addition to custom gates.
When either
targetorbasis_gatesare specified we recursively synthesize/unroll everything till gates in target/basis_gates/equivalence library, in accordance with Matthew's comment above.In particular, if there are no high-level-objects/annotated operations, the behavior of HLS should be exactly the same as of UCD. Testing-wise, I took the two suites for checking the correctness of UCD:
TestUnrollerCompatabilityfromtest_basis_translatorandTestUnrollCustomDefinitionsfromtest_unroll_custom_definitions, and copied them totest_high_level_synthesiswith UCD replaced by HLS.When neither
targetnorbasis_gatesare specified, I aimed to make the behavior both compatible to UCD (which would not do anything) and with the older behavior of HLS (which allowed to synthesize some of the top-level high-level-objects based on theHLSConfig). The pass synthesizes only top-level high-level-objects/annotated gates, but does not unroll gate definitions.I have modified preset pass managers in
common.pyextending HLS with new arguments and removing UCD. It was also important to passmin_qubits=3when unrolling, as forgetting to do so results in HLS synthesizing unitary gates over 2-qubits (and in addition to not wanting to do this, this also results in test failures as it does not use unitary synthesis plugins/respect basis_gates).Things to pay attention to when reviewing:
I may also want to add a few more complex tests for checking corner cases with recursion + combinations of annotated ops/high-level-objects/custom gates. (Update: I did, which required a few fixes to support high-level-objects annotated with control or power modifiers -- after synthesizing the base object and constructing the controlled/powered version of that, we need to unroll the complex circuit definition).