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

Add CriticalHeuristic to SABRE routing heuristics #13016

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

henryzou50
Copy link
Contributor

Summary

This pull request introduces a new heuristic, CriticalHeuristic, to the SabreSwap pass in Qiskit. The CriticalHeuristic prioritizes routing decisions based on the criticality of quantum operations, as determined by the number of descendant gates in the circuit's Directed Acyclic Graph (DAG). This enhancement provides users with additional control over the routing process, allowing them to optimize circuits by focusing on critical paths, potentially reducing overall circuit depth and/or swap count.

Details and Comments

Key Changes:

  • New Heuristic (CriticalHeuristic) in heuristic.rs:

    • Added the CriticalHeuristic struct, which ranks quantum gates by the number of descendants they have, prioritizing gates with more descendants.
    • The CriticalHeuristic includes weight and scale parameters, allowing users to adjust its impact on routing decisions.
  • Descendant Ranking Map in route.rs:

    • Introduced a new descendants_rank field in the RoutingState struct to store the ranking of nodes based on the number of descendants. This map is populated only when the CriticalHeuristic is enabled.
    • Implemented a new method populate_descendants_rank_map that calculates and ranks nodes based on their number of descendants, storing the results in the descendants_rank map.
  • Integration of CriticalHeuristic in choose_best_swap:

    • Updated the choose_best_swap method to evaluate potential swaps by considering how they enable the routing of gates with high descendant rankings. Swaps that allow the routing of more critical operations are favored.
    • The update_route method and the main routing loop in swap_map_trial have been updated to integrate the CriticalHeuristic, ensuring that routing decisions consider the criticality of operations when this heuristic is used.

Future Considerations:

  • Benchmarking and Scoring Refinement:
    • Further benchmarking is required to assess the impact of the CriticalHeuristic on different types of circuits, particularly those with complex critical paths. Depending on the results, I may need to reconsider how the scoring is calculated to optimize performance across a broader range of circuits.

Notes

  • This pull request is currently a draft as I plan to conduct more benchmarking and testing. The CriticalHeuristic is expected to perform well in circuits where managing critical paths is essential, but its effectiveness across all circuit types needs further validation.
  • Additional tests have been added to ensure the correct integration of the CriticalHeuristic with the SABRE routing framework, and to verify that it interacts as expected with other heuristics.

This commit introduces a new heuristic, `CriticalHeuristic`, to the SABRE routing algorithm in Qiskit. The `CriticalHeuristic` prioritizes routing decisions based on the criticality of quantum operations, as determined by their descendant count in the circuit's Directed Acyclic Graph (DAG).

### Changes in heuristic.rs:
- **CriticalHeuristic Added**:
  - Introduced a new `CriticalHeuristic` struct that ranks quantum gates by the number of descendants they have, assigning a higher priority to gates with more descendants.
  - The `CriticalHeuristic` is implemented with `weight` and `scale` parameters, allowing users to control the heuristic's impact during the routing process.

### Changes in route.rs:
- **Descendant Ranking Map**:
  - Added a new `descendants_rank` field in the `RoutingState` struct to store the ranking of nodes based on the number of descendants. This map is populated only when the `CriticalHeuristic` is enabled.
  - Introduced a new method `populate_descendants_rank_map` that calculates and ranks nodes based on their number of descendants, storing the results in the `descendants_rank` map.

- **CriticalHeuristic Integration**:
  - In the `choose_best_swap` method, the `CriticalHeuristic` now evaluates potential swaps by considering how they enable the routing of gates with high descendant rankings. Swaps that allow the routing of more critical operations are favored.
  - The `update_route` method and the main routing loop in `swap_map_trial` have been updated to integrate the `CriticalHeuristic`, ensuring that routing decisions consider the criticality of operations when this heuristic is used.

### Notes:
- This enhancement gives users the ability to prioritize critical paths during the routing process, potentially leading to more optimized circuits by reducing the risk of bottlenecks and minimizing overall circuit depth.
- We may need to rethink the scoring strategy for the `CriticalHeuristic` based on benchmarking results to ensure it performs optimally across different circuit types and sizes.
@henryzou50 henryzou50 requested a review from a team as a code owner August 22, 2024 16:30
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

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

