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

Port star_preroute to rust (backport #12761) #12846

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Jul 29, 2024

Summary

Closes #12337

This PR ports the core logic of star_preroute from Python to Rust. The changes involve creating a new Rust module for the star prerouting algorithm and updating the corresponding Python code to integrate with this new Rust functionality.

These changes only involve the first part of porting star_prerouting to rust, as it mainly handles porting the logic of transforming the DAG with the StarBlocks to rust, but we will still need to decide how we should port collecting the StarBlocks to rust. This is a bit more tricky as there are still ongoing discussion on how we can extend the block collection, such as in PR #12498 .

Details and comments

  • New Rust file: Added star_preroute.rs to handle the core logic of the function star_preroute from the python side. This file defines the type aliases for node and block representations, which matches the current block representation of the StarBlock (except that the center is just a bool, as we only need to know if there is a center), and the node representation matches how the nodes used in SabreDAG. The star_preroute function processes the star blocks witihin the SabreDAG to finds the linear routing equivalent and then returns the result as a SabreResult.
  • Node representation: A key part of this implementation is how it takes advantage of SabreResult and SabreDAG, so the node representation is a tuple of the node id, list of qubit indices, a set of classical bit indices, and a directive flag. However, once we update the regular DAG to rust, this part may change significantly, but the main logic will remain unchanged.
  • Updates in the SABRE rust module: To use sabre_dag.rs and swap_map.rs in star_prerouting, I change them to be public in crates/accelerate/src/sabre/mod.rs. Not sure if it makes more sense to do it this way or to move star_prerouting to crates/accelerate/src/sabre/ since it mimics the methods used in Sabre to change the dag.
  • Python side updates: Imported the necessary modules and only modified the function star_preroute so that now the function performs the heavy lifting of transforming the DAG within the Rust space, leveraging _build_sabre_dag and _apply_sabre_result.
  • Issues: I was not sure how correctly handle nodes with control flow from the rust side. I know that route.rs handles this with route_control_flow_block to populate the node_block_results for SabreResult, but I was not sure how to best take advantage of this function for star_prerouting. Currently, the node_block_results for star_prerouting essentially always empty and just there to haveSabreResult. There also seems to be no unit tests for star_prerouting that includes control flow.
    This is an automatic backport of pull request Port star_preroute to rust #12761 done by Mergify.

* This commit ports the core logic of `star_preroute` from Python to Rust. The changes involve creating a new Rust module for the star prerouting algorithm and updating the corresponding Python code to integrate with this new Rust functionality.

Details:
- New Rust file: Added `star_preroute.rs` to handle the core logic of the function `star_preroute` from the python side. This file defines the type aliases for node and block representations, which matches the current block representation of the `StarBlock` (except that the center is just a bool, as we only need to know if there is a center), and the node representation matches how the nodes used in `SabreDAG`. The `star_preroute` function processes the star blocks witihin the `SabreDAG`  and finds the linear routing equivalent and then returns the result as a `SabreResult`. Thus we can use the same methods we used in Sabre, such as `_build_sabre_dag` and `_apply_sabre_result`.
- Node representation: A key part of this implementation is how it takes advantage of `SabreResult` and `SabreDAG`, so the node representation is a tuple of the node id, list of qubit indices, a set of classical bit indices, and a directive flag. However, once we update the regular DAG to rust, this part may change significantly.
- Updates in the SABRE rust module: To use `sabre_dag.rs` and `swap_map.rs` in `star_prerouting`, I change them to be public in `crates/accelerate/src/sabre/mod.rs`. Not sure if it makes more sense to do it this way or to move `star_prerouting` to `crates/accelerate/src/sabre/` since it mimics the methods used in Sabre to change the dag.
- Python side updates: Imported the necessary modules and only modified the function `star_preroute` so that now the function performs the heavy lifting of transforming the DAG within the Rust space, leveraging `_build_sabre_dag` and `_apply_sabre_result`.
- Possible issues: I was not sure how correctly handle control flow from the rust side. I know that `route.rs` handles this with `route_control_flow_block` to populate the `node_block_results` for `SabreResult`, but I was not sure how to best take advantage of this function for `star_prerouting`. Currently, the `node_block_results` for `star_prerouting` essentially always empty and just there to have`SabreResult`. There also seems to be no unit tests for `star_prerouting` that includes control flow.

* lint

* Added additional test and adjust the swap map result

- Added the additional test of qft linearization and that the resultings circuit has `n-2` swap gates where `n` is the number of cp gates.
- Changed the `node_id` in `apply_swap` of `star_preroute.rs` to use the current node id as it is more efficient, but just does not match how we do it in Sabre. This makes it so that we apply the gate first then the swap, which fixes an error we had before where we never placed a swap gate at the end of the processing a block. This only affected tests where we had multiple blocks to process. To make sure we apply the results correctly from `SabreResult`, I added a flag to `_apply_sabre_result` to treat the case of `StarPrerouting` differently so that it applies the swap after applying the node.
- Added a hasp map from node to block to make processing each node in the given processing order have `n + n` time complexity instead of `n^2`. As a result, we can also remove the function `find_block_id`

* Reverted changes to `_apply_sabre_result` and fixed handling on rust side

- Removed `apply_swap_first` flag in `_apply_sabre_result` as it did not make sense to have it as there are no other scenario where a user may want to have control over applying the swap first.
- To adjust for this and make `star_preroute` consistent with `apply_sabre_result` to apply swaps before the node id on the swap map, I adjusted `star_preroute.rs` to first process the blocks to gather the swap locations and the gate order. Once we have the full gate order, we can use the swap locations to apply the swaps while knowing the `qargs` of the node before the swap and the `node_id` of the node after the swap.
- Since the above in done in the main `star_preroute` function, I removed `qubit_ampping` and `out_map` as arguments for `process_blocks`.

(cherry picked from commit b23c545)

# Conflicts:
#	crates/pyext/src/lib.rs
Copy link
Contributor Author

mergify bot commented Jul 29, 2024

Cherry-pick of b23c545 has failed:

On branch mergify/bp/stable/1.2/pr-12761
Your branch is up to date with 'origin/stable/1.2'.

You are currently cherry-picking commit b23c54523.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   crates/accelerate/src/lib.rs
	modified:   crates/accelerate/src/sabre/mod.rs
	new file:   crates/accelerate/src/star_prerouting.rs
	modified:   qiskit/__init__.py
	modified:   qiskit/transpiler/passes/routing/star_prerouting.py
	new file:   releasenotes/notes/port_star_prerouting-13fae3ff78feb5e3.yaml
	modified:   test/python/transpiler/test_star_prerouting.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   crates/pyext/src/lib.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot added the conflicts used by mergify when there are conflicts in a port label Jul 29, 2024
@mergify mergify bot requested a review from a team as a code owner July 29, 2024 16:07
@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 following people are relevant to this code:

  • @Qiskit/terra-core
  • @kevinhartman
  • @mtreinish

@github-actions github-actions bot added performance Changelog: New Feature Include in the "Added" section of the changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Jul 29, 2024
@github-actions github-actions bot added this to the 1.2.0 milestone Jul 29, 2024
ElePT
ElePT previously approved these changes Jul 29, 2024
@ElePT ElePT removed the conflicts used by mergify when there are conflicts in a port label Jul 29, 2024
crates/pyext/src/lib.rs Outdated Show resolved Hide resolved
@ElePT ElePT enabled auto-merge July 29, 2024 16:26
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10148101452

Details

  • 167 of 174 (95.98%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 89.977%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/routing/star_prerouting.py 27 34 79.41%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 92.62%
Totals Coverage Status
Change from base Build 10145758938: 0.03%
Covered Lines: 66331
Relevant Lines: 73720

💛 - Coveralls

@ElePT ElePT added this pull request to the merge queue Jul 29, 2024
Merged via the queue into stable/1.2 with commit 0093c65 Jul 29, 2024
17 checks passed
@mergify mergify bot deleted the mergify/bp/stable/1.2/pr-12761 branch July 29, 2024 19:49
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 performance Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants