Add Split2qUnitaries to C API by mtreinish · Pull Request #14720 · Qiskit/qiskit

mtreinish · 2025-07-09T21:43:49Z

Summary

This commit adds a standalone transpiler pass function to the C API for
running the Split2qUnitaries transpiler pass. This pass mutates the dag
in place if there is no swap splitting that occurs (either by user
option or because there are no applicable gates) or returns a new dag
with a permutation. This result is encapsulated in a
Split2qUnitariesResult struct that contains the circuit and the
permutation.

One small change was needed to the dag circuit and the rust code of the
pass to enable running it without python. A py token was used for
updating the instruction cache when splitting instructions. This was
mostly a leftover from when unitary gate was a PyGate where this
mattered, but now there isn't anything to prepopulate. But also it
wasn't ever necessary in the case of a python defined operation it would
do the ref count update implicitly and populate the cache on the first
access. This commit removes the python usage for this which simplifies
the code and also enables running this pass from standalone C.

Details and comments

Fixes #14448

TODO:

Expand test coverage

~~This PR is based on top of #14668 will need to be rebased once #14668 merges. In the meantime you can review just the contents of this PR by looking at the HEAD commit on the PR: 0e582b7~~ This has been rebased now

qiskit-bot · 2025-07-09T21:43:54Z

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

@Eric-Arellano
@abbycross
@beckykd
@Qiskit/terra-core

coveralls · 2025-07-09T22:18:54Z

Pull Request Test Coverage Report for Build 17153328607

Details

32 of 35 (91.43%) changed or added relevant lines in 3 files are covered.
23 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.006%) to 88.429%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/cext/src/transpiler/passes/split_2q_unitaries.rs	27	30	90.0%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	5	92.01%
crates/qasm2/src/parse.rs	18	96.62%

Totals
Change from base Build 17143354507:	0.006%
Covered Lines:	89843
Relevant Lines:	101599

💛 - Coveralls

This commit adds a standalone transpiler pass function to the C API for running the Split2qUnitaries transpiler pass. This pass mutates the dag in place if there is no swap splitting that occurs (either by user option or because there are no applicable gates) or returns a new dag with a permutation. This result is encapsulated in a Split2qUnitariesResult struct that contains the circuit and the permutation. One small change was needed to the dag circuit and the rust code of the pass to enable running it without python. A py token was used for updating the instruction cache when splitting instructions. This was mostly a leftover from when unitary gate was a PyGate where this mattered, but now there isn't anything to prepopulate. But also it wasn't ever necessary in the case of a python defined operation it would do the ref count update implicitly and populate the cache on the first access. This commit removes the python usage for this which simplifies the code and also enables running this pass from standalone C. Fixes Qiskit#14448

mtreinish · 2025-07-30T19:59:20Z

This is ready for review now, I've rebased it on main now that the pre-reqs have merged and added the missing test coverage.

jakelishman

I stopped looking through the C tests at the point of the last comment, but I suspect it'd just be the same comments in the last test too.

jakelishman · 2025-08-19T19:00:26Z

+/// The result from ``qk_transpiler_pass_standalone_split_2q_unitaries()``.
+pub struct Split2qUnitariesResult {
+    circuit: CircuitData,
+    permutation: Vec<u32>,
+}


This will interact awkwardly with the new push to having TranspileLayout always store the output permutation. Perhaps we want to do a similar thing with this pass as we're doing for SabreLayout, but it's starting to get a bit awkward about whether we want to expose a stand-alone way of calling the pass if we can't either a) pass a TranspileLayout from pass to pass or b) provide methods on TranspileLayout in C to update the tracking data based on new changes.

I feel like our options are going to be:

don't bother with standalone functions like this one until we have a better answer for exactly what we'll do

claim "standalone passes can't be composed because you don't get the full pipeline tracking" so the current API is fine

