Correctly handle target with global 1q or 3q gates in Sabre

[mtreinish]

Summary

The recently merged #14317 the sabre passes were updated to build a coupling graph from a target in Rust as part of moving all the passes' logic into rust. There was a small oversight in the construction of the coupling graph when handling targets that contain globally defined operations. Previously, the constructed connectivity graph would treat the presence of any globally defined operation in the target as meaning the target had all to all connectivity which isn't always the case. If the globally defined gate is a 1q operation there could still be a connectivity graph. Similarly in the presence of > 2q gates in the target the python version didn't invalidate the construction of a connectivity graph unless there were no 2q operations. Instead the connectivity graph was ignoring the multiqubit operations. This commit updates the logic to match what is done in the Python space Target.build_coupling_map().

Details and comments

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 16972786837

Details

• 8 of 16 (50.0%) changed or added relevant lines in 3 files are covered.
• 9 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.004%) to 88.262%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/transpiler/src/passes/sabre/layout.rs	0	1	0.0%
crates/transpiler/src/passes/sabre/route.rs	0	1	0.0%
crates/transpiler/src/target/mod.rs	8	14	57.14%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/parameter/parameter_expression.rs	1	83.39%
crates/circuit/src/parameter/symbol_expr.rs	4	74.58%
crates/qasm2/src/lex.rs	4	92.78%

Totals
Change from base Build 16967470304:	0.004%
Covered Lines:	87858
Relevant Lines:	99542

---

💛 - Coveralls

[jakelishman]

Is there a path to testing the multi-q handling, whatever we choose to do with it?

[mtreinish]

Is there a path to testing the multi-q handling, whatever we choose to do with it?

We can very easily add a test that has a toffoli in the target on a circuit that needs swaps. That'll validate whatever we need. I didn't have any tests because I viewed this as restoring the previous behavior and not really changing anything. The multi-q gate in the target case is sufficiently uncommon and poorly supported I wasn't as worried about it as much as the 1q global case which was blocking my testing from C.

[jakelishman]

Thanks - I'm fine with this, but just to check: does the new form, with Sabre still rejecting the MultiQ case, still satisfy everything you were after?

Feel free to add to the queue if so.

[mtreinish]

All I really cared about in this PR was fixing the global gates thing because it's blocking #14711 . I think this is technically a breaking api change though now that you mention it. For example with 2.1.x if you did:

                ┌───┐
q_0: ─■───■───■─┤ X ├
      │   │   │ └─┬─┘
q_1: ─■───■───■───■──
      │ ┌─┴─┐ │   │
q_2: ─■─┤ X ├─■───■──
        └───┘

transpiled to:

Target
Number of qubits: 4
Instructions:
	u
		(0,)
		(1,)
		(2,)
		(3,)
	cx
		(0, 1)
		(1, 0)
		(1, 2)
		(2, 1)
		(2, 3)
		(3, 2)
	ccx

You'd get:

         ┌────────────┐┌───┐┌────────────┐┌───┐┌────────────┐┌───┐┌────────────┐
q_2 -> 0 ┤ U(π/2,0,π) ├┤ X ├┤ U(π/2,0,π) ├┤ X ├┤ U(π/2,0,π) ├┤ X ├┤ U(π/2,0,π) ├──■──
         └────────────┘└─┬─┘└────────────┘└─┬─┘└────────────┘└─┬─┘└────────────┘  │
q_1 -> 2 ────────────────■──────────────────■──────────────────■──────────────────■──
                         │                  │                  │                ┌─┴─┐
q_0 -> 3 ────────────────■──────────────────■──────────────────■────────────────┤ X ├
                                                                                └───┘

but with sabre on main it would error right?

[jakelishman]

Yeah, I suppose in this particular case with concrete cx but global ccx, it could be argued as a breaking change, though really I think there's a fair call that it was just undefined behaviour before. The documentation of Sabre previously stated that all multi-qubit gates already had to be decomposed, and it implicitly treated them as directives. #14605 fixes Sabre to loudly error on multi-q gates (since before they just got falsely treated as directives), though technically if the multi-q is valid everywhere, then the previous version would have returned a valid circuit in this situation.

I think we could merge this (and #14605) as-is now, and later add additional logic to Sabre such that SabreDAG::from_interactions queries the Target to see if a multi-q is globally supported, and formally lift it to a Synchronize node if so? That would the guarantee that Sabre is only returning results for circuits that are actually accurate, which is rather better than the silent nonsense we're doing right now.

[mtreinish]

Yeah that sounds like a fair plan as long as we remember to do the last piece, I'll open an issue from your comment and track it as a bugfix for 2.2.0