-
-
Notifications
You must be signed in to change notification settings - Fork 54
✨ Add Rotation Gate Merging using Quaternions #1407
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
base: main
Are you sure you want to change the base?
Changes from all commits
b566c21
34a9e7c
6749f3c
f8aaa9d
44c3c62
aa8caf4
9f468aa
f75edf8
cfe9eda
1a22b2b
54e612b
c11e856
05d5fdc
22d17ad
1ee703b
fb0d8cb
abdacce
b267779
409f94d
e707dc5
e506cec
7e6299b
40b697d
7198eb3
e7b8da6
e16f12d
b2a7d4b
f889a11
114392b
3bf55ff
212605a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,9 @@ struct QuantumCompilerConfig { | |
|
|
||
| /// Print IR after each stage | ||
| bool printIRAfterAllStages = false; | ||
|
|
||
| /// Enable quaternion-based rotation gate merging | ||
| bool mergeSingleQubitRotationGates = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like this should default to true. There is no real downside of running this, right? We just need to make sure to be careful when this pass is run in relation to gate synthesis in general.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may need a refresher, but doesn't this pass also do stuff like merging One could argue that this IR should be device agnostic, so it shouldn't matter. I'm just not sure if we maybe would then lose some information or potential for reductions when going back from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It all depends on where this pass is being run in the pipeline. For the hardware-agnostic stage, I would argue it should be on by default. We do not have a hardware-dependent level; but this pass should not be run after translating to the native gate-set. |
||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,17 @@ | |
|
|
||
| include "mlir/Pass/PassBase.td" | ||
|
|
||
| def MergeRotationGates : Pass<"merge-rotation-gates", "mlir::ModuleOp"> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changelog talks about a |
||
| let dependentDialects = ["mlir::qco::QCODialect", | ||
| "::mlir::arith::ArithDialect", | ||
| "::mlir::math::MathDialect"]; | ||
| let summary = "Merge rotation gates using quaternion-based fusion"; | ||
| let description = [{ | ||
| Merges consecutive rotation gates of different types (rx, ry, rz, p, r, u2, u) | ||
| using quaternion representation and the Hamilton product. | ||
| }]; | ||
|
Comment on lines
+19
to
+22
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is the documentation that also shows up in the API docs, this should be a bit more extensive and give more details on what is actually going on internally, including references. You can take the |
||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // Transpilation Passes | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
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.
Just noticed this here when being contrasted to the
place-and-routepass below; do we want to drop themlir-prefix for the pass name?Or, to open up another box, would it make sense to define our own prefix that identifies passes being run as part of
mqt-cc? (e.g., by using themqt-cc-prefix)@munich-quantum-toolkit/mqt-cc any opinions on that?
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 would personally drop the
mlir(makemliran identifier for options/concepts taken directly from MLIR). I don't have a super strong opinion on themqt-cc-prefix, except that it's a bit weird to call the program usingOne could say "well of course it's an
mqt-ccoption, I literally use it onmqt-cc.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.
Fair point. Should be safe to just drop then and use no prefix.