@henryzou50 henryzou50 added 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 Aug 22, 2024
@henryzou50 henryzou50 added this to the 1.3 beta milestone Aug 22, 2024
@coveralls
Copy link

coveralls commented Aug 22, 2024

Pull Request Test Coverage Report for Build 11666378971

Details

  • 71 of 94 (75.53%) changed or added relevant lines in 3 files are covered.
  • 14 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 88.736%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/sabre/route.rs 61 63 96.83%
crates/accelerate/src/sabre/heuristic.rs 8 29 27.59%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/accelerate/src/two_qubit_decompose.rs 1 92.09%
crates/qasm2/src/lex.rs 6 92.48%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 11665230895: -0.03%
Covered Lines: 76570
Relevant Lines: 86290

💛 - Coveralls

@mtreinish mtreinish modified the milestones: 1.3 beta, 1.3.0 Sep 4, 2024
@henryzou50 henryzou50 self-assigned this Oct 29, 2024
@raynelfss raynelfss assigned jakelishman and unassigned henryzou50 Oct 29, 2024
@henryzou50 henryzou50 changed the title [WIP] Add CriticalHeuristic to SABRE routing heuristics Add CriticalHeuristic to SABRE routing heuristics Nov 4, 2024
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for this, Henry. I'm a bit more suspicious of this PR than the depth one - the depth feels rather more intuitively like it would help, whereas I have a few qualms with both what we're "ranking" as the critical path, how we convert the rank into a modification to the heuristic, and the idea of distilling the base number into a "score".

Do you have more examples of the types of circuits this works on, and how well it works on the types of circuits we have in the utility scale benchmarks (not necessary at that scale yet)?

Comment on lines +173 to +177
/// The relative weighting of this heuristic to others. Typically you should just set this to
/// 1.0 and define everything else in terms of this.
pub weight: f64,
/// Set the dynamic scaling of the weight based on the layer it is applying to.
pub scale: SetScaling,
Copy link
Member

Choose a reason for hiding this comment

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

Same comments from the depth PR on the docstring comments here (they look out of sync) and that the scale thing doesn't really seem to have a well-defined "collection" whose size it should scale relative to.

Comment on lines +232 to +237
elif self.heuristic == "critical":
heuristic = (
Heuristic(attempt_limit=10 * num_dag_qubits)
.with_basic(1.0, SetScaling.Size)
.with_critical(0.5, SetScaling.Size)
)
Copy link
Member

Choose a reason for hiding this comment

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

Similar comments to depth about needing to document this and potentially add it to SabreLayout.

