Skip to content

Conversation

@taminob
Copy link
Collaborator

@taminob taminob commented Sep 16, 2025

Description

This PR introduces a new pattern and pass for swap reconstruction from two consecutive reversed CNOT gates and its immediate elision.
It replaces #1158 which implemented a more advanced, but also much more complex, variant of the pattern which can also re-construct controlled SWAP gates and replace full three CNOT patterns equivalent to a SWAP.
It is equivalent in functionality to #1194 which implemented the same pattern in PDLL.

Related to #1122

Checklist:

  • The pull request only contains commits that are focused and relevant to this change.
  • I have added appropriate tests that cover the new/changed functionality.
  • I have updated the documentation to reflect these changes.
  • I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
  • I have added migration instructions to the upgrade guide (if needed).
  • The changes follow the project's style guidelines and introduce no new warnings.
  • The changes are fully tested and pass the CI checks.
  • I have reviewed my own code changes.

@taminob taminob self-assigned this Sep 16, 2025
@taminob taminob added c++ Anything related to C++ code enhancement New feature or request MLIR Anything related to MLIR labels Sep 16, 2025
@taminob taminob requested a review from burgholzer September 16, 2025 15:38
@taminob taminob marked this pull request as ready for review September 16, 2025 15:38
@taminob taminob force-pushed the taminob/mlir-simplified-swap-reconstruction-pass branch from 5010024 to cf2463f Compare September 16, 2025 15:39
@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @taminob for consolidating this! This looks really great already (no surprise after quite some iteration in the other PRs).
I still have a couple of comments which you will find inline.

Besides that, it would obviously be great to get the CI green by fixing all the open linter warnings.

Finally, as per the PR template, would you please add a changelog entry for this PR.

@taminob taminob force-pushed the taminob/mlir-simplified-swap-reconstruction-pass branch from cf2463f to 47940e3 Compare September 17, 2025 13:37
@taminob
Copy link
Collaborator Author

taminob commented Sep 18, 2025

@burgholzer I added the three-CNOT matching logic again, feel free to have another look (no need to hold back on comments :) ). I looked a little bit into the greedy driver logic and also found some interesting paragraphs in its documentation.

If you're interested, this paragraph is about the with recursion (I verified in the PDLL parser sources that it's exactly the same). However, setHasBoundedRewriteRecursion(false) does not cause the recursion test cases to fail - I guess that the recursion detection has a way higher limit - do you think it is worth it to add a test case with accordingly many CNOTs? I'll need to do some digging on how many are needed.

The reason why the benefit didn't work before was that it was actually doing a bottom-up traversal for the operations and thus the two CNOT operation will be matched first of course. Fortunately, the greedy pattern rewrite driver does have some configuration options which are useful. For more complex passes, we might want to actually look into multiple applyPatternsGreedily loops since using GreedyRewriteStrictness::ExistingOps could prevent the loop in #1182, but that's for the future and more complex passes (config.enableConstantCSE(true)config.enableFolding(true) also simplifies the constant issues we had over there).

@taminob taminob force-pushed the taminob/mlir-simplified-swap-reconstruction-pass branch from e946c68 to aeedaf4 Compare September 18, 2025 10:46
@taminob taminob force-pushed the taminob/mlir-simplified-swap-reconstruction-pass branch from 54ead71 to b42750d Compare September 18, 2025 11:01
…pass

Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
@burgholzer burgholzer added this to the MLIR Support milestone Sep 18, 2025
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work on this @taminob 🙏🏼
I played around with this a little more, tried a couple of things, and improved some of the code slightly.
This should do it for now. If CI is green, let's get this in!
Another PR with quite some lengthy discussion merged; I'll count that as a win ✨

@burgholzer burgholzer enabled auto-merge (squash) September 18, 2025 19:38
@burgholzer burgholzer merged commit cd55179 into main Sep 18, 2025
36 checks passed
@burgholzer burgholzer deleted the taminob/mlir-simplified-swap-reconstruction-pass branch September 18, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Anything related to C++ code enhancement New feature or request MLIR Anything related to MLIR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants