-
Notifications
You must be signed in to change notification settings - Fork 84
Support new discrete qp.PPR in to_ppr pass.
#3185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 23 commits
4b5d10f
0b3833e
ec5ef4d
702d8ba
a9d6071
64b3482
ec13245
b87dca7
2de9ff9
216ae91
37fe90c
a608b1d
0cbce94
6e88763
b0baf11
37bbec3
11f7488
ba7f7cd
e2b7543
90cb543
6af8a39
a8f598b
642684b
becba4d
3f6aa57
037dd82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| # Adjoint PPR-to-PPM Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Make `ppr-to-ppm` resolve `quantum.adjoint` regions before decomposing their PPR operations into PPM operations. | ||
|
|
||
| **Architecture:** Reuse the existing adjoint-lowering rewrite implementation by exposing its pattern-population function through the Quantum transforms pattern API. `PPRToPPMPass` applies those patterns first, then retains its existing non-Clifford and Clifford decomposition phases. | ||
|
|
||
| **Tech Stack:** C++20, MLIR rewrite patterns and passes, CMake, LLVM lit/FileCheck. | ||
|
|
||
| ## Global Constraints | ||
|
|
||
| - Reuse the canonical adjoint lowering rather than duplicating PPR-specific SSA reversal logic. | ||
| - A PPR inside an adjoint has its operation order reversed and its rotation kind negated before PPR decomposition. | ||
| - Any adjoint-lowering failure fails `ppr-to-ppm` before decomposition begins. | ||
| - Preserve the existing non-Clifford-then-Clifford decomposition order. | ||
|
|
||
| --- | ||
|
|
||
| ### Task 1: Resolve adjoints before PPR decomposition | ||
|
|
||
| **Files:** | ||
| - Modify: `mlir/test/PBC/PPRToPPM.mlir` | ||
| - Modify: `mlir/include/Quantum/Transforms/Patterns.h` | ||
| - Modify: `mlir/lib/Quantum/Transforms/AdjointLowering/AdjointLowering.cpp` | ||
| - Modify: `mlir/lib/PBC/Transforms/ppr_to_ppm.cpp` | ||
| - Modify: `mlir/lib/PBC/Transforms/CMakeLists.txt` | ||
|
|
||
| **Interfaces:** | ||
| - Consumes: Existing `AdjointSingleOpRewritePattern`, `applyPatternsGreedily`, and PPR decomposition pattern-population functions. | ||
| - Produces: `void catalyst::quantum::populateAdjointLoweringPatterns(mlir::RewritePatternSet &patterns)`. | ||
|
|
||
| - [ ] **Step 1: Write the failing regression test** | ||
|
|
||
| Append an input section to `mlir/test/PBC/PPRToPPM.mlir`. Adjoint lowering must | ||
| reverse `Z(4)` followed by `X(-4)` into `X(4)` followed by `Z(-4)`. Clifford | ||
| decomposition represents those signs with a negated `["X", "Y"]` PPM followed | ||
| by a non-negated `["Z", "Y"]` PPM. | ||
|
|
||
| ```mlir | ||
| // ----- | ||
|
|
||
| func.func @test_ppr_to_ppm_adjoint(%q0 : !quantum.bit) -> !quantum.bit { | ||
| %0 = quantum.adjoint(%q0) : !quantum.bit { | ||
| ^bb0(%arg0: !quantum.bit): | ||
| %1 = pbc.ppr ["Z"](4) %arg0 : !quantum.bit | ||
| %2 = pbc.ppr ["X"](-4) %1 : !quantum.bit | ||
| quantum.yield %2 : !quantum.bit | ||
| } | ||
| return %0 : !quantum.bit | ||
|
|
||
| // CHECK-LABEL: @test_ppr_to_ppm_adjoint | ||
| // CHECK-NOT: quantum.adjoint | ||
| // CHECK: pbc.ppm ["X", "Y"](-) %q0 | ||
| // CHECK: pbc.ppm ["Z", "Y"] {{.*}} | ||
| // CHECK-NOT: quantum.adjoint | ||
| // CHECK: return | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Run the regression test and verify RED** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| build/bin/quantum-opt --ppr-to-ppm --split-input-file -verify-diagnostics mlir/test/PBC/PPRToPPM.mlir | build/bin/FileCheck mlir/test/PBC/PPRToPPM.mlir --check-prefix=CHECK | ||
| ``` | ||
|
|
||
| Expected: FAIL in `test_ppr_to_ppm_adjoint` because current output retains `quantum.adjoint` and lowers the positive PPR to a negated PPM inside it. | ||
|
|
||
| - [ ] **Step 3: Expose the shared adjoint-lowering patterns** | ||
|
|
||
| Add this declaration to the `catalyst::quantum` namespace in `mlir/include/Quantum/Transforms/Patterns.h`: | ||
|
|
||
| ```cpp | ||
| void populateAdjointLoweringPatterns(mlir::RewritePatternSet &patterns); | ||
| ``` | ||
|
|
||
| In `mlir/lib/Quantum/Transforms/AdjointLowering/AdjointLowering.cpp`, include the public pattern header: | ||
|
|
||
| ```cpp | ||
| #include "Quantum/Transforms/Patterns.h" | ||
| ``` | ||
|
|
||
| Define the population function after the anonymous namespace: | ||
|
|
||
| ```cpp | ||
| namespace catalyst { | ||
| namespace quantum { | ||
|
|
||
| void populateAdjointLoweringPatterns(RewritePatternSet &patterns) { | ||
| patterns.add<AdjointSingleOpRewritePattern>(patterns.getContext(), 1); | ||
| } | ||
|
|
||
| } // namespace quantum | ||
| } // namespace catalyst | ||
| ``` | ||
|
|
||
| Replace the pass-local `patterns.add<AdjointSingleOpRewritePattern>(...)` call with: | ||
|
|
||
| ```cpp | ||
| populateAdjointLoweringPatterns(patterns); | ||
| ``` | ||
|
|
||
| - [ ] **Step 4: Apply adjoint lowering first in `ppr-to-ppm`** | ||
|
|
||
| Add the public Quantum pattern include to `mlir/lib/PBC/Transforms/ppr_to_ppm.cpp`: | ||
|
|
||
| ```cpp | ||
| #include "Quantum/Transforms/Patterns.h" | ||
| ``` | ||
|
|
||
| At the start of `runOnOperation`, before constructing non-Clifford patterns, add: | ||
|
|
||
| ```cpp | ||
| RewritePatternSet adjoint_patterns(ctx); | ||
| quantum::populateAdjointLoweringPatterns(adjoint_patterns); | ||
|
|
||
| if (failed(applyPatternsGreedily(module, std::move(adjoint_patterns)))) { | ||
| return signalPassFailure(); | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 5: Link the shared implementation** | ||
|
|
||
| Add `quantum-transforms` to `LIBS` in `mlir/lib/PBC/Transforms/CMakeLists.txt`: | ||
|
|
||
| ```cmake | ||
| set(LIBS | ||
| ${dialect_libs} | ||
| ${conversion_libs} | ||
| MLIRPBC | ||
| PBCUtils | ||
| PBCAnalysis | ||
| quantum-transforms | ||
| ) | ||
| ``` | ||
|
|
||
| - [ ] **Step 6: Build the affected tools** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| cmake --build build --target quantum-opt FileCheck -j2 | ||
| ``` | ||
|
|
||
| Expected: Build succeeds without compile or link errors. | ||
|
|
||
| - [ ] **Step 7: Run the focused test and verify GREEN** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| build/bin/quantum-opt --ppr-to-ppm --split-input-file -verify-diagnostics mlir/test/PBC/PPRToPPM.mlir | build/bin/FileCheck mlir/test/PBC/PPRToPPM.mlir --check-prefix=CHECK | ||
| ``` | ||
|
|
||
| Expected: PASS. The output contains no `quantum.adjoint`; the reversed | ||
| `X(-4)` decomposes first as a negated `["X", "Y"]` PPM, and the reversed | ||
| `Z(4)` decomposes second as a non-negated `["Z", "Y"]` PPM. | ||
|
|
||
| - [ ] **Step 8: Run adjacent adjoint and PBC tests** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| build/bin/llvm-lit -sv mlir/test/PBC/PPRToPPM.mlir mlir/test/PBC/AdjointTest.mlir mlir/test/Quantum/AdjointTest.mlir | ||
| ``` | ||
|
|
||
| Expected: All selected tests pass. | ||
|
|
||
| - [ ] **Step 9: Commit the implementation** | ||
|
|
||
| ```bash | ||
| git add mlir/test/PBC/PPRToPPM.mlir \ | ||
| mlir/include/Quantum/Transforms/Patterns.h \ | ||
| mlir/lib/Quantum/Transforms/AdjointLowering/AdjointLowering.cpp \ | ||
| mlir/lib/PBC/Transforms/ppr_to_ppm.cpp \ | ||
| mlir/lib/PBC/Transforms/CMakeLists.txt | ||
| git commit -m "fix: resolve adjoint PPRs before PPM lowering" | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Resolve Adjoint PPRs Before PPM Lowering | ||
|
|
||
| ## Goal | ||
|
|
||
| Ensure `ppr-to-ppm` never lowers a Pauli product rotation while it is still | ||
| nested inside `quantum.adjoint`. The pass must first resolve the adjoint, | ||
| including reversing operation order and negating each PPR rotation kind, and | ||
| only then decompose PPRs into PPMs. | ||
|
|
||
| ## Design | ||
|
|
||
| Expose the existing adjoint-lowering rewrite-pattern population function from | ||
| the Quantum transforms library. At the start of `PPRToPPMPass::runOnOperation`, | ||
| apply those patterns greedily to the module. Reuse is important because the | ||
| existing implementation already handles SSA remapping, reversed operation | ||
| order, nested control-flow requirements, and PPR angle negation. | ||
|
|
||
| After adjoint lowering succeeds, retain the current two decomposition phases: | ||
|
|
||
| 1. Decompose non-Clifford PPRs. | ||
| 2. Decompose Clifford PPRs. | ||
|
|
||
| If adjoint lowering fails, signal pass failure and do not attempt PPR | ||
| decomposition. | ||
|
|
||
| ## Build Integration | ||
|
|
||
| Link the PBC transforms library against the Quantum transforms library so | ||
| `ppr-to-ppm` can populate the shared adjoint-lowering patterns. | ||
|
|
||
| ## Testing | ||
|
|
||
| Extend the `PPRToPPM.mlir` lit test with an adjoint region containing PPRs. | ||
| Check that: | ||
|
|
||
| - `quantum.adjoint` is absent after the pass. | ||
| - PPRs are processed in reverse order. | ||
| - Their rotation kinds are negated before decomposition. | ||
| - No PPM remains nested beneath an adjoint operation. | ||
|
|
||
| The test is added and observed failing before production code changes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -411,6 +411,54 @@ LogicalResult convertPauliRotGate(PauliRotOp op, ConversionPatternRewriter &rewr | |
| op.getAdjoint(), rewriter); | ||
| } | ||
|
|
||
| LogicalResult convertPPROperator(OperatorOp op, ConversionPatternRewriter &rewriter) { | ||
| if (!op.getAllParams().empty()) { | ||
| return op.emitOpError("PPR operator does not support dynamic parameters"); | ||
| } | ||
|
Comment on lines
+415
to
+417
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it the responsibility of this pass to check that the PPR op is malformed? 🤔 I would say no, because if it was does that mean we check such properties over and over again in any pass that interacts with an op? The line is not 100% clear to me, but generally assumptions the pass uses concretely uses could be added as assertions, but general verification should the responsibility of a different piece of code (normally the verifier, but this is where having opaque ops is a disadvantage, as we don't have any op-specific verifiers for them).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mhm yeah, that is a fair point. I am not so experienced with contributing at this level, so I was not aware that this is a bit out of place. |
||
|
|
||
| DictionaryAttr staticData = op.getStaticData(); | ||
| auto denominatorAttr = staticData.getAs<IntegerAttr>("angle_denominator"); | ||
|
|
||
| int64_t denominator = denominatorAttr.getInt(); | ||
| if (denominator != 2 && denominator != -2 && denominator != 4 && denominator != -4 && | ||
| denominator != 8 && denominator != -8) { | ||
| return op.emitOpError("unsupported PPR angle denominator: ") << denominator; | ||
| } | ||
|
|
||
| auto pauliWordAttr = staticData.getAs<StringAttr>("pauli_word"); | ||
| if (!pauliWordAttr) { | ||
| return op.emitOpError("PPR operator requires a string 'pauli_word' in static_data"); | ||
| } | ||
|
Comment on lines
+429
to
+431
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor inconsistency that here we check the existence of the attribute but above we don't (we just use it directly). |
||
|
|
||
| StringRef pauliWord = pauliWordAttr.getValue(); | ||
| if (pauliWord.empty()) { | ||
| return op.emitOpError("PPR operator requires a non-empty Pauli word"); | ||
| } | ||
| if (pauliWord.size() != op.getInQubits().size()) { | ||
| return op.emitOpError("PPR operator requires one Pauli character per input qubit"); | ||
| } | ||
|
|
||
| SmallVector<Attribute> pauliCharacters; | ||
| pauliCharacters.reserve(pauliWord.size()); | ||
| for (char pauli : pauliWord) { | ||
| if (pauli != 'X' && pauli != 'Y' && pauli != 'Z' && pauli != 'I') { | ||
| return op.emitOpError("PPR operator Pauli word may contain only X, Y, Z and I"); | ||
| } | ||
| pauliCharacters.push_back(rewriter.getStringAttr(StringRef(&pauli, 1))); | ||
| } | ||
|
|
||
| ArrayAttr pauliProduct = rewriter.getArrayAttr(pauliCharacters); | ||
| int8_t rotationKind = static_cast<int8_t>(denominator); | ||
| if (op.getAdjoint()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to check the op is uncontrolled? We are currently just dropping control qubits I think.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, very good catch 😅 |
||
| rotationKind = -rotationKind; | ||
| } | ||
|
|
||
| auto pprOp = | ||
| PPRotationOp::create(rewriter, op.getLoc(), pauliProduct, rotationKind, op.getInQubits()); | ||
| rewriter.replaceOp(op, pprOp.getOutQubits()); | ||
| return success(); | ||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // PBC Lowering Patterns | ||
| //===----------------------------------------------------------------------===// | ||
|
|
@@ -420,7 +468,7 @@ struct PBCGateLowering : public OpInterfaceConversionPattern<QuantumOperation> { | |
|
|
||
| LogicalResult matchAndRewrite(QuantumOperation operation, ArrayRef<Value> operands, | ||
| ConversionPatternRewriter &rewriter) const final { | ||
| StringRef supportedGates = "Supported gates: H, S, T, X, Y, Z, S†, T†, I, CNOT, CZ, " | ||
| StringRef supportedGates = "Supported gates: H, S, T, X, Y, Z, S†, T†, I, CNOT, CZ, PPR," | ||
| "RX, RY, RZ, IsingXX, IsingYY, IsingZZ, MultiRZ, and PauliRot."; | ||
| Operation *op = operation.getOperation(); | ||
|
|
||
|
|
@@ -469,6 +517,10 @@ struct PBCGateLowering : public OpInterfaceConversionPattern<QuantumOperation> { | |
| return convertMultiRZGate(originOp, rewriter); | ||
| } else if (auto originOp = dyn_cast<PauliRotOp>(op)) { | ||
| return convertPauliRotGate(originOp, rewriter); | ||
| } else if (auto originOp = dyn_cast<OperatorOp>(op)) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better to add a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean create a custom mlir op for it, rather than representing it as an quantum.operator op? It's true that currently we're assuming any quantum.operator instance would be resolved by the graph decomposer (before further compilation or execution). |
||
| if (originOp.getOpName() == "PPR") { | ||
| return convertPPROperator(originOp, rewriter); | ||
| } | ||
| } | ||
|
|
||
| return op->emitError("Unsupported operation for PBC conversion. " + supportedGates); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.