✨ Implement single-qubit gate decomposition pass #1182
Conversation
taminob
added
enhancement
taminob
force-pushed
the
taminob/mlir-gate-decomposition-pass
branch
2 times, most recently
from
ee89a21 to
38f10fe
Compare
Cpp-Linter Report
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@burgholzer if you have some time to spare, feel free to have a look at the implementation. The results of the rotation test case were verified using and More test cases are probably desirable, do you have any suggestions for what to test? Also, floating-point imprecisions could make it necessary to reduce the precision of the CHECK statements. |
taminob
force-pushed
the
taminob/mlir-gate-decomposition-pass
branch
from
65f913b to
7fddcb5
Compare
There was a problem hiding this comment.
Many thanks @taminob for this initial draft!
As I already alluded to previously, I have quite some comments on this PR and how it is currently set up.
As these are quite fundamental, I'd propose to try to split this up into a sequence of smaller PRs. I'd also propose to get the swap reconstruction and elision PR merged first in order to not have to many open PRs that try to add various things. After that, the best step is probably to extend the unitary interface with the necessary methods and implementations to facilitate the implementation here in a series of small but dedicated PRs. This will entail an investigation for how to best deal with linear algebra in MLIR (as this will be necessary for properly defining the unitaries of gates). Let's take it from there.
| let summary = "This pass will perform various gate decompositions to simplify the used gate set."; | ||
| let description = [{ | ||
| }]; |
There was a problem hiding this comment.
| let summary = "This pass will perform various gate decompositions to simplify the used gate set."; | |
| let description = [{ | |
| }]; | |
| let summary = "This pass performs various gate decompositions to translate quantum gates being used."; | |
| let description = [{ | |
| }]; |
also needs a full description.
| /** | ||
| * @brief This pass attempts to cancel consecutive self-inverse operations. | ||
| */ |
There was a problem hiding this comment.
I'd argue that headers should be put into the include directory.
But even more so, I am not a big fan of "helpers" or "utils" files. They tend to become a huge pile of unrelated functions that would be better suited elsewhere.
Most of the functions here should most likely be part of the unitary interface.
More precisely:
- checking whether something is a single-qubit operation
- getting the unitary belonging to an operation implementing the unitary interface.
- getting the kind of operation (although this is kind of already possible by simply matching against the actual type)
Further comments on the rest to follow.
There was a problem hiding this comment.
For headers, I guess it's a personal preference that "private" headers (only used in source files) are also in src - but I'll remove this file anyway to incorporate it into the file of the gate decomposition pattern or the unitary interface.
I already had a look at how to incorporate the matrix definitions into the unitary interface, but didn't find a satisfying solution to incorporate it without repeating the definitions (which as you said is the way to go since there should be no dependency to the DD target) and also how to deal with the fact that not all operations have a trivial 4x4 or 8x8 matrix. I'll have a look at the definition property of Qiskit that you mentioned below.
There was a problem hiding this comment.
I agree that private headers could be placed in the src tree. However, we haven't needed such headers in basically all of the MQT so far. Probably due to the fact that we typically follow a pretty object oriented design where these free functions would be private methods in the respective classes.
As for the unitary definitions:
Duplication is unavoidable and actually to some degree intended here. The fact that these unitary definitions are not part of the MQT CoreIR library is a pure consequence of history. Ideally these definitions would be part of the core IR from the start; which is essentially what we want to do here.
Small corrections: single qubit gate matrices are 22 and two-qubit gate matrices are 44.
For the static gates without parameters, this should be extremely straightforward. For the parameterized gates, it is no longer as straight forward, but should still be doable.
Any parameterized operation has mlir::Values for its parameters (or static attributes). These are the inputs to the unitary getter. Gates without parameters simply use the empty variadic list. If all the parameters are either static attributes or values that are constants, then the result is a fully specified complex-valued unitary. If any of the parameters is not a compile-time constant, the resulting matrix remains parametrized by the respective value.
Decompositions might not be applicable there or might need to be deferred to runtime.
Generally, I believe the tensor dialect of MLIR together with the linalg one seems like a very natural fit for representing this.
One could "simply" convert from the (single-qubit) unitary gates to corresponding tensors, runs of those could be contracted, the result could be converted back to a unitary operation according to the given decomposition.
Just some thoughts..
There was a problem hiding this comment.
As for the unitary definitions: Duplication is unavoidable and actually to some degree intended here. The fact that these unitary definitions are not part of the MQT CoreIR library is a pure consequence of history. Ideally these definitions would be part of the core IR from the start; which is essentially what we want to do here.
I think I might have misunderstood you before. So you want to have the matrix definitions in CoreIR & CoreDD and not in MLIR & CoreDD, is that correct? Since you mentioned before that you also don't like to have a dependency to CoreIR, I expected to implement this in the MLIR library.
Small corrections: single qubit gate matrices are 2_2 and two-qubit gate matrices are 4_4.
My bad, of course - thanks for the correction.
For the static gates without parameters, this should be extremely straightforward. For the parameterized gates, it is no longer as straight forward, but should still be doable. Any parameterized operation has mlir::Values for its parameters (or static attributes). These are the inputs to the unitary getter. Gates without parameters simply use the empty variadic list. If all the parameters are either static attributes or values that are constants, then the result is a fully specified complex-valued unitary. If any of the parameters is not a compile-time constant, the resulting matrix remains parametrized by the respective value. Decompositions might not be applicable there or might need to be deferred to runtime.
If you look at
, I'm not sure if it is actually that straight-forward. I didn't find any elegant way to actually execute MLIR operations and we can't just assumeConstantOps because in e.g. merge-rotation-gates we also introduce e.g. AddFOps.
Generally, I believe the tensor dialect of MLIR together with the linalg one seems like a very natural fit for representing this. One could "simply" convert from the (single-qubit) unitary gates to corresponding tensors, runs of those could be contracted, the result could be converted back to a unitary operation according to the given decomposition.
Especially for the datatypes, matrix multiplications and kronecker products (necessary for 2-qubit series), this is a good idea. I'll look into it.
There was a problem hiding this comment.
Sorry for not being clear here (at a big event and only on the phone between sessions).
I definitely meant to have the definitions in MLIR. That is "the place to be" for newly added functionality at the moment.
As for the merge rotation pass introducing floating point operations: a constant propagation and folding pass should eliminates those operations. We have the first precedent of running existing MLIR passes as part of testing (see the latest IfElse PR that was merged a couple of days back). This already shows how to run passes on modules, which could prove useful here.
Similarly, conversions to linalg and then running transformation passes on that before converting back to unitary operations could be one way to get to our goal.
I figured from the very start that this feature will not be as easy to cleanly and idiomatically port over from existing software to MLIR.
However, all of these learnings are well worth writing down in some kind of formal document, so I believe it is truly worth to find the "right" way to do this.
|
|
||
| get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS) | ||
| set(LIBRARIES ${dialect_libs} MQT::CoreIR) | ||
| set(LIBRARIES ${dialect_libs} MQT::CoreIR MQT::CoreDD) |
There was a problem hiding this comment.
We should generally avoid to add further dependencies to existing MQT Core libraries if at all possible. On the contrary, we'd rather like to replace the MQT::CoreIR dependency in the other libraries with the MLIR one.
The necessary functionality here should boil down to getting the unitary of an operation implementing the unitary interface.
This is something that should be directly built into the interface and its implementation. Otherwise, we will never get a clear separation of concerns.
At the moment, I see no need to refactor the DD code to rely on the MLIR representation.
In the context of providing the unitary for a gate, it might make sense to, additionally, adopt something fairly similar to OpenQASM's and Qiskit's definition property, which essentially describes how to build the gate from known, already defined gates (starting with just the global phase gate and the control, inverse, and power modifiers). This might be beyond the scope of this PR though.
There was a problem hiding this comment.
I understood that when you said that we already have the definitions that we want to re-use them. Good to know that you want to keep it separate 👍
| return mlir::failure(); | ||
| } | ||
|
|
||
| dd::GateMatrix unitaryMatrix = dd::opToSingleQubitGateMatrix(qc::I); |
There was a problem hiding this comment.
We should not rely on the dd::GateMatrix representation. MLIR/LLVM should have builtin support for working with matrices (or tensors) and their manipulations.
Maybe https://mlir.llvm.org/docs/Dialects/Linalg/ is a good first stop or https://www.stephendiehl.com/posts/mlir_linear_algebra/.
Generally, I would argue that we should be taking advantage of MLIR's infrastructure as much as possible.
There was a problem hiding this comment.
Thanks for the pointer, I'll look into it.
| auto series = getSingleQubitSeries(op); | ||
| if (series.size() <= 3) { | ||
| // TODO: find better way to prevent endless optimization loop | ||
| return mlir::failure(); | ||
| } |
There was a problem hiding this comment.
This is definitely not ideal as a termination criterion, as there may still be better decompositions, e.g., for a sequence of two gates.
I think it pays off to take some inspiration from https://github.com/Qiskit/qiskit/blob/stable/2.1/crates/synthesis/src/euler_one_qubit_decomposer.rs here.
There was a problem hiding this comment.
I'm not entirely sure what you mean here, the file does not contain the creation/detection of the gate series, I think that's in https://github.com/Qiskit/qiskit/blob/stable/2.1/crates/transpiler/src/passes/unitary_synthesis.rs. I can have a look again if I can find their abort criterion.
A naive approach I thought of was alternatively checking if the gate series only consists of Z/Y (so the used rotation gates for the decomposition). Does not catch everything, but at least better than this.
There was a problem hiding this comment.
Yeah, I meant to imply following the import chain for the one qubit gate sequence struct. 🙂
I wouldn't tie the abortion criterion to a specific decomposition.
| * indicating that they have been altered from the originals. | ||
| */ | ||
| [[nodiscard]] static std::array<qc::fp, 4> | ||
| paramsZyzInner(dd::GateMatrix unitaryMatrix) { |
There was a problem hiding this comment.
I believe it would be quite useful to not limit this gate decomposition to ZYZ, but to also include further bases. Different quantum computers provide different gate sets, which is why the different decompositions are almost a requirement.
- the general
circuit_kakmethod in Qiskit already provides a very solid basis for ZYZ, ZXZ, XZX, XYX - beyond that one of the decompositions including the SX gate would be interesting (native to IBM systems).
PSX
While it is not extremely important for this PR here itself, it is fairly important for setting up the respective infrastructure to expand it in the future.
Similar to Qiskit, the type of decomposition should be a parameter of the pattern (and subsequently of the pass). Or just of the pass and depending on that parameter, the respective pattern is added to the pipeline (this could make more sense).
There was a problem hiding this comment.
I think that it is a good idea to add the other gates for the KAK decomposition to this pattern, but don't think it's a good idea to stuff every decomposition into this pattern. But you already said that it is probably out-of-scope for this PR anyway and it can be easily extended by adding additional patterns.
There was a problem hiding this comment.
Or just of the pass and depending on that parameter, the respective pattern is added to the pipeline (this could make more sense).
I believe the above is what makes sense for this PR. The available patterns in the beginning may only contain the different KAK decompositions, but the architecture should be flexible enough so that future PRs could easily add more patterns to the pass.
|
I'll try to summarize here the next steps for this PR - each step is its own PR in my opinion:
@burgholzer did I miss anything or do you have something else in mind for one of these steps? |
Couple of comments in addition to what you already nicely summarised (thanks for that!)
A lot of the inspiration for how to do this might come from the way Catalyst is handling this:
They essentially define something they refer to as the Just browsing through the above sources, this entails concepts like:
But, naturally, our driver would be much simpler as we don't (yet) support as diverse of a set of dialects and programs as Catalyst does.
I believe these two points might actually be combined, because they are fairly related to another.
I agree that it is probably overkill to use As for the question of templates vs. runtime specification: I believe the actual decomposition to be used as part of the CompilerDriver has to be one of the I hope this provides you with enough context to proceed. Don't hesitate to ask further questions as they pop up! |
2 participants
Description
In order to build a compiler pipeline, we need different passes which can also be applied after the routing.
One such pass is this which will take a series of single qubit gates and transform them into one to three rotation gates.
Related to #1122
Checklist: