⚡️ Improve canonicalizations for gate fusion and inverse cancellation#1782
Conversation
simon1hofmann
added
MLIR
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThis PR adds tolerant parameter comparison and ctrl-chain partner discovery utilities, refactors/extends gate merge templates, and registers new canonicalization patterns across single-target and two-target QCO gates with corresponding unit-test builders and test cases. ChangesQCO gate merges through control chains
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/QCOUtils.h`:
- Around line 53-60: addFloatParameters currently creates arith::AddFOp at the
rewriter's current insertion point which can be before one operand's def and
yield non-dominating uses; update addFloatParameters to materialize the add at a
dominance-safe location by computing the later-definition point of lhs and rhs
(e.g., inspect lhs.getDefiningOp() and rhs.getDefiningOp() and choose the later
op in the same block) and call rewriter.setInsertionPointAfter(that op) (or
otherwise set the insertion point to a block position that is guaranteed to
dominate both uses) before creating arith::AddFOp; apply this change so callers
like mergeOneTargetOneParameterThroughCtrlControlChain,
mergeOneTargetTwoParameter, mergeTwoTargetOneParameterImpl, and
mergeTwoTargetTwoParameterImpl do not need to manually adjust insertion point
and ensure the surviving gate is chosen/adjusted to be the later one when
materialization must occur later.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 641-643: The negative tests "TwoRZSplitAcrossCtrlTargetWires" and
"TwoXXMinusYYMismatchedBeta" are using the same builder for both sides so
ProgramEquivalence's canonicalization masks mismatches; update the test harness
so these negative cases do not canonicalize the reference side: locate the
creation/registration of QCOTestCase entries for those names and set
canonicalizeReference = false (or otherwise pass a distinct, non-canonicalized
reference) so the incorrect merge cannot be applied to both modules; do the same
change for the other occurrence of these cases around the later block mentioned
(the second set at lines ~1031-1033).
- Around line 482-486: Add a test case that exercises the tolerance branch by
inserting a QCOTestCase where the two constants differ by a small delta less
than utils::TOLERANCE and assert the merged canonical form (same expectation as
the exact-equality case): locate the test vectors where QCOTestCase entries are
constructed (e.g., the entries with names "CanonicalizeRToRx",
"CanonicalizeRToRy", and "TwoR") and add a new case (name like
"TwoRWithinTolerance") using the same builder for the canonical form (e.g.,
MQT_NAMED_BUILDER(r)) and a source builder that produces the near-equal
constants (similar to MQT_NAMED_BUILDER(twoR) but with phi/beta adjusted by a
delta < utils::TOLERANCE); repeat this addition in the other test list
referenced (the block around the other occurrence of QCOTestCase at the later
location).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18fed9e1-adaf-49b4-bc5a-342b6992aaac
📒 Files selected for processing (13)
mlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/lib/Dialect/QCO/IR/Operations/StandardGates/POp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/ROp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXMinusYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks a lot for your work on this, @simon1hofmann! 🙂 This already looks really good to me. I just have some points on consistency ✨ below.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp (1)
485-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TwoRcan still pass if mismatched-phimerging is broken.Because Line 91 canonicalizes the reference module too, this mixed case (one merge-allowed pair + one merge-forbidden pair) can become self-fulfilling: a buggy rewrite may apply to both sides and still pass. This mirrors the previously addressed harness pitfall and is reintroduced by the new
TwoRcase.💡 Minimal hardening
struct QCOTestCase { std::string name; mqt::test::NamedBuilder<QCOProgramBuilder> programBuilder; mqt::test::NamedBuilder<QCOProgramBuilder> referenceBuilder; + bool canonicalizeReference = true; @@ - EXPECT_TRUE(runQCOCleanupPipeline(reference.get()).succeeded()); - printer.record(reference.get(), "Canonicalized Reference QCO IR" + name); - EXPECT_TRUE(verify(*reference).succeeded()); + if (GetParam().canonicalizeReference) { + EXPECT_TRUE(runQCOCleanupPipeline(reference.get()).succeeded()); + printer.record(reference.get(), "Canonicalized Reference QCO IR" + name); + EXPECT_TRUE(verify(*reference).succeeded()); + } @@ - QCOTestCase{"TwoR", MQT_NAMED_BUILDER(twoR), - MQT_NAMED_BUILDER(twoRMerged)})); + QCOTestCase{"TwoR", MQT_NAMED_BUILDER(twoR), + MQT_NAMED_BUILDER(twoRMerged), false}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 485 - 486, The TwoR test case at line 485-486 can incorrectly pass if the phi merging logic is broken because Line 91 canonicalizes the reference module, allowing a buggy rewrite to apply incorrectly to both the input and reference modules while still matching. Harden the test by ensuring the reference module (twoRMerged) cannot inadvertently match the input module (twoR) when phi merging is broken. This may require modifying the test data structure or the reference case to create a meaningful divergence that a correct implementation would achieve but a buggy one would not, preventing the self-fulfilling test condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/QCOUtils.h`:
- Line 41: The twoTargetWiresMatch function at line 41 only validates that the
second target wire (wire 1) matches between operations, but it must also
validate the first target wire (wire 0) to properly fulfill the helper's
contract. Modify the return statement to check both op.getOutputQubit(0) ==
nextOp.getInputQubit(0) AND op.getOutputQubit(1) == nextOp.getInputQubit(1),
ensuring both wires match before returning true.
---
Duplicate comments:
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 485-486: The TwoR test case at line 485-486 can incorrectly pass
if the phi merging logic is broken because Line 91 canonicalizes the reference
module, allowing a buggy rewrite to apply incorrectly to both the input and
reference modules while still matching. Harden the test by ensuring the
reference module (twoRMerged) cannot inadvertently match the input module (twoR)
when phi merging is broken. This may require modifying the test data structure
or the reference case to create a meaningful divergence that a correct
implementation would achieve but a buggy one would not, preventing the
self-fulfilling test condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d42f0866-ac49-4cf1-b991-e17401561d6e
📒 Files selected for processing (18)
mlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/lib/Dialect/QCO/IR/Operations/StandardGates/DCXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/POp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/ROp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RXXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SWAPOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXMinusYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp (1)
486-487:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TwoROppositePhaseis currently a self-fulfilling negative test.Because both modules are canonicalized (Line 82 and Line 91), using
twoROppositePhaseas both source and reference means an incorrect merge can be applied to both sides and still pass. Make this case compare canonicalized source against a non-canonicalized reference (or disable reference canonicalization per-case) to actually catch regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp` around lines 486 - 487, The TwoROppositePhase test case uses the same canonicalized twoROppositePhase builder for both the source and reference, making it a self-fulfilling test that cannot catch merge logic regressions. Replace the second parameter (the reference) with either a non-canonicalized version of twoROppositePhase or a different builder that does not undergo canonicalization, so that the test actually validates the merge behavior against an expected non-canonicalized result. This ensures that any incorrect transformations applied during merging will be caught by the comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 486-487: The TwoROppositePhase test case uses the same
canonicalized twoROppositePhase builder for both the source and reference,
making it a self-fulfilling test that cannot catch merge logic regressions.
Replace the second parameter (the reference) with either a non-canonicalized
version of twoROppositePhase or a different builder that does not undergo
canonicalization, so that the test actually validates the merge behavior against
an expected non-canonicalized result. This ensures that any incorrect
transformations applied during merging will be caught by the comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 641c6266-c9d0-450f-91d7-828b19c7e661
📒 Files selected for processing (3)
mlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments! 🙂
Apart from the open CodeRabbit comment, this looks good to me now. I'll leave the final judgment to @burgholzer.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Adds direct handling for symmetric gates, which halves the number of patterns for certain operations. Furthermore, optimizes the implementation itself. Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for another PR @simon1hofmann 🙏🏼
I also edited this one a bit.
In particular, I further improved the existing canonicalization patterns. I really liked the direction here!
On the other hand, I reverted the changes for for the control-wire canonicalization for now. I am not convinced that this is the way to go here. I believe that a more promising way to go about this would be to introduce commutation rules that push diagonal gates through control qubits. I would argue this should not be part of a canonicalization pipeline though. Rather I'd put this into some kind of optimization pass similar to Qiskit's commutation-based optimizer. I would not put this on high priority though.
Since I really believe the other changes are a genuine improvement, I figured this is the quickest way to get this in.
I'll change the title and the PR description a little bit to reflect this.
burgholzer
changed the title
ctrl control chains3 participants
Description
Adds a few merge patterns and consolidates shared logic in
QCOUtils.h.Additional patterns:
For
r: merge whenphimatches (sumtheta).For
xx_plus_yyandxx_minus_yy: sumthetawith swapped target wires.AI Assistance
Used Composer via Cursor for parts of this change. I reviewed the full diff and take responsibility for everything in this PR.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.