Add SabreLayout to the C API by mtreinish · Pull Request #14711 · Qiskit/qiskit

mtreinish · 2025-07-08T22:17:02Z

Summary

This commit adds a standalone transpiler pass function to the C API for
running the SabreLayout transpiler pass. This function returns a
new result type QkSabreLayoutResult that contains the output circuit
along with the layouts from running the pass.

Details and comments

Fixes #14449

TODO:

Expand test coverage

This PR is based on top of #14668 and #14715 and will need to be rebased once those two PRs merge. In the meantime you can review just the contents of this PR by looking at the HEAD commit on the PR: d1aff29 and d088960 This has been rebased now.

qiskit-bot · 2025-07-08T22:17:07Z

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

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

coveralls · 2025-07-08T22:49:47Z

Pull Request Test Coverage Report for Build 17142393122

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

59 of 59 (100.0%) changed or added relevant lines in 1 file are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.009%) to 88.43%

Files with Coverage Reduction	New Missed Lines	%
crates/cext/src/transpiler/passes/elide_permutations.rs	1	95.24%
crates/qasm2/src/lex.rs	2	92.27%
crates/circuit/src/parameter/symbol_expr.rs	6	72.9%

Totals
Change from base Build 17137677112:	0.009%
Covered Lines:	89827
Relevant Lines:	101580

💛 - Coveralls

mtreinish · 2025-07-09T14:24:44Z

I have a test locally that validates inserting swaps, but it hit an issue that was introduced in #14317. I pushed up a fix in: #14715 I'll embed that into this PR but it will add a dependency on #14715 so this PR will depend on both that and #14668.

mtreinish · 2025-07-22T17:10:24Z

I've rebased this PR on main, but it's still depending on: #14715 which is in the branch. I will rebase it again after that PR merges which is needed to enable the tests as they're written here. I could adjust the tests to not rely on global gates, but I figured since it uncovered a bug that I should leave it using a global 1q gate.

mtreinish · 2025-08-14T21:45:53Z

This has been rebased and is unblocked now

This commit adds a standalone transpiler pass function to the C API for running the SabreLayout transpiler pass. This function returns a new result type QkSabreLayoutResult that contains the output circuit along with the layouts from running the pass. Fixes Qiskit#14449

jakelishman

I'll look again after the interface change to QkTranspileLayout. There's a few places in this PR (and maybe others?) where we're doing match result { Ok(x) => x, Err(e) => panic!("{e}") }, which I think is just Result::unwrap, but with Display formatting instead of Debug?

jakelishman · 2025-08-19T18:35:07Z

+/// @ingroup QkSabreLayoutResult
+/// Get the circuit from sabre layout result
+///
+/// @param result a pointer to the result of the pass.
+///
+/// @returns A pointer to the output circuit
+///
+/// # Safety
+///
+/// Behavior is undefined if ``result`` is not a valid, non-null pointer to a
+/// ``QkSabreLayoutResult``. The pointer to the returned circuit is owned by
+/// the result object, it should not be passed to ``qk_circuit_free()`` as it
+/// will be freed by ``qk_sabre_layout_result_free()``.
+#[no_mangle]
+#[cfg(feature = "cbinding")]
+pub unsafe extern "C" fn qk_sabre_layout_result_circuit(
+    result: *mut SabreLayoutResult,
+) -> *mut CircuitData {
+    let result = unsafe { mut_ptr_as_ref(result) };
+    (&mut result.circuit) as *mut _
+}


It's not going to be relevant because of the change in direction of the PR, but were it to stay like this, I would have liked to think more about the ownership / lifetime model being implied by this - it's not really clear to me who'd be responsible for freeing the circuit, and how the freeing of the total layout object would have worked under the assumption that the C caller wanted to "move" ownership of the circuit out of this return object.

jakelishman · 2025-08-19T18:37:09Z

+/// @ingroup QkSabreLayoutResult
+/// Get the number of qubits in the initial layout
+///
+/// @param result a pointer to the result
+///
+/// @returns The number of qubits in the initial layout
+///
+/// # Safety
+///
+/// Behavior is undefined if ``result`` is not a valid, non-null pointer to a
+/// ``QkSabreLayoutResult``.
+#[no_mangle]
+#[cfg(feature = "cbinding")]
+pub unsafe extern "C" fn qk_sabre_layout_result_initial_layout_num_qubits(
+    result: *const SabreLayoutResult,
+) -> u32 {
+    let result = unsafe { const_ptr_as_ref(result) };
+    result.initial_layout.num_qubits() as u32
+}


