Port star_preroute to rust
#12761
Merged
Port star_preroute to rust
#12761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…st. 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.
|
One or more of the following people are relevant to this code:
|
mtreinish
added
Changelog: New Feature
Pull Request Test Coverage Report for Build 10144562567Details
💛 - Coveralls |
sbrandhsn
requested changes
Jul 16, 2024
There was a problem hiding this comment.
This mostly looks great to me, thanks! :-) I left a couple of in-line comments that should partially be addressed in my opinion. Apart from this:
- Do you think porting SPR to Rust requires any extra testing?
- I think a good additional test would be to route our standard QFT definition and check whether the resulting circuit requires n-1 swap gates where n is the number of CNOT gates and check whether the QFT circuit has been linearized (you can run and check whether #edges -1 == #vertices)?
- Can you run a quick benchmark on QFT to see how the runtime improves? I'm thinking about just routing the QFT circuit and measuring the time of doing that (no ASV or so required).
WRT circuits with classical feedback, I think it is fine to just have it as is right now. Ideally, we would check whether we wanted to run SPR at all and a ControlFlowOp may very likely result in blocking a SPR run.
- 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`
where only the first and last cp gates do not have a corresponding swap gate.
Which shows that this PR is about 21% faster. |
|
@sbrandhsn Thank you for your comments and I finished adding the changes! While doing these changes, I also made another change of changing how the |
|
Looks good, thanks for addressing these. Yeah it is Thanks for your work. I will accept the PR as soon as |
…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`.
Thanks! No, I do not see a scenario where a user may want to have control over |
|
LGTM! Can you resolve the merge conflict? :-) I would be happy to approve then! Thanks! |
Done! Thanks :) |
ElePT
added
the
stable backport potential
mergify bot
pushed a commit
that referenced
this pull request
Jul 29, 2024
* 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
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 29, 2024
* Port `star_preroute` to rust (#12761) * 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 * Update lib.rs * Update crates/pyext/src/lib.rs --------- Co-authored-by: Henry Zou <87874865+henryzou50@users.noreply.github.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Procatv
pushed a commit
to Procatv/qiskit-terra-catherines
that referenced
this pull request
Aug 1, 2024
* 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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #12337
This PR ports the core logic of
star_preroutefrom 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_preroutingto 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
star_preroute.rsto handle the core logic of the functionstar_preroutefrom the python side. This file defines the type aliases for node and block representations, which matches the current block representation of theStarBlock(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 inSabreDAG. Thestar_preroutefunction processes the star blocks witihin theSabreDAGto finds the linear routing equivalent and then returns the result as aSabreResult.SabreResultandSabreDAG, 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.sabre_dag.rsandswap_map.rsinstar_prerouting, I change them to be public incrates/accelerate/src/sabre/mod.rs. Not sure if it makes more sense to do it this way or to movestar_preroutingtocrates/accelerate/src/sabre/since it mimics the methods used in Sabre to change the dag.star_prerouteso that now the function performs the heavy lifting of transforming the DAG within the Rust space, leveraging_build_sabre_dagand_apply_sabre_result.route.rshandles this withroute_control_flow_blockto populate thenode_block_resultsforSabreResult, but I was not sure how to best take advantage of this function forstar_prerouting. Currently, thenode_block_resultsforstar_preroutingessentially always empty and just there to haveSabreResult. There also seems to be no unit tests forstar_preroutingthat includes control flow.