extend the C API such that you could reasonably use the methods sequentially to emulate the Qiskit pipeline, just with loads of extra dag_to_circuit(do_pass(circuit_to_dag(qc))) everywhere

Personally I'd tend towards either the first or second options - I don't think the standalone passes are necessarily all super high priority. I think they're useful as a research direction, and for incrementally moving us towards full exposure to C, so that's an argument in favour of point 2 over point 1.

I think we've discussed this in several other PRs, and settled on option 2. We'll want to figure out how to do this as part of exposing composable transpilation pipelines in C.

jakelishman · 2025-08-19T19:10:43Z

+    let result = unsafe { const_ptr_as_ref(result) };
+    result.permutation.as_ptr()


Do we actually have to produce the &Split2qUnitariesResult here? I vaguely thought we could just unsafe { result.permutation }.as_ptr(), with all the undefined behaviour that goes with that.

I'm totally cool if we deliberately want explicit panics on bad accesses, though.

This was changed in: ae02b37 it returns a null pointer if there is no permutation and a pointer to a transpile layout if there is one.

jakelishman · 2025-08-19T19:15:03Z

+///             qk_circuit_gate(qc, QkGate_CX, qargs, NULL);
+///         }
+///     }
+///     QkSplit2qUnitariesResult *result = qk_transpiler_pass_standalone_split_2q_unitaries(qc, 1.0 - 1e-16, true)


It's probably neither here nor there, but the closest double-precision floating-point value to 1.0 - 1e-16 is actually more like 1.11e-16 - saying 1e-16 is actually kind of wrong by over 10%. Are we actually able to hit a precision that's just 1ULP?

jakelishman · 2025-08-19T19:19:01Z

-            #[cfg(feature = "cache_pygates")]
-            let py_op = match new_op.view() {
-                OperationRef::StandardGate(_)
-                | OperationRef::StandardInstruction(_)
-                | OperationRef::Unitary(_) => OnceLock::new(),
-                OperationRef::Gate(gate) => OnceLock::from(gate.gate.clone_ref(py)),
-                OperationRef::Instruction(instruction) => {
-                    OnceLock::from(instruction.instruction.clone_ref(py))
-                }
-                OperationRef::Operation(op) => OnceLock::from(op.operation.clone_ref(py)),
-            };
            let inst = PackedInstruction {
                op: new_op,
                qubits: self.qargs_interner.insert_owned(qubits),
                clbits: self.cargs_interner.get_default(),
                params: (!params.is_empty()).then(|| Box::new(params)),
                label: None,
                #[cfg(feature = "cache_pygates")]
-                py_op,
+                py_op: OnceLock::new(),


This might have been better as a standalone patch, but let's not bother now.

Heh, this I think is the reason this is needed for #14760

raynelfss

This is looking good, I just have a couple of comments.

raynelfss

This looks about ready, just a couple small comments.

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

jakelishman

Other than minor stuff, this is fine by me.

… exist

…pected

jakelishman

Thanks for the changes - I think this is fine now (though tbh, I've looked at it enough times that my eyes are kind of glossing over). I'll let Ray press the merge button if he's happy too.

* Add Split2qUnitaries to C API This commit adds a standalone transpiler pass function to the C API for running the Split2qUnitaries transpiler pass. This pass mutates the dag in place if there is no swap splitting that occurs (either by user option or because there are no applicable gates) or returns a new dag with a permutation. This result is encapsulated in a Split2qUnitariesResult struct that contains the circuit and the permutation. One small change was needed to the dag circuit and the rust code of the pass to enable running it without python. A py token was used for updating the instruction cache when splitting instructions. This was mostly a leftover from when unitary gate was a PyGate where this mattered, but now there isn't anything to prepopulate. But also it wasn't ever necessary in the case of a python defined operation it would do the ref count update implicitly and populate the cache on the first access. This commit removes the python usage for this which simplifies the code and also enables running this pass from standalone C. Fixes Qiskit#14448 * Pivot to returning TranspileLayout instead of custom return type * Fix memory leak in test * Add missing newlines from error strings * Apply suggestions from code review Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * Fix docstring example * Fix error message on internal pass failure * Add missing QkTranspileLayout free in failure mode where it shouldn't exist * Fix failure message in test where permutation doesn't exist and is expected --------- Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

mtreinish added this to the 2.2.0 milestone Jul 9, 2025

mtreinish requested a review from a team as a code owner July 9, 2025 21:43

mtreinish added on hold Can not fix yet Changelog: Added Add an "Added" entry in the GitHub Release changelog. mod: transpiler Issues and PRs related to Transpiler C API Related to the C API labels Jul 9, 2025

raynelfss self-assigned this Jul 22, 2025

mtreinish force-pushed the split-c-2q-unitary branch from 0e582b7 to d1f83e7 Compare July 30, 2025 19:58

mtreinish removed the on hold Can not fix yet label Jul 30, 2025

mtreinish changed the title ~~[WIP] Add Split2qUnitaries to C API~~ Add Split2qUnitaries to C API Jul 30, 2025

Merge remote-tracking branch 'origin/main' into split-c-2q-unitary

c204ba1

mtreinish mentioned this pull request Aug 17, 2025

Transpile C API #14760

Merged

13 tasks

Merge remote-tracking branch 'origin/main' into split-c-2q-unitary

d7fabd4

jakelishman reviewed Aug 19, 2025

View reviewed changes

raynelfss reviewed Aug 19, 2025

View reviewed changes

mtreinish added 3 commits August 20, 2025 18:05

Merge remote-tracking branch 'origin/main' into split-c-2q-unitary

62a1b15

Pivot to returning TranspileLayout instead of custom return type

ae02b37

Fix memory leak in test

cf52658

mtreinish requested review from jakelishman and raynelfss August 20, 2025 23:04

Add missing newlines from error strings

54695d0

raynelfss reviewed Aug 21, 2025

View reviewed changes

Comment thread test/c/test_split_2q_unitaries.c Outdated

Comment thread test/c/test_split_2q_unitaries.c Outdated

Comment thread test/c/test_split_2q_unitaries.c Outdated

Apply suggestions from code review

72f8d63

Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

mtreinish requested a review from raynelfss August 21, 2025 20:41

jakelishman reviewed Aug 21, 2025

View reviewed changes

Comment thread crates/cext/src/transpiler/passes/mod.rs

Comment thread crates/cext/src/transpiler/passes/split_2q_unitaries.rs Outdated

Comment thread test/c/test_split_2q_unitaries.c

Comment thread test/c/test_split_2q_unitaries.c

Merge branch 'main' into split-c-2q-unitary

550ebe5

mtreinish added 3 commits August 22, 2025 06:49

Fix docstring example

86bfbe3

Fix error message on internal pass failure

6ecbc63

Add missing QkTranspileLayout free in failure mode where it shouldn't…

18f8361

… exist

mtreinish requested a review from jakelishman August 22, 2025 10:52

Fix failure message in test where permutation doesn't exist and is ex…

d44560a

…pected

jakelishman approved these changes Aug 22, 2025

View reviewed changes

raynelfss approved these changes Aug 22, 2025

View reviewed changes

raynelfss added this pull request to the merge queue Aug 22, 2025

Merged via the queue into Qiskit:main with commit 72cb5a2 Aug 22, 2025
25 checks passed

		let result = unsafe { const_ptr_as_ref(result) };
		result.permutation.as_ptr()

Uh oh!

Conversation

mtreinish commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Jul 9, 2025

Uh oh!

coveralls commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17153328607

Details

💛 - Coveralls

Uh oh!

mtreinish commented Jul 30, 2025

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jakelishman Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

jakelishman Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raynelfss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raynelfss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mtreinish commented Jul 9, 2025 •

edited

Loading

coveralls commented Jul 9, 2025 •

edited

Loading