Repository navigation
Conversation
…ting a gate cx a, b over two registers unrolled to cx a[0], a[1]; cx b[0], b[1] because every operand was flattened into one list and chunked by the gate's arity. When a call has one operand per gate qubit, register operands now pair by index and single qubits repeat, as the spec defines. Slices, aliases, subroutine register arguments and ctrl @ take the same path. Calls with fewer operands than gate qubits, such as cx q2, keep filling operands from the register as before. Closes #384
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR. Running Argus review... Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
![argus-eye[bot]](https://avatars.githubusercontent.com/in/2969822?s=60&v=4){kind=small}
There was a problem hiding this comment.
Argus · 9/10 — Empty broadcast operands raise IndexError
Check 1 warning before you merge. Argus reviewed 3 files.
- 🟡 Warning · Empty broadcast operands raise IndexError ·
src/pyqasm/visitor.py:L1654
What this PR does · 9 author checks
- Fix gate broadcasting so register operands pair element by element and single-qubit operands repeat, rather than silently unrolling to a different circuit.
- Excludes: Rejecting the existing register-fills-several-operands extension or the four-single-qubit cx calling form.
- Excludes: Changing existing behavior for opaque gates or non-constant modifier arguments.
Author's checks
- _visit_generic_gate_operation broadcasts first through _broadcast_operands, resolving operands separately and preserving operand boundaries.
- Inside subroutines, operand resolution preserves formal names so existing argument mapping still applies.
- Calls with one operand per gate qubit, including controls, pair register operands by index and repeat single-qubit operands.
- Registers of different lengths raise ValidationError; for cx with lengths 2 and 3, report Cannot broadcast 'cx' over registers of different lengths [2, 3].
- Each broadcast application uses the normal path, retaining support for modifiers, custom gates, external gates and subroutines.
- Calls with fewer operands than gate qubits and the cx q[0], q[1], q[2], q[3] form retain existing behavior.
- Opaque gates and non-constant modifier arguments retain existing behavior.
- tests/qasm3/test_broadcast.py covers registers, slices, aliases, subroutine arguments, swap, ctrl @, mixed operands, g4 and length mismatch.
- The spec's mixed-operand g4 example emits three applications, pairing qr1 and qr3 by index while repeating qr0[0] and qr2[0].
usage: 282.3k tokens · 17 stages
Contract: production/full · checked: bug_hunter, security, architecture, regression, single-pass review, ponytail-review · review took 2m44s
| Stage | Model | Tokens | Cost |
|---|---|---|---|
| Intent | openai/gpt-6.1-sol |
6.5k | $0.0135 |
| Triage | openai/gpt-6.1-sol |
6.4k | $0.0070 |
| Lead agent | openai/gpt-6.1-sol |
1.3k | $0.0067 |
| Review specialists (10) | openai/gpt-6.1-sol |
237.3k | $0.5146 |
| Review | openai/gpt-6.1-sol |
14.2k | $0.0295 |
| Acceptance | openai/gpt-6.1-sol |
984 | $0.0026 |
| Simulation | openai/gpt-6.1-sol |
12.6k | $0.0363 |
| Scoring | openai/gpt-6.1-sol |
3.0k | $0.0038 |
Dashboard → · React 👎 on an inline comment to dismiss it · Reply to an inline comment or use @argus-eye help
🧠 Learned: 1 pattern · 1 PR summary · 1 finding memory · 1 file memory · …
Cross-Repo PR Coverage
No conflict found with qBraid/qBraid#1468.
Updated at 10:26 UTC on 2026-10-24
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Changes requested — The fix is correct: register operands now pair by index, and the output matches Qiskit's importer. Every multi-qubit gate now pays about 33% more unroll time, even when it has no register operand.
Findings
All findings are posted inline on the relevant lines. Checklist:
- M1 — Broadcast check resolves every operand of every multi-qubit gate; unroll is ~33% slower
- L1 — A length-1 register beside a longer register is accepted instead of raising
- L2 — Opaque branch in
_gate_qubit_countis unreachable; the docstring describes it wrongly - L3 — No test for OpenQASM 2 registers or for duplicate-qubit broadcast
How it was tested
- Full suite on head: 1024 passed, 3 skipped. On main (a742796): 1015 passed, 3 skipped. 9 of the 10 new tests fail on main.
- black, isort, pylint (10.00) and mypy are clean on head.
- 28 edge-case programs ran on head and on main: registers, slices, sets, aliases, mixed register and qubit,
ccx, custom and nested custom gates,pow/inv/ctrl/ctrl(2)/negctrl/ctrl(n)with a constn, loop indices, nested subroutines, mismatched lengths, duplicates, physical qubits, and the legacycx q2;form. - Unitaries compared with
qiskit.qasm3.loads(Operator.equiv): all 13 programs that Qiskit can load match, including the spec'sg4example. - OpenQASM 2
cx q, r;and opaque gates on registers paired wrongly on main. Both pair correctly now. - Benchmark, 30k
cx q[i], q[j]inunroll(), best of 3: 1.11 s on head against 0.83 s on main. - All four suggestion blocks applied together: same suite results plus the 2 new tests, and the benchmark drops to 0.86 s.
Next steps
Address M1 before merge. L1–L3 are optional.
| or self._scope_manager.in_gate_scope() | ||
| or len(operation.qubits) != self._gate_qubit_count(operation) |
There was a problem hiding this comment.
[M1] Broadcast check resolves every multi-qubit gate — Implementation · Medium
Rationale: _broadcast_operands runs for every gate with two or more operands. It calls _gate_qubit_count (a second map_qasm_op_to_callable). Then it runs _get_op_bits 1 + n times before it can tell that cx q[0], q[1] has nothing to broadcast. On 30k cx q[i], q[j], unroll() takes 1.11 s against 0.83 s on main, and validate() slows by about the same amount. Only a few operand forms can name several qubits: a bare identifier that is not $n, a set index, or a range index. One indexed expression always names one qubit. A syntactic check placed before the count check skips the resolution in the common case. With the change below, the suite, the edge-case programs and the Qiskit unitary comparisons give the same results, and the benchmark drops to 0.86 s. Declared single qubits (qubit x; cx x, y;) still take the slow path, because telling them apart from registers needs a scope lookup.
Change requested:
| or self._scope_manager.in_gate_scope() | |
| or len(operation.qubits) != self._gate_qubit_count(operation) | |
| or self._scope_manager.in_gate_scope() | |
| or not any( | |
| ( | |
| isinstance(operand, qasm3_ast.Identifier) | |
| and not operand.name.startswith(PHYSICAL_QUBIT_PREFIX) | |
| ) | |
| or ( | |
| isinstance(operand, qasm3_ast.IndexedIdentifier) | |
| and ( | |
| isinstance(operand.indices[0], qasm3_ast.DiscreteSet) | |
| or isinstance(operand.indices[0][0], qasm3_ast.RangeDefinition) | |
| ) | |
| ) | |
| for operand in operation.qubits | |
| ) | |
| or len(operation.qubits) != self._gate_qubit_count(operation) |
There was a problem hiding this comment.
Applied in a6defcf, with your syntactic check moved into a small _is_register_operand helper, which L1 reuses. Every operand form is checked before _gate_qubit_count and before anything is resolved. A call whose operands are all single indexed qubits or physical qubits returns straight away.
Same benchmark, 30k cx q[i], q[j] in unroll(), best of 5 on this machine:
| Version | Time |
|---|---|
main (dc1b320) |
1.37s |
| previous head (840aed0) | 1.86s |
| now | 1.42s |
| resolved.append( | ||
| self._get_op_bits(single, qubits=True, function_qubit_sizes=function_sizes) | ||
| ) | ||
| lengths = {len(bits) for bits in resolved if len(bits) > 1} |
There was a problem hiding this comment.
[L1] A length-1 register is treated as a single qubit — Implementation · Low
Rationale: len(bits) > 1 decides from the resolved length whether an operand is a register, not from the operand itself. So qubit[3] q; qubit[1] r; cx q, r; gives cx q[0], r[0]; cx q[1], r[0]; cx q[2], r[0];, and cx q[0:1], q[2:2]; repeats q[2]. Neither raises. The spec requires broadcast registers to have the same length, and Qiskit's importer rejects the first program with mismatched lengths in gate broadcast. This is the one mismatched-length case the new check misses. Main also gave a wrong circuit here.
Change requested: decide "register operand" from the operand's form (a bare register name, alias, slice or set) and its declared type, not from its resolved length. Then apply the length check to every register operand. If this stays as a design choice, add one line saying so to the _broadcast_operands docstring.
There was a problem hiding this comment.
Partly applied in a6defcf.
Now applied: slices and sets are register operands by their form. cx q[0:1], q[2:2] now raises different lengths [1, 2], and test_broadcast_length_one_slice_is_a_register covers it.
Kept as a documented choice: a bare qubit[1] r register. The register's declared type can't be relied on, because _visit_quantum_register rewrites qubit x; to qubit[1] x; in place on the caller's AST. After validate(), dumps() already prints qubit[1] x; cx x, q;, so a strict check would reject the reload of a valid program.
I tried two ways around that, and both broke existing behaviour:
- Copying the declaration:
remove_idle_qubitsrelies on the unrolled declaration being the same object as the module's. - Skipping the rewrite under
check_only: this changes the echoed source in five declaration-error tests, and breakspopulate_idle_qubits.
_is_register_operand's docstring now says this, and test_broadcast_single_qubit_repeats pins the qubit x behaviour. Stopping that in-place rewrite seems like its own change, so I can open an issue for it if you agree.
| Returns None when it cannot be known up front (an opaque gate, or a non-constant | ||
| modifier argument), which leaves broadcasting to the existing path. | ||
| """ | ||
| name = operation.name.name | ||
| if name in self._custom_gates: | ||
| count = len(self._custom_gates[name].qubits) | ||
| elif name in self._opaque_gates: | ||
| return None | ||
| else: |
There was a problem hiding this comment.
[L2] Opaque branch is unreachable — Maintenance · Low
Rationale: rewrite_opaque_declarations (preprocess.py) rewrites each opaque into a gate definition with an empty body, so its name is always in self._custom_gates first. elif name in self._opaque_gates never runs. Opaque gates broadcast like custom gates (opaque g a, b; g q, r; gives g q[0], r[0]; g q[1], r[1];), which is correct. But the docstring and the PR description both say opaque gates keep the old path.
Change requested:
| Returns None when it cannot be known up front (an opaque gate, or a non-constant | |
| modifier argument), which leaves broadcasting to the existing path. | |
| """ | |
| name = operation.name.name | |
| if name in self._custom_gates: | |
| count = len(self._custom_gates[name].qubits) | |
| elif name in self._opaque_gates: | |
| return None | |
| else: | |
| Returns None when it cannot be known up front (a non-constant modifier argument), | |
| which leaves broadcasting to the existing path. An ``opaque`` gate is a custom gate | |
| with an empty body by now, so it broadcasts like one. | |
| """ | |
| name = operation.name.name | |
| if name in self._custom_gates: | |
| count = len(self._custom_gates[name].qubits) | |
| else: |
There was a problem hiding this comment.
Applied in a6defcf: I removed the branch and used your docstring. The PR description now says that opaque gates broadcast like custom gates, which fixes the opaque g a, b; g q, r; pairing.
| with pytest.raises(ValidationError, match="different lengths"): | ||
| loads(qasm3_string).unroll() |
There was a problem hiding this comment.
[L3] Missing OpenQASM 2 and duplicate-qubit tests — Implementation · Low
Rationale: OpenQASM 2 cx q, r; gave cx q[0], q[1]; cx r[0], r[1]; on main, and this PR fixes it, but no test covers it. cx q, q; now raises Duplicate qubit through the broadcast path, and nothing tests that either. Both tests below pass on head.
Change requested:
| with pytest.raises(ValidationError, match="different lengths"): | |
| loads(qasm3_string).unroll() | |
| with pytest.raises(ValidationError, match="different lengths"): | |
| loads(qasm3_string).unroll() | |
| def test_broadcast_qasm2_registers(): | |
| """OpenQASM 2 pairs register operands the same way.""" | |
| result = loads('OPENQASM 2.0;\ninclude "qelib1.inc";\nqreg q[2];\nqreg r[2];\ncx q, r;\n') | |
| result.unroll() | |
| gates = [line for line in dumps(result).splitlines() if line.startswith("cx")] | |
| assert gates == ["cx q[0], r[0];", "cx q[1], r[1];"] | |
| def test_broadcast_duplicate_qubit_raises(): | |
| """Broadcasting a register against itself repeats a qubit within one gate.""" | |
| qasm3_string = 'OPENQASM 3.0;\ninclude "stdgates.inc";\nqubit[2] q;\ncx q, q;\n' | |
| with pytest.raises(ValidationError, match="Duplicate qubit"): | |
| loads(qasm3_string).unroll() |
There was a problem hiding this comment.
Added both in a6defcf, plus three more:
test_broadcast_length_one_slice_is_a_registertest_broadcast_empty_operand_raisestest_broadcast_single_qubit_repeats
After merging main, the suite is at 1033 passed, 3 skipped, and format-check is clean.
…eat slices as registers Address review on #462: - Check operand forms before counting gate qubits or resolving operands. An indexed operand without a slice or set names one qubit, so plain `cx q[0], q[1]` no longer resolves every operand twice (30k-gate unroll: 1.86s -> 1.42s; main 1.37s). - A slice or set is a register operand even when it names one qubit, so `cx q[0:1], q[2:2]` raises a length mismatch instead of repeating `q[2]`. A bare length-1 register is still repeated, because the visitor rewrites `qubit x` to `qubit[1] x` in place. - Raise a QASM error for an empty broadcast operand instead of an IndexError. - Drop the unreachable opaque branch in `_gate_qubit_count`; opaque gates are custom gates with empty bodies by then. - Test OpenQASM 2 registers, duplicate qubits, length-1 slices, empty operands and single-qubit repetition.
…ting # Conflicts: # CHANGELOG.md
Closes #384
Problem
unroll()flattened every operand of a gate call into one list and chunked it by the gate's arity. With register operands this silently produced a different circuit:cx a, b;(twoqubit[2])cx a[0], a[1]; cx b[0], b[1];cx a[0], b[0]; cx a[1], b[1];cx q[0:1], q[2:3];cx q[0], q[1]; cx q[2], q[3];cx q[0], q[2]; cx q[1], q[3];let a = q[0:1]; let b = q[2:3]; cx a, b;def f(qubit[2] a, qubit[2] b) { cx a, b; }f(r, s);cx r[0], r[1]; cx s[0], s[1];cx r[0], s[0]; cx r[1], s[1];ctrl @ x a, b;cx a[0], a[1]; cx a[0], b[0]; cx a[0], b[1];cx a[0], b[0]; cx a[1], b[1];cx q, r[0];(qubit[3] q)ValidationErrorcx q[0], r[0]; cx q[1], r[0]; cx q[2], r[0];g4 qr0[0], qr1, qr2[0], qr3;ValidationErrorg4calls, one per index ofqr1/qr3Change
_visit_generic_gate_operationnow broadcasts first, in a new_broadcast_operands:ValidationError: Cannot broadcast 'cx' over registers of different lengths [2, 3]. A slice or set counts as a register operand even when it names one qubit, socx q[0:1], q[2:2]raises too. A barequbit[1]register is still repeated like a single qubit, because the visitor rewritesqubit xtoqubit[1] xin place and the two can't be told apart afterwards.cx q[0:1], q[3:2]) raisesValidationErrorinstead ofIndexError.cx q[0], q[1]) skips the resolution. Unrolling 30k such gates takes 1.42s, against 1.37s onmain.opaque g a, b; g q, r;, which paired wrongly onmain.cx q2;,inv @ ccx q[0], q2;,ctrl (2) @ x a, b[0];, which existing tests rely on), thecx q[0], q[1], q[2], q[3]form, and non-constant modifier arguments. The issue left it to the maintainers whether to keep that register-fills-several-operands extension, so this PR leaves it alone.Testing
tests/qasm3/test_broadcast.py: two registers, slices, aliases, subroutine register arguments,swap,ctrl @, the mixed register/qubit form, the spec'sg4example, length mismatches (including a length-1 slice), empty operands, OpenQASM 2 registers, and duplicate qubits.main: 1033 passed, 3 skipped.black,isort,pylint(10.00),headerspass.mypyreports only thetyper/richimport errors that come from the tox environment lacking the CLI extra.negctrl @, a parametrised custom two-qubit gate on registers, andcxkept withunroll(external_gates=["cx"])all pair by index.Found while testing qBraid/qBraid#1468, whose
qasm3 -> qasm2conversion currently detects register broadcasts and falls back to another path to avoid this bug.