Again, this will be invalidated by the pivot in return type, but I think this would have always returned the same as qk_sabre_layout_result_final_layout_num_qubits, so we probably could have just had one?

jakelishman · 2025-08-19T18:38:40Z

+/// This function is multithreaded and will launch a thread pool with threads equal to the number
+/// of CPUs by default. You can tune the number of threads with the ``RAYON_NUM_THREADS``
+/// environment variable. For example, setting ``RAYON_NUM_THREADS=4`` would limit the thread pool
+/// to 4 threads.


In the future, we're almost certainly going to want to expose some sort of per-function and per-process "threading configuration" objects into C space, given the target audience.

Yeah, I am putting these comments in the docstrings because I've already had discussions about calling some of the C API functions in an OpenMP context and wanted to make sure we were at least documenting how it worked now. But I agree we'll probably want to expose a structure maybe as part of TranspileOptions to control the threading behavior.

mtreinish · 2025-08-19T20:06:30Z

I'll look again after the interface change to QkTranspileLayout. There's a few places in this PR (and maybe others?) where we're doing match result { Ok(x) => x, Err(e) => panic!("{e}") }, which I think is just Result::unwrap, but with Display formatting instead of Debug?

Yeah that's fair, I was thinking there was a header in the message with something about unwrap() but I was misremembering or confusing it with something else. Regardless I probably need to change this to have an explicit error string since we'll get a panic inside the panic handling if it's a PyErr.

jakelishman · 2025-08-19T23:48:43Z