Comment on lines +300 to +308
for node in self.dag.dag.node_indices() {
let mut stack = vec![node];
let mut count = 0;
while let Some(n) = stack.pop() {
count += 1;
for edge in self.dag.dag.edges(n) {
stack.push(edge.target());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

A couple of things here:

  • the for edge in self.dag.dag.edges(n) loop will overcount the descendents of nodes that have multiple paths to the same target. I think what you're calculating here is less the number of elements in the lightcone from this node, and more like the sum of the number of paths from this node to every other node in the graph.
  • I imagine that there's a petgraph or rustworkx function that calculates what you want to calculate here
  • If we do want to do this in Sabre itself, at the moment I think we're doing this fairly inefficiently - there's probably some sort of reverse-topological-iteration approach that lets us re-use information from nodes at the end of the graph to turn the calculation into a sum over the union of sets of descendants that we already encountered. I don't have it very clearly, and it's definitely still going to be important to avoid overcounting nodes that have multiple paths to them, but I do feel like there's something here.

Comment on lines +311 to +319

// Sort nodes by the number of descendants and assign rankings.
let mut desc_list: Vec<_> = node_id_to_descendants.iter().collect();
desc_list.sort_by(|a, b| b.1.cmp(a.1)); // Sort in descending order of descendants

// Populate the `descendants_rank` map with rankings.
for (rank, (node_id, _)) in desc_list.into_iter().enumerate() {
self.descendants_rank.insert(*node_id, rank + 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is imposing a strict total order on data that's non-strictly ordered (there are elements with unequal node IDs that were assigned the same value). That's going to cause problems where certain nodes are prioritised over others based simply on the order they got inserted into the map, if they've got the same number of descendents. At the moment, this is actually going to make this heuristic somewhat dangerously dependent on how the SabreDAG object was constructed - the output of the function changes significantly depending on the assignment of node indices to nodes, which is unrelated to the actual structure of the graph.

I think for many circuits, there should be many nodes that share the same number of total descendants. For example, consider some overly deep and "perfect" QV circuit, constructed entirely of as-yet-unknown 2q gates. For each layer, each gate in the layer should have about the same number of descendents - they're not guaranteed to be exactly the same, but close enough that there'll be a lot of collisions.

In the limit, consider that the final layer of the QV will certainly have all gates having the same number of descendants - zero. I'm concerned that swapping out the number of descendents for the rank is imposing false order on the data, and that the calculated number of descendents might be a bit more appropriate.

Finally: I suspect that the descendents we care about are actually only the nodes that require routing. Anything that's automatically routing shouldn't affect how "critical" a routable node is - a 2q gate followed by 3000 1q gates shouldn't be seen as more critical than a 2q gate followed by 3 other 2q gates for the purposes of routing, I think?

Comment on lines +470 to +500
// Check what gates can be routed after the swap
let mut trial_front_layer = self.front_layer.clone();
trial_front_layer.apply_swap(*swap);

let mut routable_nodes = Vec::<NodeIndex>::with_capacity(2);
if let Some(node) =
trial_front_layer.qubits()[swap[0].index()].and_then(|(node, other)| {
self.target
.coupling
.contains_edge(
NodeIndex::new(swap[0].index()),
NodeIndex::new(other.index()),
)
.then_some(node)
})
{
routable_nodes.push(node);
}
if let Some(node) =
trial_front_layer.qubits()[swap[1].index()].and_then(|(node, other)| {
self.target
.coupling
.contains_edge(
NodeIndex::new(swap[1].index()),
NodeIndex::new(other.index()),
)
.then_some(node)
})
{
routable_nodes.push(node);
}
Copy link
Member

Choose a reason for hiding this comment

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

Given that this turned up in the depth heuristic as well, I feel like some encapsulation of a speculatively_routable_after(swap, out) function might be appropriate. I imagine that that could do much of this, and we could avoid cloning the front layer, since we only make a single modification to it; we could modify it then just revert the modification before returning.

Comment on lines +502 to +507
// For each routable node, substract 10^{-rank} from the score to prioritize routing in tie cases
for node in routable_nodes {
if let Some(rank) = self.descendants_rank.get(&node) {
*score -= weight / 10.0_f64.powi(*rank as i32);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

For large graphs, this is fairly quickly doing to send the denominator to infinity, and so remove its ability to break ties. I think we might need some slightly different way of doing this.

Comment on lines +2 to +24
prelude: >
A new feature has been added to the `SabreSwap` pass in Qiskit that introduces
a `CriticalHeuristic`. This heuristic allows users to prioritize routing
decisions based on the criticality of operations, as determined by the number
of descendant gates in the quantum circuit's Directed Acyclic Graph (DAG).
This enhancement aims to minimize circuit depth by ensuring that more critical
operations, which have the most dependent operations, are routed first.

features_transpiler:
- |
The `CriticalHeuristic` is a new addition to the `SabreSwap` pass in Qiskit.
This heuristic ranks quantum gates based on their criticality, which is determined
by the number of descendant gates they have in the circuit's DAG. Gates with more
descendants are considered more critical and are prioritized during the routing process.

This heuristic works similarly to the existing ones like "basic," "lookahead," or "decay,"
but with a focus on ensuring that the critical path of the quantum circuit is minimized.
This can lead to more efficient routing by reducing the potential for bottlenecks in
the circuit, potentially lowering overall circuit depth.

This new heuristic gives users additional control over the routing process, enabling
them to optimize their circuits by focusing on the critical paths, which can be
particularly beneficial for complex quantum circuits with many dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

Same comments here as on the depth PR.

@raynelfss raynelfss modified the milestones: 1.3.0, 2.0.0 Nov 6, 2024
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 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.

6 participants