-
Notifications
You must be signed in to change notification settings - Fork 35
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
[MLIR] Provide convenience gate builders in tablegen #1180
base: main
Are you sure you want to change the base?
Conversation
…erations) in tablegen
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1180 +/- ##
=======================================
Coverage 97.86% 97.86%
=======================================
Files 76 76
Lines 10819 10819
Branches 1283 1283
=======================================
Hits 10588 10588
Misses 179 179
Partials 52 52 ☔ View full report in Codecov by Sentry. |
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.
Thanks for adding this!
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.
We should give the same treatment to our other quantum gates :) (anything with control wires)
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 adding them.
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.
Done.
I am basing the category on whether the gate (a) has controls and (b) has parameters. This means there are 4 buidlers.
I think these sufficiently covers all the needs as of now.
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 we really want tests in the source directory 🤔
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.
The problem is, is there even another way to do mlir lit tests? The test directory is just a bunch of IR files.
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 your asking for is C++ unit tests, unfortunately we don't have a framework setup for it yet, although Erick implemented one here: #1105 (just needs review)
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.
Wow, nice!
builder.create<quantum::CustomOp>(loc, "SWAP", mlir::ValueRange({inQubit, pz->getResult(0)})); | ||
|
||
builder.create<quantum::CustomOp>(loc, "Rot", | ||
mlir::ValueRange({inQubit, pz->getResult(0)}), |
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.
Since all the arguments are wrapped in ValueRange()
, thoughts about letting the helpers just take in a std::vector
(or some other containers that can be implicitly passed via an initialization list), and converting them to ValueRange
insider the helper?
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.
e.g.
void f(std::vector<int> v){
for (int x : v){
std::cout << x;
}
}
int main() {
f({10, 21, 44});
return 0;
}
------
102144
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.
We should avoid using vectors unless necessary. The mlir devs use specific data structures (like the ValueRange) in as many places as possible to avoid allocations, copies, etc.
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 pretty sure you can pass a vector to a function expecting ValueRange though.
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.
This mlir::ValueRange({inQubit, pz->getResult(0)})
is actually extremely dangerous though in a function call, because ValueRange is a non-owning object, your temporaries don't have a storage location for duration of the function call.
Context:
There's many scheduled circuit transformation work at the mlir layer, like merge rotations and control flow related transforms, or even hardware gateset decomposition, where creating new gates will be necessary.
However, it came to our realization that currently the way to create a new gate in mlir (aka a
quantum.custom
operation) is more complicated than necessary, since all the fields need to be specified, even when the field values are empty. For example, see https://github.com/PennyLaneAI/catalyst/pull/1154/files#diff-bc5ebab69d5c90919840a29058595f8718f5e8a150e141f721250b76a0278f44R49In light of this, we provide convenience builders in tablegen.
Description of the Change:
Provide convenience gate builders (aka builders for quantum.custom operations) in tablegen;
Create a tester pass
-test-gate-buidler
.Benefits:
No need to specify a bunch of unnecessary boilerplate arguments when creating a new
quantum.custom
operation in a pass.This can also be greatly beneficial for external contributors.
Possible Drawbacks:
Related GitHub Issues: