Added merge rotation patterns for qml.Rot and qml.CRot #1270
Conversation
| @@ -21,13 +21,16 @@ | |||
| #include "llvm/ADT/StringSet.h" | |||
| #include "llvm/Support/Debug.h" | |||
| #include "llvm/Support/Errc.h" | |||
| #include "mlir/Dialect/Math/IR/Math.h" | |||
| #include "mlir/Dialect/Arith/IR/Arith.h"" | |||
There was a problem hiding this comment.
I think there's an extra quotation mark here, which made all your code into comments below :)
There was a problem hiding this comment.
(Also this file is already included in here no?)
|
@Mohxen Thanks for working on this! I will take a closer look on Monday; for now, let me quickly mention that you can compile your new pass/pattern with |
| # | ||
|
|
||
| # Parameterize test with different angle sets for qml.Rot and qml.CRot to ensure coverage of complex cases. | ||
| @pytest.mark.parametrize("params1, params2", [ |
There was a problem hiding this comment.
So testing this new merge rotation pattern is a bit tricky: we know that regardless of whether the merge rotation transformation took effect or not, the circuit will produce the same results. Given that, does this test here actually test for whether the rotation gates are merged? If not, what is the best way to test that the rotation gates are merged, and what is the purpose of these end-to-end circuit execution tests here?
Hint: search through the code base and look for how the existing merge rotation patterns are tested!
There was a problem hiding this comment.
Thank you @paul0403
I’ve added explicit checks in test_complex_merge_rotation to verify that the merge_rotations transformation reduces the number of rotation gates (Rot and CRot) and preserves the circuit's functionality. By explicitly calling qml.transforms.merge_rotations, we can compare the unoptimized and optimized circuits directly. This allows us to confirm both that the rotation gates are actually merged (fewer gates) and that the results remain the same, addressing the need for both functionality and transformation verification in the test. Please let me know if this method is correct.
There was a problem hiding this comment.
Thanks for adding the test! An additional approach on top of plain functionality check is definitely good to have 💯
| op.replaceAllUsesWith(mergeOp); | ||
|
|
||
| return success(); | ||
| if (opGateName == "qml.Rot" || opGateName == "qml.CRot") { |
There was a problem hiding this comment.
What happens if the rotation gates are adjointed? Should the merge still happen?
(In Catalyst adjointed gates are indicated by a adjoint unit attribute, see for example here
There was a problem hiding this comment.
Thank you @paul0403
Please let me know if you did not consider whether the rotation gates are adjointed for regular merging rotations (not for merging non-commutative rotations), or if we do not have adjointed gates for regular merging rotations?
There was a problem hiding this comment.
The merge rotation pass applies an rotation gate adjoint canonicalization. The canonicalization simply changes all angles to their negative and removes the adjoint attribute. See #1205
However, looking at the canonicalization pattern, you will find that Rot and CRot are not canonicalized.
(a) Why do you think that is?
(b) Knowing this, what do you think you should do in your added pattern here (assuming no new canonicalization is added for (C)Rot)?
There was a problem hiding this comment.
Thank you @paul0403
(a) Rot and CRot are not canonicalized because they involve complex, multi-parameter rotations that cannot be standardized by simply negating a single parameter.
(b) First, check for the adjoint attribute on qml.Rot and qml.CRot. If the operation is adjointed, transform it into its non-adjointed, canonical form by reversing the order of the parameters and negating each parameter. After this transformation, remove the adjoint attribute to standardize the operation. Then, proceed with the merging process as if all rotations are in canonical form, ensuring consistency across operations.
Thus, if I have an operation qml.Rot(π/4, π/2, π/3), its adjoint will be qml.Rot(-π/3, -π/2, -π/4)
Please let me know if my method is correct.
There was a problem hiding this comment.
Good insights! This is what I would do as well.
Due to how the work is organized (aka adjoint canonicalization happens somewhere else, not here in the merge rotation patterns), in the merge rotation patterns, it suffices to assume that the rotation gates will not carry adjoint attributes when the patterns are hit.
Thus the only thing needed here is a check that the (C)Rot gates do not carry adjoint attributes. If they do, the pattern should do nothing.
| // Assuming params[0] = alpha1, params[1] = theta1, params[2] = beta1 | ||
| // and parentParams[0] = alpha2, parentParams[1] = theta2, parentParams[2] = beta2 | ||
|
|
||
| // Step 1: Calculate c1, c2, s1, s2 |
There was a problem hiding this comment.
Thanks for adding the new merge rotation pattern! The formula is very long, so we appreciate the good work 🥳
There was a problem hiding this comment.
Thank you so much for your help :) If there's anything specific you'd like me to refine or expand on, please let me know.
Before submitting
Please complete the following checklist when submitting a PR:
All new functions and code must be clearly commented and documented.
Ensure that code is properly formatted by running
make format.The latest version of black and
clang-format-14are used in CI/CD to check formatting.All new features must include a unit test.
Integration and frontend tests should be added to
frontend/test,Quantum dialect and MLIR tests should be added to
mlir/test, andRuntime tests should be added to
runtime/tests.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context:
In quantum circuits, consecutive rotation gates about the same axis on the same qubit can often be combined into a single gate. For instance, two successive Rx gates with angles a and b can be merged into a single Rx(a + b) gate. While this merging is straightforward for single-axis rotations, it becomes more complex for composite rotations like
qml.Rotandqml.CRot, which involve rotations around multiple axes. Previously, an attempt to includeqml.Rotandqml.CRotin Catalyst's merge rotation pass was reverted due to the non-commutative nature of these rotations. However, by applying the appropriate full-angle formulas derived from Euler angles, these gates can be accurately merged.The scalar formula for combining these rotations is derived from the [PennyLane Single Qubit Fusion documentation (https://docs.pennylane.ai/en/stable/code/api/pennylane.transforms.single_qubit_fusion.html#derivation), which describes the mathematical approach for fusing consecutive rotation operations.
Description of the Change:
This update reintroduces
qml.Rotandqml.CRotinto Catalyst's merge rotation pass by implementing the correct mathematical formulas for combining their angles.Added
qml.Rotandqml.CRotto Catalyst’s MLIR rotation merging pass.Implemented the necessary trigonometric calculations to accurately determine the combined rotation angles when merging these gates, using
Arith.handMath.hfrom MLIR for operations such as cosine, sine, and addition.test_peephole_optimizations.pyto include tests for mergingqml.Rotandqml.CRotgates.Benefits:
qml.Rotorqml.CRotgates into a single operation, optimizing circuit depth and potentially reducing execution time.Possible Drawbacks:
The trigonometric calculations required to merge
qml.Rotandqml.CRotgates are more complex than simple angle addition, which may introduce additional computational overhead during the compilation process.Related GitHub Issues:
qml.Rotandqml.CRotfrom the merge rotation pass due to incorrect angle merging.qml.Rotandqml.CRotinto the merge rotation pass with correct angle calculations.