-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added loop boundary optimization #1476
base: main
Are you sure you want to change the base?
Conversation
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: paul0403 <[email protected]>
Co-authored-by: paul0403 <[email protected]>
Co-authored-by: paul0403 <[email protected]>
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
Improve the code dynamic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far! I left a few comments regarding the foundations of the pass, but since it's still a work in progress I didn't look at the code too closely yet :)
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Quantum/Transforms/LoopBoundaryOptimizationPatterns.cpp
Outdated
Show resolved
Hide resolved
Remember to link to the shortcut story! You can see how this is done on other PRs :D |
Of course, will do. Thanks Paul! |
Integrate with merge rotation and cancel inverse Improve code dynamic
Add new tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @sengthai, this is looking really good 💯
A couple of questions I still have:
doc/releases/changelog-dev.md
Outdated
return qml.expval(qml.Z(0)) | ||
``` | ||
|
||
Note that this pass works with perfectly matching operations such as Pauli gates, Hadamard gate, CNOT and rotations gates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by perfectly matching operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean operations that cancel each other out exactly when applied in sequence, like Hadamard followed by Hadamard. I'll update the documentation to be more explicit about this.
RewritePatternSet patternsCanonicalization(&getContext()); | ||
scf::ForOp::getCanonicalizationPatterns(patternsCanonicalization, &getContext()); | ||
|
||
if (failed(applyPatternsAndFoldGreedily(module, std::move(patternsCanonicalization)))) { | ||
return signalPassFailure(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious as to why you are running the for loop patterns here?
There is also the option to combine the patterns from the for loop and the loop boundary and run them together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right! If we were only using ForOp canonicalization patterns thinking they would help filter/dispatch ForOps to our pass, that's not necessary. Our LoopBoundaryPattern already specifically matches and handles ForOps.
populateLoopBoundaryPatterns(patternsLoopBoundary); | ||
|
||
if (failed(applyPatternsAndFoldGreedily(module, std::move(patternsLoopBoundary)))) { | ||
return signalPassFailure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this apply adjoint cancellation as well even though we are in the merge rotation pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's intentional. Loop boundary optimization can create more opportunities for rotation merging. However, I will combine merge rotation and and loop boundary instead of applying pattern individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is the intention product had though. Applying the rotation merging pass should simply merge rotations (now including across boundaries), not cancel inverses. Interaction between those two passes can be benefitted from by introducing a better pass that combines multiple quantum optimization patterns, but that's out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got the point. And yes, the loop boundary pass here takes action on both Rotation and Hermitian gates no matter where we called it. Example:
For _ in range(3)
X -> Q0
Rx(0.1) -> Q1
CNOT -> Q0, Q1
Rx(0.3) -> Q1
X -> Q0
In here even we just apply merge_rotation
, the pass will consider to optimize on X gate as well.
I agree with you that current merge_rotation
with loop boundary do more than what it supposed to do.
Let's me check if we can control the loop boundary behavior when applying with merge_rotations
and cancel_inverse
// CHECK: [[reg:%.+]] = quantum.alloc( 1) : !quantum.reg | ||
// CHECK: [[qubit_0:%.+]] = quantum.extract [[reg]][ 0] : !quantum.reg -> !quantum.bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test checks anything meaningful, the added complexity comes into play when the register is a loop iter_arg no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! test_loop_boundary_register_2
is indeed a better test case. This simpler one can stay for basic verification, but we should focus on more complex register iter_arg cases.
Updated changelog Remove unnecessary codes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1476 +/- ##
==========================================
+ Coverage 90.90% 95.90% +4.99%
==========================================
Files 21 80 +59
Lines 1485 8319 +6834
Branches 0 779 +779
==========================================
+ Hits 1350 7978 +6628
- Misses 135 286 +151
- Partials 0 55 +55 ☔ View full report in Codecov by Sentry. |
Context:
The Loop Boundary Optimization identifies and optimizes redundant quantum operations that occur at loop iteration boundaries, where operations at iteration boundaries often cancel each other out.
This method is standalone pass but also integrated in merge rotation and cancel inverses pass.
It specifically targets:
Description of the Change:
merge_rotation
andcancel_inverses
passBenefits:
Limited Scope
Possible Drawbacks:
[sc-83037]