Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.11k stars 2.34k forks source link

Sabre routing does not insert current swaps to out_map when doing forcing routing #13011

Closed chcfz closed 1 month ago

chcfz commented 1 month ago

Environment

What is happening?

In Sabre routing, when the program doing force routing, it will call drain to get an iter of current_swaps and then get a force routed node, but because of the feature of drain, these swaps in current_swaps will not be inserted to out_maps, which will lead to an wrong transpiled circuit.

force routing (574 in route.rs)

 if routable_nodes.is_empty() {
            // If we exceeded the max number of heuristic-chosen swaps without making progress,
            // unwind to the last progress point and greedily swap to bring a ndoe together.
            // Efficiency doesn't matter much; this path never gets taken unless we're unlucky.
            current_swaps
                .drain(..)
                .rev()
                .for_each(|swap| state.apply_swap(swap));
            let force_routed = state.force_enable_closest_node(&mut current_swaps);
            routable_nodes.push(force_routed);
        }
state.update_route(&routable_nodes, current_swaps);

update_route (110 in route.rs) which insert current swaps to out_map

fn update_route(&mut self, nodes: &[NodeIndex], swaps: Vec<[PhysicalQubit; 2]>) {
        // First node gets the swaps attached.  We don't add to the `gate_order` here because
        // `route_reachable_nodes` is responsible for that part.
        self.out_map
            .insert(self.dag.dag[nodes[0]].py_node_id, swaps);
        for node in nodes {
            self.front_layer.remove(node);
        }
        self.route_reachable_nodes(nodes);
        // Ideally we'd know how to mutate the extended set directly, but since its limited size
        // ties its construction strongly to the iteration order through the front layer, it's not
        // easy to do better than just emptying it and rebuilding.
        self.extended_set.clear();
        self.populate_extended_set();
    }

How can we reproduce the issue?

As saying in comments, it never happen unless we're unlucky. an easy way to reproduce is setting the max_iterations_without_progress smaller to make the program doing force route

What should happen?

even doing force swap, current swaps will be still insert to out_map

Any suggestions?

not using drain to get an iter

jakelishman commented 1 month ago

The current_swaps.drain(..) is deliberately throwing away those swaps without adding them to the out_map; the for_each(|swap| state.apply_swap(swap)) is undoing that set. We deliberately throw those away because those swaps didn't cause any progress, and now there's loads of them. state.force_enable_closest_node then re-populates current_swaps with a set of swaps that definitely makes progress, starting from the last progress point, and then those are the ones that are added to out_map.

Do you have an example system where you can show a broken output?

chcfz commented 1 month ago

The current_swaps.drain(..) is deliberately throwing away those swaps without adding them to the out_map; the for_each(|swap| state.apply_swap(swap)) is undoing that set. We deliberately throw those away because those swaps didn't cause any progress, and now there's loads of them. state.force_enable_closest_node then re-populates current_swaps with a set of swaps that definitely makes progress, starting from the last progress point, and then those are the ones that are added to out_map.

Do you have an example system where you can show a broken output?

Oh,sry. It's my fault, I was misunderstanding why doing state.apply_swap before do force route. I am doing some changes in sabre routing, i fixed this bug(now i know it's not) along with another(the real bug) in my code. Thank u for ur reply and sorry again for my mistake

jakelishman commented 1 month ago

No worries, thanks for the interest!