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.

Sign up for GitHub

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

improved fast-path detection in HLS #13070

Merged
merged 6 commits into from
Sep 11, 2024
Merged

improved fast-path detection in HLS #13070

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b2bf81
improved fast-path detection in HLS
alexanderivrii Sep 2, 2024
d29d827
Merge branch 'main' into hls
alexanderivrii Sep 11, 2024
b2ea63d
Merge branch 'main' of github.com:Qiskit/qiskit into hls
alexanderivrii Sep 11, 2024
20a355b
Merge branch 'hls' of github.com:alexanderivrii/qiskit-terra into hls
alexanderivrii Sep 11, 2024
0962be4
apply Jake's code review comments
alexanderivrii Sep 11, 2024
0c29c0a
making the comment even more clearer
alexanderivrii Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions qiskit/transpiler/passes/synthesis/high_level_synthesis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,21 @@ def run(self, dag: DAGCircuit) -> DAGCircuit:
return self._run(dag, tracker)

def _run(self, dag: DAGCircuit, tracker: QubitTracker) -> DAGCircuit:
# Check if HighLevelSynthesis can be skipped.
all_skipped = True
for node in dag.topological_op_nodes():
Copy link
Member

Choose a reason for hiding this comment

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

We technically don't need to waste time getting a topological order for this function, we can just look at every op node in arbitrary order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 0962be4.

qubits = tuple(dag.find_bit(q).index for q in node.qargs)
if not (
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to cache this result so we can just look it up in the second iteration over the DAG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean hashing whether a particular node can be skipped? I briefly tried this, but in the main use-case, where only a single pass is required, this extra hashing makes the runtime a bit slower.

dag.has_calibration_for(node)
or len(node.qargs) < self._min_qubits
or node.is_directive()
or self._definitely_skip_node(node, qubits)
):
Copy link
Member

Choose a reason for hiding this comment

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

This set of checks is the exact same set that we do during the loop below - should they be folded into _definitely_skip_node so it's shorter to read and everything stays in sync?

Looking quickly, it seems like the internal logic of HLS has changed slightly since I wrote that fast path within the loop, so the logic around having a separate _definitely_skip_node might be a bit different to when I first wrote it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done in 0962be4.

all_skipped = False
break
if all_skipped:
return dag
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to do this, but in case you're interested, Python has a very uncommon control-flow construction that handles this precise case without an auxiliary variable:

for node in dag.topological_op_nodes():
    qubits = ...
    if not self._should_skip(dag, node, qubits):
        break
else:
    return dag

(The else is attached to the for loop.) Totally up to you if you want to use it - there's plenty of reason not to, since most people don't know what the for/else pattern in Python means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is neat, I like this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 0962be4.


# Start by analyzing the nodes in the DAG. This for-loop is a first version of a potentially
# more elaborate approach to find good operation/ancilla allocations. It greedily iterates
# over the nodes, checking whether we can synthesize them, while keeping track of the
Expand Down Expand Up @@ -312,7 +327,7 @@ def _run(self, dag: DAGCircuit, tracker: QubitTracker) -> DAGCircuit:
# now we are free to synthesize
else:
# this returns the synthesized operation and the qubits it acts on -- note that this
# may be different than the original qubits, since we may use auxiliary qubits
# may be different from the original qubits, since we may use auxiliary qubits
synthesized, used_qubits = self._synthesize_operation(node.op, qubits, tracker)

# if the synthesis changed the operation (i.e. it is not None), store the result
Expand All @@ -335,7 +350,7 @@ def _run(self, dag: DAGCircuit, tracker: QubitTracker) -> DAGCircuit:
if len(synthesized_nodes) == 0:
return dag

# Otherwise we will rebuild with the new operations. Note that we could also
# Otherwise, we will rebuild with the new operations. Note that we could also
# check if no operation changed in size and substitute in-place, but rebuilding is
# generally as fast or faster, unless very few operations are changed.
out = dag.copy_empty_like()
Expand Down Expand Up @@ -642,8 +657,9 @@ def _definitely_skip_node(self, node: DAGOpNode, qubits: tuple[int] | None) -> b
# The fast path is just for Rust-space standard gates (which excludes
# `AnnotatedOperation`).
node.is_standard_gate()
# If it's a controlled gate, we might choose to do funny things to it.
and not node.is_controlled_gate()
# At the moment we don't consider fast-path handling for controlled gates over 3 or
# more controlled qubits. However, we should not abort early for CX/CZ/etc.
and not (node.is_controlled_gate() and node.num_qubits >= 3)
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe this comment has too many negatives in it? I think we do abort early for CX/CZ, right?

Or maybe the comment is referring to "abort _definitely_skip" and I'm reading it as "abort synthesis for this gate", in which case we might want to tweak the words, since it wasn't clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reworded the comment. I have indeed meant that the function _definitely_skip should not immediately return False when seeing a CX-gate or a CZ-gate, even though these are controlled gates.

# If there are plugins to try, they need to be tried.
and not self._methods_to_try(node.name)
# If all the above constraints hold, and it's already supported or the basis translator
Expand Down
Loading