Optimize shortestpath in SABRE

[csookim]

This PR is an addition to #1398.

Since deepcopy() makes the compiler slower, it needs to be changed.

code

Line 901 takes over 30% of the routing time.

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[csookim]

Previous approach

• Save the circuit after the gate is routed.

self._temporary_added_swaps: int
self._saved_circuit: CircuitMap

qibo/src/qibo/transpiler/router.py

Lines 900 to 901 in 9db9b66

	self._temporary_added_swaps = 0
	self._saved_circuit = deepcopy(self.circuit)

• Revert to the saved circuit when the number of SWAPs exceeds the threshold.

qibo/src/qibo/transpiler/router.py

Lines 689 to 693 in 9db9b66

	if (
	self._temporary_added_swaps > self.swap_threshold * longest_path
	): # threshold is arbitrary
	self.circuit = deepcopy(self._saved_circuit)
	self._shortest_path_routing()

Updated approach

• Save the sequence of SWAPs.

self._temp_added_swaps: list

qibo/src/qibo/transpiler/router.py

Lines 817 to 818 in a7a1c62

	self.circuit.update(best_candidate)
	self._temp_added_swaps.append(best_candidate)

• Undo all of the temporary SWAPs when the number of SWAPs exceeds the threshold.

qibo/src/qibo/transpiler/router.py

Lines 701 to 708 in a7a1c62

	if (
	len(self._temp_added_swaps) > self.swap_threshold * longest_path
	): # threshold is arbitrary
	while self._temp_added_swaps:
	swap = self._temp_added_swaps.pop()
	self.circuit.update(swap, undo=True)
	self._temp_added_swaps = []
	self._shortest_path_routing()

Results

• Average runtime of routing 100 random CZ circuits (20 qubits, 50 gates)
• Trivial Placer, SABRE router, Line connectivity

Before update: 0.558s
After update: 0.253s

Discussion

• The undo functionality is currently implemented in CircuitMap.update(). Would it be better to create a new function undo() for this purpose?
• TranspilerPipelineError is raised when the gate to be undone doesn't match the last added SWAP gate. Is there a more suitable error type for this situation?

[Simone-Bordoni]

Thank you for this work, the code looks nice.
Regarding the two points you asked in the discussion:

• For the undo I think it would be better to have a separate function.
• Regarding the error type I think it would be better to raise ConnectivityError. In my Idea Connectivity error is raised every time there is some problem during routing while TranspilerPipelineError regards an error occurring in defining the pipeline for example if you forget to define the Placer.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (3d87ba7) to head (96a0bd8).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1408   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          78       78           
  Lines       11225    11237   +12     
=======================================
+ Hits        11219    11231   +12     
  Misses          6        6           

Flag	Coverage Δ
unittests	99.94% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[csookim]

@Simone-Bordoni , I have a question about the def test_circuit_map() function.

qibo/tests/test_transpiler_router.py

Line 264 in a2f7424

def test_circuit_map():

The function includes the following code:

...
initial_layout = {"q0": 2, "q1": 0, "q2": 1, "q3": 3}
...
circuit_map.update((0, 2)) # first swap
...
circuit_map.update((1, 2)) # second swap
assert routed_circuit.queue[5].qubits == (2, 0)    # routed_circuit.queue[5] is the second swap

After the first swap, the layout should be converted to:

{"q0": 0, "q1": 2, "q2": 1, "q3": 3}

since update() operates on logical qubits. This would be equivalent to swapping physical qubits q0 and q1.

After the second swap, the swap should be on logical qubits (1, 2). So, the layout will be changed to:

{"q0": 0, "q1": 1, "q2": 2, "q3": 3}

Does this mean that the qubits affected by the swap are q2 and q1?

assert routed_circuit.queue[5].qubits == (2, 1)

Since routed_circuit.queue[i].qubits contains physical qubits, I think the qubits should be (2, 1). Could you help me understand if there's something I’m misunderstanding here?

Thank you for your time.

[BrunoLiegiBastonLiegi]

This looks ok code-wise, the only thing that seems a bit weird to me is that the undo requires for you to pass the swap you wish to undo. I mean, since in principle you undo the last one, you wouldn't need to pass explicitely the swap, but just look for the last one added in a routed block, right? And, indeed, here swap is only used to check that the last swap you find is the one you expected.

[csookim]

I used swap to double-check if the undo function works correctly. I'll remove the checking step.

Also, could you take a look at the question I posted above?

[BrunoLiegiBastonLiegi]

Sorry for the delay. Double checking could always be useful, indeed, but, if you want to keep it, I wouldn't make it mandatory at least. In the sense that you could have the swap=None argument as optional. In any case, this check should be covered in the tests in principle.
Regarding the question above, instead, I had a look at it last week. Now, the best person to answer this is probably @Simone-Bordoni, but I believe he is currently on a leave. Thus, I will provide you with my guess, which however may be incorrect. By looking at the update function

qibo/src/qibo/transpiler/router.py

Line 241 in a2f7424

def update(self, swap: tuple):

it seems that you update the circuit_logical mapping, not the physical one (physical_logical), meaning that in the layout dict, you do not swap the values, but rather the keys.
Therefore you start from:

{"q0": 0, "q1": 2, "q2": 1, "q3": 3}

after the (0, 2) swap you get:

{"q2": 0, "q1": 2, "q0": 1, "q3": 3}

and after (1, 2):

{"q1": 0, "q2": 2, "q0": 1, "q3": 3}

This means that you end up with circuit_logical = [1, 2, 0, 3], which is indeed checked in the test:

assert circuit_map._circuit_logical == [1, 2, 0, 3]

I hope this helped, assuming it is correct. In general, though, this small review of the transpiler code that I did to find the answer, made me think that probably, both, the naming scheme could be improved to make it clearer, and some further documentation should be added to the docstrings. Otherwise, everytime we go back to the code, we risk to waste time in re-understanding everything.

[csookim]

Thanks for your response. I noticed that while optimizing SABRE and a better naming scheme seems to be needed. I also found and implemented a more efficient way to manage the mapping. I think it would be best to wait until @Simone-Bordoni returns to discuss the new mapping scheme.

[Simone-Bordoni]

I am currently on leaves. If you want to implement a new mapping scheme you can do the PR and I will review it when I will be back.