Skip to content

Conversation

@taminob
Copy link
Collaborator

@taminob taminob commented Sep 8, 2025

Description

This PR introduces the MLIR declarative language for pattern matching and rewriting into the existing build system.
As an example, the ElidePermutations pattern will be replaced by a PDLL implementation and extended by the functionality of #1158 (with the exclusion of multi-controlled swaps).

This has previously been discussed in #1158.

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 8, 2025
@taminob taminob force-pushed the taminob/mlir-introduce-pdll-for-patterns branch from 8d3e060 to 3343485 Compare September 8, 2025 10:57
@burgholzer burgholzer added enhancement New feature or request refactor Anything related to code refactoring c++ Anything related to C++ code MLIR Anything related to MLIR labels Sep 8, 2025
@burgholzer burgholzer added this to the MLIR Support milestone Sep 8, 2025
@burgholzer
Copy link
Member

Pre-commit won't be hapy here until you add a line for PDLL to

".mlir": "SLASH_STYLE",
".td": "SLASH_STYLE",

@taminob
Copy link
Collaborator Author

taminob commented Sep 8, 2025

@burgholzer this is a first draft and I'd like to discuss with you maybe how to best integrate this new kind of pattern definition. Especially regarding directory structure since it might make sense to add sub-directories passes and patterns. Also, do we want to extract the "single pattern passes" which probably are only necessary for testing?

The code generation generates this here:

namespace {

struct GeneratedPDLLPattern0 : ::mlir::PDLPatternModule {
  template <typename... ConfigsT>
  GeneratedPDLLPattern0(::mlir::MLIRContext *context, ConfigsT &&...configs)
    : ::mlir::PDLPatternModule(::mlir::parseSourceString<::mlir::ModuleOp>(
R"mlir(pdl.pattern : benefit(0) {
  %0 = operand loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":3:30)
  %1 = operand loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":3:43)
  %2 = types loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":3:14)
  %3 = operation "mqtopt.swap"(%0, %1 : !pdl.value, !pdl.value)  -> (%2 : !pdl.range<type>) loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":3:14)
  rewrite %3 {
    replace %3 with(%1, %0 : !pdl.value, !pdl.value) loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":6:3)
  } loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":6:3)
} loc("<path_to_repo>/core/mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.pdll":1:1)
    )mlir", context), std::forward<ConfigsT>(configs)...) {
  }
};

} // end namespace

template <typename... ConfigsT>
static void LLVM_ATTRIBUTE_UNUSED populateGeneratedPDLLPatterns(::mlir::RewritePatternSet &patterns, ConfigsT &&...configs) {
  patterns.add<GeneratedPDLLPattern0>(patterns.getContext(), configs...);
}

Unfortunately, the populateGeneratedPDLLPatterns name is hardcoded and adding namespaces are also hard-coded (see https://mlir.llvm.org/doxygen/CPPGen_8cpp_source.html).

So, we basically need to put each pattern group into a separate file and putting a pattern in multiple passes could become a little bit more complicated (we need to e.g. wrap the #include in their own namespace to avoid violoations of the ODR).
This could also become a little bit cumbersome regarding modifying the CMakeLists.txt each time. Could probably be automated by a glob for *.pdll.

@taminob taminob force-pushed the taminob/mlir-introduce-pdll-for-patterns branch from fab95ec to a95d061 Compare September 8, 2025 11:12
@burgholzer
Copy link
Member

Let me play around with this a little. I'll get back to you 😌

@taminob taminob marked this pull request as ready for review September 8, 2025 11:15
@codecov
Copy link

codecov bot commented Sep 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2025

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v21.1.1) reports: 1 concern(s)
  • mlir/lib/Dialect/MQTOpt/Transforms/SwapReconstructionAndElision.cpp:13:1: warning: [misc-include-cleaner]

    included header PDLInterp.h is not used directly

       13 | #include <mlir/Dialect/PDLInterp/IR/PDLInterp.h>
          | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       14 | #include <mlir/IR/PatternMatch.h>

Have any feedback or feature suggestions? Share it here.

@burgholzer
Copy link
Member

Alright. I played around with this a little and made a couple of adjustments.
Unfortunately, the end result is rather frustrating: I haven't found a way to make the pattern itself work properly because I found no way to either constrain or index the variadic lists of qubits through PDLL.
Maybe you could experiment just a little bit further if this is indeed not possible or if I was just missing something.

If it turns out that this does indeed not work even for such a simple use case, I believe we should do the following:

  • document this experiment in the MLIR Docs as a separate section (similar to the newly added classical results semantics section), clearly stating that we tried and what the resulting roadblocks where. Given how PDLL is supposed to be an improvement over the older tablegen based format for declaring patterns, we can also rule that one out and, hence, will definitely write future patterns in C++ with the respective boilerplate code.
  • maybe keep ff7114d ; I have a feeling this could be helpful.
  • continue with the simplification of ✨ Implement (controlled) swap reconstruction MLIR pass #1158

@taminob
Copy link
Collaborator Author

taminob commented Sep 8, 2025

@burgholzer thanks for looking into it and working on this! I continued playing around with it and yes, it is frustrating - especially because the documentation is a little bit sparse at times.

However, I wouldn't call it a failure because your issue can be simply fixed by adding a native rewrite embedded in PDLL with: [{ /* C++ code */ }]
I'll push the current state of my experiments in a moment.

Edit: it is also not fully working yet and I still think there is an easier way to build a new mqtopt operation without the native rewrite thing. Unfortunately I couldn't test the rebased version because I'll need to re-compile LLVM because I'm currently using 20.1.8. :)

@burgholzer
Copy link
Member

@burgholzer thanks for looking into it and working on this! I continued playing around with it and yes, it is frustrating - especially because the documentation is a little bit sparse at times.

However, I wouldn't call it a failure because your issue can be simply fixed by adding a native rewrite embedded in PDLL with: [{ /* C++ code */ }] I'll push the current state of my experiments in a moment.

Edit: it is also not fully working yet and I still think there is an easier way to build a new mqtopt operation without the native rewrite thing. Unfortunately I couldn't test the rebased version because I'll need to re-compile LLVM because I'm currently using 20.1.8. :)

Unfortunately, that can't be working because statements like let root = op<mqtopt.x>(target: Value, control: Value); will never compile as there is no such constructor/builder for the X gate. Maybe there still is a way to make this work with native code, but then again, there is no real advantage of using PDLL.

Hint: if you do not want to rebuild from scratch you can overwrite the minimum required MLIR version via CMake. Just pass -DMQT_MLIR_MIN_VERSION=20.0 to the configure and you should be good (for now). Which OS are you on by the way?

@taminob
Copy link
Collaborator Author

taminob commented Sep 9, 2025

Unfortunately, that can't be working because statements like let root = op<mqtopt.x>(target: Value, control: Value); will never compile as there is no such constructor/builder for the X gate. Maybe there still is a way to make this work with native code, but then again, there is no real advantage of using PDLL.

It really bothered me that this didn't work at all and so I spent some more time yesterday and today on making it work - you can have a look at the current version, it is not the prettiest, but works.
Do you have any thoughts on C++ vs PDLL when we can make it work?

I think the inline native code integration is pretty neat and works reasonably well (although I got a lot of issues with Value lifetimes and went down the rabbit hole of debugging the LLVM code... Compilation+Installation takes ~110GiB of disk space 😖 ). However, it is another language you need to have a look into and the documentation isn't as good as the C++ API yet.
For the future if anyone looks at this PR: inline native code constraints cannot wrap values in a LogicalResult as of now in PDLL. :( There was one small comment in the LLVM code and I already thought I found a bug there.

Hint: if you do not want to rebuild from scratch you can overwrite the minimum required MLIR version via CMake. Just pass -DMQT_MLIR_MIN_VERSION=20.0 to the configure and you should be good (for now). Which OS are you on by the way?

Thanks, I re-compiled it anyway in debug mode (although I'll re-compile it again in release mode - linking times go through the roof in debug mode). I'm on Linux 🐧

@burgholzer
Copy link
Member

Ah. Nice. I just took a quick look and it does not look too bad complexit-wise. Will have to take a deeper look later or tomorrow to better judge the pro's and con's.

If you are on Linux and you want a release build, you can get this pretty much for free by following the steps in our CI; see

sudo apt-get update
TMP_SH="${RUNNER_TEMP:-/tmp}/llvm_install.sh"
download https://apt.llvm.org/llvm.sh "$TMP_SH"
chmod +x "$TMP_SH"
if sudo "$TMP_SH" "$LLVM_MAJOR"; then
sudo apt-get install -y \
libmlir-"$LLVM_MAJOR"-dev \
llvm-"$LLVM_MAJOR"-tools \
mlir-"$LLVM_MAJOR"-tools \
clang-"$LLVM_MAJOR" \
clang-tools-"$LLVM_MAJOR" || { echo "apt-get install failed"; exit 1; }
else
echo "Installation from apt.llvm.org script failed." >&2
exit 1
fi
append_env "CC=clang-$LLVM_MAJOR"
append_env "CXX=clang++-$LLVM_MAJOR"
append_env "MLIR_DIR=/usr/lib/llvm-$LLVM_MAJOR/lib/cmake/mlir"
append_env "LLVM_DIR=/usr/lib/llvm-$LLVM_MAJOR/lib/cmake/llvm"

@burgholzer
Copy link
Member

I played around with this a little more now.
My personal conclusion at the moment: PDLL is quite cumbersome to use. At least considering the way we currently define our operations.
The value ranges make it fairly hard to write elegant code.
Furthermore, there is no syntax highlighting or LSP support out of the box for most IDEs.
Considering all of that, I don't believe PDLL is the way to go for the moment.
It was nice that we tried, and it's good to know that it exists, but I haven't felt that it considerably reduces complexity. Even in cases where I would have expected this to be the case.

@taminob if you agree with the above, would you mind continuing/resuming the work in #1158 and streamlining it based on all the lessons lernt in here.
It could be worth to cherry pick ff7114d into that PR.

This is meant to better showcase the capabilities and different styles
that are possible in PDLL.
@taminob
Copy link
Collaborator Author

taminob commented Sep 16, 2025

This PR has been superseded by #1207 which re-implements the functionality in C++.

I reverted some of your changes since I think that it can still be used as a reference for how to use PDLL and the different variants serve as a showcase.

@taminob taminob closed this Sep 16, 2025
@burgholzer
Copy link
Member

I reverted some of your changes since I think that it can still be used as a reference for how to use PDLL and the different variants serve as a showcase.

Technically, there would not have been a need for that as the branch contains all the commits and history anyway. Still fine though; just couldn't resist to comment. :o)

@taminob
Copy link
Collaborator Author

taminob commented Sep 16, 2025

Technically, there would not have been a need for that as the branch contains all the commits and history anyway.

But if someone looks at this in 2 years, I am pretty sure they won't look at the content of each commit (speaking from my own experience). Probably already unlikely anyone will look at this at all. :)

Still fine though; just couldn't resist to comment. :o)

Me too :P

burgholzer added a commit that referenced this pull request Sep 18, 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:

<!---
This checklist serves as a reminder of a couple of things that ensure
your pull request will be merged swiftly.
-->

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

---------

Signed-off-by: burgholzer <[email protected]>
Signed-off-by: Lukas Burgholzer <[email protected]>
Co-authored-by: burgholzer <[email protected]>
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 refactor Anything related to code refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants