Keep the control when decomposing a controlled swap by VaggelisGian · Pull Request #5276 · NVIDIA/cuda-quantum

VaggelisGian · 2026-08-26T09:34:10Z

SwapToCX read only the two targets of a quake.swap and emitted three CNOTs between them, silently dropping any controls. A controlled swap translated to OpenQASM 2 therefore produced an unconditional swap, which is a different operator.

What this does:

Handles the single-control case with the standard Fredkin lowering cx b,a; ccx c,a,b; cx b,a, so it composes with the existing CCXToCCZ / CCZToCX patterns on gate sets without a native toffoli.
Forwards a complemented control via x wrappers around the whole gate.
Returns failure for swaps with more than one control instead of dropping the extra controls, matching how other patterns in the file bail out (checkNumControls).
Registers the additional source set {swap(1), x(1), x(2)} next to the existing {swap, x(1)}, which makes the pattern selectable for controlled swaps in decomposition-graph runs.

The unit tests pin the exact toffoli wiring, the complemented-control x wrappers, that swaps with more than one control or an unsized veq control are left untouched, and that a basis which already provides a plain swap keeps bare swaps via the disabled-control-counts mechanism. A dedicated SwapToCXControlled.qke covers the controlled rewrite at the IR level; it uses FileCheck only because CircuitCheck's UnitaryBuilder has no controlled multi-target path and would miscompute the reference unitary of a controlled swap. The stale controlled-swap expectation in all_qir_gates.qke, which encoded exactly the dropped-control output this PR removes, is updated to the Fredkin lowering (pinning only the sandwiching CNOTs since the middle toffoli is decomposed further on that basis).

Note: #5263 independently adds end-to-end coverage through cudaq.translate to OpenQASM 2. The two cover different layers and compose well; that one exercises the full pipeline, this PR fixes the lowering itself and pins the IR-level wiring.

DCO: all commits signed off as required.

Test output (devcontainer cu12.6-gcc12, Release, no GPU in this environment):

$ ctest -R 'DecompositionPattern|SwapToCX|all_qir_gates|skipsNonGate'
100% tests passed, 0 tests failed out of 39

$ llvm-lit cudaq/test/Transforms
Passed           : 324 (99.69%)
Expectedly Failed:   1 (0.31%)   <- pre-existing XFAIL in CliffordTSynthesis, untouched by this branch

The filtered unit/selection run includes the new SwapToCXKeepsSingleControl, SwapToCXForwardsComplementedControl, SwapToCXRejectsUnknownOrMultipleControls, SwapToCXRespectsDisabledControlCounts and SwapToCXDisablesControlCountsCoveredByTheBasis cases, plus the updated FullDecompositionPatternSelectionTest.DecomposeCCXToCZ. pre-commit run --all-files --hook-stage pre-push passes on every hook except markdown-link-check, whose single failure is a pre-existing stale badge URL in the top-level README.md (untouched by this branch); all other checked files report OK.

copy-pr-bot · 2026-08-26T09:34:14Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

SwapToCX only read the two targets of a quake.swap and emitted three CNOTs between them, silently dropping any controls. A controlled swap translated to OpenQASM 2 therefore produced an unconditional swap, which implements a different operator. Handle the single-control case by emitting the standard Fredkin lowering (cx b,a; ccx c,a,b; cx b,a) so it composes with the existing CCXToCCZ and CCZToCX patterns on gate sets without a native toffoli, forward a complemented control via x wrappers around the whole gate, and make the pattern return failure for swaps with more than one control instead of dropping them. The unit test pins down the toffoli wiring so a decomposition that is not a cswap cannot pass. Fixes NVIDIA#5192 Signed-off-by: Vaggelis <baggelis100@gmail.com>

SwapToCX now registers a separate single-control source set, so the selection graph lists the pattern twice for this basis. Pin both entries so the test reflects the intended graph. Signed-off-by: Vaggelis <baggelis100@gmail.com>

The basis-conversion expectation for the controlled swap still pinned the control-dropping output this PR removes; update it to the Fredkin lowering, pinning only the sandwiching CNOTs since the toffoli in the middle is decomposed further on that basis. Add a dedicated SwapToCXControlled.qke covering the controlled rewrite at the IR level. It uses FileCheck only because CircuitCheck's UnitaryBuilder has no controlled multi-target path and would miscompute the reference unitary of a controlled swap. Extend the unit tests to cover what review flagged as untested: the complemented-control x wrappers, leaving swaps with more than one control or an unsized veq control untouched, and the disabled-control- count behavior that comes with registering the swap(1) source set next to swap (a basis that already provides a plain swap keeps bare swaps). Signed-off-by: Vaggelis <baggelis100@gmail.com>

schweitzpgi · 2026-08-26T16:02:29Z

 // quake.cnot a, b;
 // quake.cnot b, a;
+//
+// quake.swap<ctrl> c, a, b


Suggested change

// quake.swap<ctrl> c, a, b

// quake.swap [c] a, b

schweitzpgi · 2026-08-26T16:06:02Z

+// quake.cnot b, a;
+// quake.cnot<ctrl> c, a, b;
+// quake.cnot b, a;


cnot is not a quake gate, fwiw. This makes is more consistent

Suggested change

// quake.cnot b, a;

// quake.cnot<ctrl> c, a, b;

// quake.cnot b, a;

// quake.x [b] a;

// quake.x [c, a] b;

// quake.x [b] a;

schweitzpgi · 2026-08-26T16:07:50Z

+// the terms of the Apache License 2.0 which accompanies this distribution.   //
+// ========================================================================== //
+
+// CircuitCheck is deliberately not used here: its UnitaryBuilder has no


Please file an issue for tracking purposes.

Filed #5304 for the CircuitCheck gap. The comment in the test file now points to it.

Both comment suggestions are applied in 42d24ab.

quake has no cnot op, the x op with controls is the cnot, and the pattern header should use the bracket control notation. Requested in review on NVIDIA#5276. Signed-off-by: Vaggelis <baggelis100@gmail.com>

taalexander · 2026-08-27T12:18:20Z

+        qRewriter.create<cudaq::quake::XOp>(loc, c);
+      qRewriter.create<cudaq::quake::XOp>(loc, b, a);
+      SmallVector<Value, 2> ccxControls{c, a};
+      qRewriter.create<cudaq::quake::XOp>(loc, ccxControls, b);


Instead of adding the CX before/after to negate the sequence could we instead negate the first control here and allow the rest of the downstream infrastructure to handle?

Done in 6f8d484. The outer cnots do not touch the control qubit, so complementing the toffoli's first control is equivalent to the x pair, and expand-control-negations runs after decomposition in the target deploy pipeline to materialize it. One wrinkle: the verifier wants one flag per control operand, so the forwarded attr is padded to [flag, false] on the two-control toffoli.

taalexander · 2026-08-27T12:23:31Z

+      qRewriter.create<cudaq::quake::XOp>(loc, b, a);
+    }

    qRewriter.selectWiresAndReplaceUses(op, ValueRange{a, b});


I'm wondering if we're missing c here? Thoughts @schweitzpgi?

Yes, it was missing. The op's wires are ordered controls first, then targets, so the replacement list now carries all three outputs ({toffeoli control wire out, a, b}); with reference semantics nothing changes since there are no results, but under wire semantics the control's downstream users were left unmapped. Fixed in 6f8d484.

The outer cnots of the Fredkin lowering do not touch the control qubit, so a complemented control is equivalent to complementing the first control on the middle toffoli. Forwarding the negation flag there (padded to one flag per control operand as the verifier requires) lets the existing expand-control-negations pass materialize the inversion instead of this pattern emitting its own x pair. Also replace the original op's uses with all three output wires: the wires are ordered controls first, then targets, and dropping the control left its downstream users unmapped in wire semantics. Requested in review on NVIDIA#5276. Signed-off-by: Vaggelis <baggelis100@gmail.com>