+pub unsafe extern "C" fn qk_transpiler_pass_standalone_sabre_layout(
+    circuit: *mut CircuitData,
+    target: *const Target,
+    max_iterations: usize,
+    num_swap_trials: usize,
+    num_random_trials: usize,
+    seed: i64,
+) -> *mut TranspileLayout {


Returning a TranspileLayout like this discounts the possibility that the user might already have an "output permutation" they care about tracking (c.f. Split2qUnitaries or ElidePermutations). I'm fine if we decide for now that "standalone means it stands entirely alone" and so there's no other context (before TranspileLayout moves into CircuitData/DAGCircuit properly), I'm mostly just flagging this decision for the record.

We didn't expose the API for permutation composition to C. We could add it I guess, but I think for right now the intent is for the standalone functions to be standalone. That's the position we took for the other C standalone pass functions too. We'll need to think about the larger workflows for building custom pass managers in >=2.3.

jakelishman · 2025-08-20T00:16:53Z

+    bool compare_result = compare_circuits(qc, expected_circuit);
+    if (!compare_result) {
+        result = EqualityError;
+        goto layout_cleanup;
+    }


Mostly unimportant, but is this the standard C return-value convention for a comparator? Things like strcmp and process return codes return 0 on match/success, but we're returning 0 (false) for the failure case.

I can invert the logic, I didn't really think about it too much when I wrote the helper function.

jakelishman

Minor stuff

jakelishman · 2025-08-21T22:02:02Z

+#[repr(C)]
+pub struct QkSabreLayoutOptions {
+    /// The number of forward-backward iterations in the sabre routing algorithm
+    max_iterations: usize,
+    /// The number of trials to run of the sabre routing algorithm for each iteration. When > 1 the
+    /// trial that routing trial that results in the output with the fewest swap gates will be
+    /// selected.
+    num_swap_trials: usize,
+    /// The number of random layout trials to run. The trial that results in the output with the
+    /// fewest swap gates will be selected.
+    num_random_trials: usize,
+    /// A seed value for the pRNG used internally.
+    seed: u64,
+}


In VF2 land I went for making the internals of the struct private to let us extend it in an API-safe way, but I think it's good to try both ways and see which feels better, while we're still uncommitted to a stable interface - let's leave this as-is.

Co-authored-by: Jake Lishman <jake@binhbar.com>

jakelishman

Thanks!

* Add SabreLayout to the C API This commit adds a standalone transpiler pass function to the C API for running the SabreLayout transpiler pass. This function returns a new result type QkSabreLayoutResult that contains the output circuit along with the layouts from running the pass. Fixes Qiskit#14449 * Improve testing * Fix tests * Fix docs * Finish API docs for standalone function * Pivot interface to use TranspileLayout This commit reworks the C interface for sabre layout to use TranspileLayout for the return type remove the Sabre result type. It was largely duplicative and it's better to just use the same interface. * Remove stale references to QkSabreLayoutResult in docs * Fix test memory leaks * Make input to pass function an options struct * Set number of input qubits correctly in output layout * Move helper functions to common.c * Use a stack variable instructions in compare_circuits() * Fix output_permutation in the return * Apply suggestions from code review Co-authored-by: Jake Lishman <jake@binhbar.com> * Update docstring for circuit arg * Don't use PyErr debug or display * Add missing newlines to test failure strings * Update error messages about output permutation test failures * Correct permutation variable names * Correct copyright header --------- Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

mtreinish added this to the 2.2.0 milestone Jul 8, 2025

mtreinish requested a review from a team as a code owner July 8, 2025 22:17

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 8, 2025

mtreinish force-pushed the sabre_at_c branch from 7f122d7 to d1aff29 Compare July 9, 2025 10:46

raynelfss assigned jakelishman Jul 22, 2025

mtreinish force-pushed the sabre_at_c branch from d088960 to af2507f Compare July 22, 2025 17:08

mtreinish mentioned this pull request Aug 14, 2025

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

Merged

mtreinish force-pushed the sabre_at_c branch from af2507f to 9435002 Compare August 14, 2025 21:45

mtreinish removed the on hold Can not fix yet label Aug 14, 2025

mtreinish added 2 commits August 14, 2025 17:45

Improve testing

9435002

mtreinish mentioned this pull request Aug 17, 2025

Transpile C API #14760

Merged

13 tasks

mtreinish added 5 commits August 18, 2025 14:54

Merge remote-tracking branch 'origin/main' into sabre_at_c

28f3737

Fix tests

94b80bc

Fix docs

ac2792a

Merge branch 'main' into sabre_at_c

b44e527

Finish API docs for standalone function

b26658f

mtreinish commented Aug 19, 2025

View reviewed changes

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

mtreinish added the on hold Can not fix yet label Aug 19, 2025

jakelishman reviewed Aug 19, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into sabre_at_c

127d6d7

Remove stale references to QkSabreLayoutResult in docs

05725ba

jakelishman reviewed Aug 20, 2025

View reviewed changes

Fix test memory leaks

6d95b87

jakelishman removed the on hold Can not fix yet label Aug 20, 2025

mtreinish added 6 commits August 20, 2025 08:18

Make input to pass function an options struct

dcc672b

Set number of input qubits correctly in output layout

be8e750

Move helper functions to common.c

6bf9a6e

Use a stack variable instructions in compare_circuits()

87e98f1

Fix output_permutation in the return

cb8960d

Merge branch 'main' into sabre_at_c

6f5ffe5

mtreinish requested a review from jakelishman August 20, 2025 21:47

Merge branch 'main' into sabre_at_c

58a6e45

jakelishman reviewed Aug 21, 2025

View reviewed changes

mtreinish and others added 5 commits August 21, 2025 19:23

Apply suggestions from code review

8d54e2c

Co-authored-by: Jake Lishman <jake@binhbar.com>

Update docstring for circuit arg

6209b8d

Don't use PyErr debug or display

59d316d

Add missing newlines to test failure strings

4ef73e4

Update error messages about output permutation test failures

fd4e06d

mtreinish requested a review from jakelishman August 21, 2025 23:42

Correct permutation variable names

df6b2e4

jakelishman previously approved these changes Aug 21, 2025

View reviewed changes

jakelishman enabled auto-merge August 21, 2025 23:47

Correct copyright header

5775678

jakelishman dismissed their stale review via 5775678 August 22, 2025 00:17

jakelishman approved these changes Aug 22, 2025

View reviewed changes

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

Merged via the queue into Qiskit:main with commit 52ebd73 Aug 22, 2025
26 checks passed

Uh oh!

Conversation

mtreinish commented Jul 8, 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 8, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17142393122

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

mtreinish commented Jul 9, 2025

Uh oh!

mtreinish commented Jul 22, 2025

Uh oh!

mtreinish commented Aug 14, 2025

Uh oh!

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish commented Aug 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

mtreinish commented Jul 8, 2025 •

edited

Loading

coveralls commented Jul 8, 2025 •

edited

Loading