-
-
Notifications
You must be signed in to change notification settings - Fork 63
✨ Add Dead Gate Elimination Pattern #1755
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 8 commits
91b3ef9
ba57aa2
b83afab
4243ac0
a50032b
d8c9ad5
9717547
2d65418
278a54e
44eaf7e
8b3af43
6998110
d56ccaf
fb9fffb
44f295a
9e8e711
b6cbf8e
dee0752
244f8bb
6eaac40
07a3f4d
cab4155
6f1c64e
7e63992
2e9f4e6
29b0cbe
225c88f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,14 +11,20 @@ | |
| #include "mlir/Dialect/QCO/IR/QCOOps.h" | ||
|
|
||
| #include "mlir/Dialect/QCO/IR/QCODialect.h" // IWYU pragma: associated | ||
| #include "mlir/Dialect/QCO/IR/QCOInterfaces.h" | ||
| #include "mlir/Dialect/QCO/QCOUtils.h" | ||
|
|
||
| #include <llvm/ADT/STLExtras.h> | ||
| #include <mlir/IR/Block.h> | ||
| #include <mlir/IR/MLIRContext.h> | ||
| #include <mlir/IR/OpImplementation.h> | ||
| #include <mlir/IR/Operation.h> | ||
| #include <mlir/IR/OperationSupport.h> | ||
| #include <mlir/IR/PatternMatch.h> | ||
| #include <mlir/IR/Region.h> | ||
| #include <mlir/IR/ValueRange.h> | ||
| #include <mlir/Interfaces/ShapedOpInterfaces.h> | ||
| #include <mlir/Interfaces/SideEffectInterfaces.h> | ||
| #include <mlir/Support/LLVM.h> | ||
|
|
||
| // The following headers are needed for some template instantiations. | ||
|
|
@@ -30,6 +36,34 @@ | |
| using namespace mlir; | ||
| using namespace mlir::qco; | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // Dialect-Level Canonicalizers | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| namespace { | ||
|
|
||
| /** | ||
| * @brief Remove dead gates. | ||
| */ | ||
| struct DeadGateElimination final | ||
| : public OpInterfaceRewritePattern<UnitaryOpInterface> { | ||
|
|
||
| explicit DeadGateElimination(MLIRContext* context) | ||
| : OpInterfaceRewritePattern(context) {} | ||
|
|
||
| LogicalResult matchAndRewrite(UnitaryOpInterface op, | ||
| PatternRewriter& rewriter) const override { | ||
| if (!isMemoryEffectFree(op)) { | ||
| // This effectively ignores the GPhase operation and variants such as its | ||
| // inverse or `ctrl` ops containing it, which should never be considered | ||
| // dead. | ||
| return failure(); | ||
| } | ||
| return checkAndRemoveDeadGate(op.getOperation(), rewriter); | ||
| } | ||
| }; | ||
|
DRovara marked this conversation as resolved.
|
||
| } // namespace | ||
|
Comment on lines
+37
to
+62
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. I am wondering whether this cannot easily be replaced with a fold instead of a canonicalization pattern. and (in QCOOps.cpp) namespace mlir::qco {
template <typename ConcreteType>
class DeadIfOnlySinksFold
: public mlir::OpTrait::TraitBase<ConcreteType, DeadIfOnlySinksFold> {
public:
static mlir::LogicalResult foldTrait(
mlir::Operation* op, mlir::ArrayRef<mlir::Attribute> /*operands*/,
mlir::SmallVectorImpl<mlir::OpFoldResult>& results) {
// Only for side-effect-free ops.
if (!mlir::isMemoryEffectFree(op)) {
return mlir::failure();
}
// Only fold when every user is qco::SinkOp.
if (!llvm::all_of(op->getUsers(),
[](mlir::Operation* user) { return mlir::isa<SinkOp>(user); })) {
return mlir::failure();
}
// Restrict to UnitaryOpInterface-bearing ops.
auto u = mlir::dyn_cast<UnitaryOpInterface>(op);
if (!u) {
return mlir::failure();
}
// Fold by forwarding outputs to corresponding inputs.
for (mlir::Value v : u.getInputQubits()) {
results.push_back(v);
}
return mlir::success();
}
};
} // namespace mlir::qcoIt seems to me that it should always be possible to implement the currently implemented canonicalizations as folds.
Collaborator
Author
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. See my high-level comment. The docs never mention it not being allowed to access the users in a fold, but Gemini, Claude, and Perplexity all said it's best not to. Apparently, the fold framework in MLIR does not add elements to the worklist when their user changes. This means (from my interpretation) that there could be situations where we have the following chain of instructions: "A => B => X" Now if we have a rule that says "any operation that is followed by an X can be folded", then the worker might first look at "A", determine it can'f be folded, then look at "B", fold it, but then not check "A" again which could now also be folded. Unfortunately, none of the AIs could give me actual sources for that (Gemini said "There are precedents of PRs in the LLVM repo being rejected because they accessed the user list inside a fold" but couldn't provide a link), but they all seem to agree on that being the reason.
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. I tried to look around a little because I simply was not quite trusting the LLMs.
In essence, looking at operation results and their users is not allowed as it is non-local and context-dependent. So, after all, a canonicalization it is.
Collaborator
Author
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 think it's a valid consideration, but my gut feeling tells me it's better to be on the gates themselves. Mainly because a transformation pattern should aim to target/modify mainly the operation it matches. So matching But also conceptually, removing dead gates is not a property of the sinks, being dead is a property of the gates. If we ever decide that there's another type of operation that randers gates dead, then it is a straightforward addition to modify the current version, but it would be more of a change to add another canonicalisation for that.
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. I see your point. I believe my main concern was that this feels so repetitive. At the same time, I agree that it does not feel quite right for the canonicalization to not modify the operation it matches on. But is that really so bad? |
||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // Custom Parsers | ||
| //===----------------------------------------------------------------------===// | ||
|
|
@@ -258,6 +292,10 @@ | |
| >(); | ||
| } | ||
|
|
||
| void QCODialect::getCanonicalizationPatterns(RewritePatternSet& results) const { | ||
| results.add<DeadGateElimination>(getContext()); | ||
| } | ||
|
|
||
| //===----------------------------------------------------------------------===// | ||
| // Types | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,114 @@ TEST_F(QCOTest, BuilderRejectsMixedStaticAndDynamicQubitAllocationModes) { | |
| "Cannot mix dynamic and static qubit allocation modes"); | ||
| } | ||
|
|
||
| TEST_F(QCOTest, CheckDeadGateElimination) { | ||
|
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. I understand that it does not necessarily make sense to define such specialized programs in
Collaborator
Author
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 agree that this is not the cleanest place to put and handle the test. I feel like, ideally, there should be a specialised test file for general canonicalisations. I'll have a quick look and then either try that or implement it the way you suggested. |
||
| QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
| auto q0S0 = builder.allocQubit(); | ||
| auto q1S0 = builder.allocQubit(); | ||
| auto q0S1 = builder.h(q0S0); | ||
| auto [q0S2, q1S1] = builder.cx(q0S1, q1S0); | ||
| auto [q1S2, c1] = builder.measure(q1S1); | ||
| builder.sink(q0S2); | ||
| builder.sink(q1S2); | ||
| auto module = builder.finalize(); | ||
|
|
||
| QCOProgramBuilder reference(context.get()); | ||
| reference.initialize(); | ||
| auto r0 = reference.allocQubit(); | ||
| auto r1 = reference.allocQubit(); | ||
| reference.sink(r0); | ||
| reference.sink(r1); | ||
|
burgholzer marked this conversation as resolved.
Outdated
|
||
| auto refModule = reference.finalize(); | ||
|
|
||
| ASSERT_TRUE(module); | ||
| EXPECT_TRUE(verify(*module).succeeded()); | ||
| EXPECT_TRUE(runQCOCleanupPipeline(module.get()).succeeded()); | ||
| EXPECT_TRUE(verify(*module).succeeded()); | ||
|
|
||
| ASSERT_TRUE(refModule); | ||
| EXPECT_TRUE(verify(*refModule).succeeded()); | ||
| EXPECT_TRUE(runQCOCleanupPipeline(refModule.get()).succeeded()); | ||
| EXPECT_TRUE(verify(*refModule).succeeded()); | ||
|
|
||
| EXPECT_TRUE( | ||
| areModulesEquivalentWithPermutations(module.get(), refModule.get())); | ||
| } | ||
|
|
||
| TEST_F(QCOTest, CheckIfOpDeadGateElimination) { | ||
| QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
| auto q0S0 = builder.allocQubit(); | ||
| auto q1S0 = builder.allocQubit(); | ||
| auto q0S1 = builder.h(q0S0); | ||
| auto [q0S2, c0] = builder.measure(q0S1); | ||
|
|
||
| // This is an `if` with memory effects - it can't be removed. | ||
| auto q1S1 = builder.qcoIf( | ||
| c0, {q1S0}, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Then = builder.x(qubits[0]); | ||
| builder.gphase(0.5); | ||
| return SmallVector<Value>{q1Then}; | ||
| }, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Else = builder.h(qubits[0]); | ||
| return SmallVector<Value>{q1Else}; | ||
| })[0]; | ||
|
|
||
| // This is an `if` without memory effects - it can be removed. | ||
| auto q1S2 = builder.qcoIf( | ||
| c0, {q1S1}, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Then = builder.x(qubits[0]); | ||
| return SmallVector<Value>{q1Then}; | ||
| }, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Else = builder.h(qubits[0]); | ||
| return SmallVector<Value>{q1Else}; | ||
| })[0]; | ||
| builder.sink(q0S2); | ||
| builder.sink(q1S2); | ||
| auto module = builder.finalize(); | ||
|
|
||
| QCOProgramBuilder reference(context.get()); | ||
| reference.initialize(); | ||
| auto r0S0 = reference.allocQubit(); | ||
| auto r1S0 = reference.allocQubit(); | ||
| auto r0S1 = reference.h(r0S0); | ||
| auto [r0S2, cr0] = reference.measure(r0S1); | ||
|
|
||
| // This is an `if` with memory effects - it can't be removed. | ||
| auto r1S1 = reference.qcoIf( | ||
| cr0, {r1S0}, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Then = reference.x(qubits[0]); | ||
| reference.gphase(0.5); | ||
| return SmallVector<Value>{q1Then}; | ||
| }, | ||
| [&](ValueRange qubits) -> SmallVector<Value> { | ||
| auto q1Else = reference.h(qubits[0]); | ||
| return SmallVector<Value>{q1Else}; | ||
| })[0]; | ||
|
|
||
| reference.sink(r0S2); | ||
| reference.sink(r1S1); | ||
| auto refModule = reference.finalize(); | ||
|
burgholzer marked this conversation as resolved.
|
||
|
|
||
| ASSERT_TRUE(module); | ||
| EXPECT_TRUE(verify(*module).succeeded()); | ||
| EXPECT_TRUE(runQCOCleanupPipeline(module.get()).succeeded()); | ||
| EXPECT_TRUE(verify(*module).succeeded()); | ||
|
|
||
| ASSERT_TRUE(refModule); | ||
| EXPECT_TRUE(verify(*refModule).succeeded()); | ||
| EXPECT_TRUE(runQCOCleanupPipeline(refModule.get()).succeeded()); | ||
| EXPECT_TRUE(verify(*refModule).succeeded()); | ||
|
|
||
| EXPECT_TRUE( | ||
| areModulesEquivalentWithPermutations(module.get(), refModule.get())); | ||
| } | ||
|
DRovara marked this conversation as resolved.
|
||
|
|
||
| TEST_F(QCOTest, DirectIfBuilder) { | ||
| // Test If construction directly | ||
| QCOProgramBuilder builder(context.get()); | ||
|
|
||
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.
At this point, I am not quite sure there is much value in the shared helper function, when the implementation here would essentially be 5 lines of code.
Maybe this also goes well with the suggestion of replacing this with a fold, which necessarily has to be defined for the op itself.
Same comment applies to the ResetOp
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.
Yeah, true. When I originally implemented it I was able to use it everywhere but due to all the edge cases it ended up getting s specialized that I might as well have individual implementations for all uses. I did that now.
Anyways, I have done something else for now: The helper function is now called
checkDeadGateand it only checks if a gate is dead. This can be implemented in a general way. At least that way, if we ever decide to update the notion of a dead gate, it only takes a single change.In an ideal world where we really want to minimize code reuse, we might even want to implement some "remove this operation" helper methods for every
qcooperation. Because that's essentially what's left in the current implementation after thecheckIfDeadcheck is done. But for now, I kept the individual implementations in the different operation files because it's not really an issue yet.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.
Related to the comment above: Wouldn't it be simpler if this were simply a canonicalization of the
qco.sinkop? would be a fairly central place for all related code.Uh oh!
There was an error while loading. Please reload this page.
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.
See the answer above. I honestly don't really feel like the way it's implemented right now is wrong. Having the special operation handling in the gate files tells you exectly: "Okay I'm looking at the handling for an
IfOpright now" etc. and you don't have to chain a bunch ofif (isa<qco::IfOp>(op)) ...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 am not saying it is wrong by any means; it is obviously working.
But I am concerned that this might not be particularly efficient compared to the alternative.
The sink canonicalization would just contain a TypeSwitch, which is pretty efficient.