taalexander · 2026-08-27T13:17:57Z

+        ccxOp.setNegatedQubitControls(
+            DenseBoolArrayAttr::get(rewriter.getContext(), flags));
+      }
+      qRewriter.create<cudaq::quake::XOp>(loc, b, a);


Should this be the updated a from ccxControls?

Yes, fixed in c280277. The toffoli create updates ccxControls and the target in place, so the final cnot now consumes ccxControls[1] and the replacement list reads {ccxControls[0], ccxControls[1], b} after that last update. Reference-semantics IR is unaffected; under wire semantics the stale a pointed at a wire the toffoli had already consumed. Added SwapToCXThreadsWireOperands to pin the threading.

taalexander · 2026-08-27T13:18:45Z


-    qRewriter.selectWiresAndReplaceUses(op, ValueRange{a, b});
+      // The wires are ordered controls first, then targets.
+      qRewriter.selectWiresAndReplaceUses(op, ValueRange{ccxControls[0], a, b});


Should a be ccxControls[1] as well?

Yes. c280277 has both read the toffoli output: the final cnot is created with ccxControls[1] as target and the wire replacement list is {ccxControls[0], ccxControls[1], b}.

The three-arg create updates the controls vector and target in place, so the final cnot and the wire replacement list must read the toffoli's updated outputs (ccxControls[1] for target a) instead of the stale local a, which refers to a wire the toffoli already consumed under wire semantics. Reference-semantics IR is unaffected. Requested in review on NVIDIA#5276. Signed-off-by: Vaggelis <baggelis100@gmail.com>

VaggelisGian · 2026-08-28T14:52:30Z

Superseded by #5083 (9f46f08): it lands the same kept-control Fredkin lowering in SwapToCX, including the control and target wire threading raised in review, and covers positive and negative controls in controlled_decomposition_polarity_exact.qke. Closing in favor of the upstream implementation.

github-actions Bot added the core compiler label Aug 26, 2026

VaggelisGian added 3 commits August 26, 2026 09:20

Update DecomposeCCXToCZ selection expectation

e8bcab6

SwapToCX now registers a separate single-control source set, so the selection graph lists the pattern twice for this basis. Pin both entries so the test reflects the intended graph. Signed-off-by: Vaggelis <baggelis100@gmail.com>

VaggelisGian force-pushed the fix-swapctrl-decomp branch from 07ea91e to 149c3b0 Compare August 26, 2026 13:22

schweitzpgi requested review from atgeller, schweitzpgi and taalexander August 26, 2026 16:00

schweitzpgi reviewed Aug 26, 2026

View reviewed changes

cleitonaugusto mentioned this pull request Aug 26, 2026

test: cover the control on a controlled swap through openqasm2 #5263

Open

VaggelisGian mentioned this pull request Aug 27, 2026

CircuitCheck UnitaryBuilder silently drops controls on multi-target gates #5304

Closed

Use quake gate names in the controlled swap comment

42d24ab

quake has no cnot op, the x op with controls is the cnot, and the pattern header should use the bracket control notation. Requested in review on NVIDIA#5276. Signed-off-by: Vaggelis <baggelis100@gmail.com>

taalexander requested changes Aug 27, 2026

View reviewed changes

VaggelisGian closed this Aug 28, 2026

VaggelisGian deleted the fix-swapctrl-decomp branch August 28, 2026 14:52

github-actions Bot pushed a commit that referenced this pull request Aug 28, 2026

Cleaning up docs preview for PR #5276.

6d9d531

Uh oh!

Conversation

VaggelisGian commented Aug 26, 2026

Uh oh!

copy-pr-bot Bot commented Aug 26, 2026

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!

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!

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!

VaggelisGian commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants