Skip to content

RFC3: Move Delay value storage to a typed arena#16242

Draft
ihincks wants to merge 4 commits into
Qiskit:mainfrom
ihincks:c-dt-delay-3
Draft

RFC3: Move Delay value storage to a typed arena#16242
ihincks wants to merge 4 commits into
Qiskit:mainfrom
ihincks:c-dt-delay-3

Conversation

@ihincks
Copy link
Copy Markdown
Contributor

@ihincks ihincks commented May 22, 2026

This PR is one of three RFCs solving the same problem: dt-unit delays panic in qk_circuit_get_instruction because their integer duration is stored as Param::Obj(PyAny).

This branch takes the most surgical fix. Delay durations leave the Param list entirely and move into a typed global arena (crates/circuit/src/delay_arena.rs). The arena holds a DelayDuration enum (Int, Float, Expr, PyObj) inside refcounted Slots, and StandardInstruction::Delay(DelayHandle) replaces StandardInstruction::Delay(DelayUnit). Standard gates and other instructions are untouched — only the Delay variant knows about durations now.

The C API gets qk_circuit_delay_dt(qubit, i64) and qk_circuit_get_delay(idx, *unit, *kind, *vi, *vf) with a CDelayDurationKind enum that distinguishes integer, float, and expression durations. QPY format is unchanged — the writer synthesizes a duration Param from the handle when it serializes a Delay, and the reader builds a fresh handle from param[0] on read.

The trade-off is implementation complexity. StandardInstruction loses Copy and gets a manual PartialEq. PackedOperation::clone and drop special-case the Delay case to bump and release the refcount. The Box<Slot> indirection in the arena is load-bearing — with() snapshots a *const Slot then drops the read guard. There's a new ParameterUse::DelayDuration variant for symbolic durations that the parameter table needs to track separately.

The other two RFCs are #16226 (a minimal int → float coercion patch) and #16241 (a uniform Param::Int variant that touches every match over Param).

Partially addresses #16121.

AI/LLM disclosure

  • I didn't use LLM tooling, or only used it privately.
  • I used the following tool to help write this PR description:
  • I used the following tool to generate or modify code:

@ihincks ihincks changed the title Move Delay value storage to an arena RFC3: Move Delay value storage to a typed arena May 22, 2026
ihincks and others added 3 commits May 22, 2026 10:29
The previous commit's diff modified `lib.rs` to reference a new
`delay_arena` module but the file itself was never staged, breaking every
CI job that builds the workspace.  Also brings `test/c/test_circuit.c` in
line with the project's `clang-format` configuration.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
`qk_circuit_delay_dt` and `qk_circuit_get_delay` are exported from the C
header but were missing from `cext-vtable`'s export list, which the
binding-vs-slot lint flagged.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
`CircuitInstruction.params` (the Python property) was returning an empty
list for delays after the arena refactor moved the duration off the Rust
params list. The Python data model still expects a one-element list, so
materialize it on demand from the `DelayHandle` here — same "lie at the
boundary" pattern already used by `StandardInstruction::create_py_op`.

Caught by `test_can_transpile_circuits_with_assigning_parameters_inbetween`,
which reads `circ.data[1].params[0]` directly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 26299637026

Coverage decreased (-0.03%) to 87.461%

Details

  • Coverage decreased (-0.03%) from the base build.
  • Patch coverage: 148 uncovered changes across 13 files (453 of 601 lines covered, 75.37%).
  • 17 coverage regressions across 7 files.

Uncovered Changes

Top 10 Files by Coverage Impact Changed Covered %
crates/circuit/src/circuit_data.rs 78 46 58.97%
crates/transpiler/src/passes/instruction_duration_check.rs 19 1 5.26%
crates/qasm3/src/exporter.rs 22 5 22.73%
crates/cext/src/circuit.rs 79 63 79.75%
crates/qpy/src/circuit_reader.rs 42 29 69.05%
crates/circuit/src/circuit_instruction.rs 45 33 73.33%
crates/transpiler/src/passes/constrained_reschedule.rs 11 0 0.0%
crates/circuit/src/delay_arena.rs 190 180 94.74%
crates/accelerate/src/circuit_duration.rs 21 13 61.9%
crates/circuit/src/circuit_drawer.rs 18 12 66.67%

Coverage Regressions

17 previously-covered lines in 7 files lost coverage.

File Lines Losing Coverage Coverage
crates/qasm2/src/lex.rs 6 91.77%
crates/circuit/src/operations.rs 4 82.21%
crates/accelerate/src/circuit_duration.rs 3 72.88%
crates/circuit/src/circuit_data.rs 1 86.06%
crates/circuit/src/parameter/parameter_expression.rs 1 91.04%
crates/circuit/src/parameter/symbol_expr.rs 1 73.88%
crates/qasm3/src/exporter.rs 1 79.18%

Coverage Stats

Coverage Status
Relevant Lines: 124345
Covered Lines: 108753
Line Coverage: 87.46%
Coverage Strength: 963161.1 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants