✨ Add Dead Gate Elimination Pattern#1755
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a dead-gate elimination canonicalizer: a shared helper detects operations whose only users are ChangesDead Gate Elimination Pattern
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/QCOUtils.h`:
- Line 252: Update the comment to use the dialect's terminology: replace
"deallocs" with "sinks" or "SinkOp" so it matches the check against SinkOp in
the code; e.g., modify the comment above the removal check to read something
like "If the operation is only used by sinks (SinkOp), we can safely remove it."
Ensure the comment near the function that inspects SinkOp usage (referencing
SinkOp) is updated for consistency.
In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp`:
- Around line 38-54: Add a Doxygen-style comment block for the
DeadGateElimination struct describing its purpose and behavior; place a brief
/** ... */ above the struct declaration for DeadGateElimination, mention that it
is an OpInterfaceRewritePattern for UnitaryOpInterface, summarize that
matchAndRewrite skips ops with no uses (while excluding GPhase/inverse) and
delegates removal to checkAndRemoveDeadGate, and keep the description concise
per repository guidelines.
In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp`:
- Around line 239-241: Update the Doxygen summary for the DeadIfRemoval pass to
accurately describe its purpose: replace the misleading "Remove dead resets"
summary with a concise Doxygen-style line such as "Canonicalize and remove dead
IfOp patterns" (or "Remove dead IfOps / canonicalize IfOp patterns") so that the
comment above the DeadIfRemoval class/function reflects that it canonicalizes
IfOps; ensure the comment uses proper Doxygen syntax and is placed directly
above the DeadIfRemoval symbol.
- Around line 245-247: Guard the removal of IfOp in matchAndRewrite by ensuring
branch regions contain no side-effecting operations before calling
checkAndRemoveDeadGate; specifically, before erasing op in matchAndRewrite (or
inside checkAndRemoveDeadGate) inspect op.thenRegion() and op.elseRegion() (or
the region bodies) and bail out if any contained Operation
mayHaveSideEffects()/hasTrait<OpTrait::HasSideEffects>() or reports
MemoryEffects—only allow deletion when both regions are empty or provably
side-effect-free. Ensure you reference IfOp, matchAndRewrite,
checkAndRemoveDeadGate, thenRegion/elseRegion and use the Operation side-effect
query APIs to make the decision.
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 117-148: Extend the test to also exercise dead-gate elimination
for measurement and conditional ops by adding a measurement (MeasureOp) whose
result is unused and an IfOp that produces an output which is never consumed,
then sinking those unused outputs and ensuring the cleaned module matches a
reference without those ops; specifically, in the TEST_F(QCOTest,
CheckDeadGateElimination) case, use QCOProgramBuilder methods that create a
measurement (e.g., builder.measure or MeasureOp via builder) and an
if/conditional op (e.g., builder.ifOp/IfOp) producing qubits/bits, do not
consume their results (call builder.sink on those outputs), finalize module and
create a reference program that omits the measurement and if op, then run
verify, runQCOCleanupPipeline, verify again and assert
areModulesEquivalentWithPermutations(module.get(), ref.get()) to validate the
dead-gate paths for MeasureOp and IfOp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0906695c-971e-4416-bb67-64f8691df3f6
📒 Files selected for processing (9)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/IR/QCODialect.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/lib/Dialect/QCO/IR/Operations/MeasureOp.cppmlir/lib/Dialect/QCO/IR/Operations/ResetOp.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QCO/IR/SCF/IfOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/IR/QCOOps.cpp (1)
44-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the Doxygen summary for
DeadGateElimination.The comment says "Remove dead measurements" but this pattern operates on
UnitaryOpInterface, notMeasureOp. The description should match the pattern's scope.✏️ Suggested fix
/** - * `@brief` Remove dead measurements. + * `@brief` Remove dead unitary operations (gates). */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp` around lines 44 - 46, Update the Doxygen summary for DeadGateElimination to accurately describe its scope: replace "Remove dead measurements" with a brief line stating that DeadGateElimination removes dead gates (or eliminates operations) on UnitaryOpInterface instances rather than referring to MeasureOp; ensure the summary references DeadGateElimination and UnitaryOpInterface (and remove or avoid mentioning MeasureOp) so the comment matches the pattern's actual target.mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp (1)
245-248:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard dead
IfOpelimination against side effects inside the branches.
IfOphasRecursiveMemoryEffects, so its memory effects are derived from child operations. Without checkingisMemoryEffectFree(op), this pattern can incorrectly remove anIfOpwhose branches contain side-effecting operations (e.g.,GPhaseOpwithMemWrite).The analogous
DeadGateEliminationpattern inQCOOps.cppcorrectly guards with!isMemoryEffectFree(op).🐛 Proposed fix to add memory-effect guard
struct DeadIfRemoval final : OpRewritePattern<IfOp> { using OpRewritePattern::OpRewritePattern; LogicalResult matchAndRewrite(IfOp op, PatternRewriter& rewriter) const override { + if (!isMemoryEffectFree(op)) { + return failure(); + } return checkAndRemoveDeadGate(op, rewriter); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp` around lines 245 - 248, The matchAndRewrite implementation for IfOp currently always calls checkAndRemoveDeadGate(op, rewriter) and can remove IfOp with side-effecting branch ops; update the pattern to first check isMemoryEffectFree(op) (like DeadGateElimination in QCOOps.cpp) and only call checkAndRemoveDeadGate when the check passes (i.e., skip removal if !isMemoryEffectFree(op)). Ensure you reference the IfOp instance passed into matchAndRewrite and use the existing isMemoryEffectFree utility to guard the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/QCOUtils.h`:
- Around line 250-251: Replace the use of std::all_of with llvm::all_of for the
users check: change the call that currently reads
std::all_of(op->getUsers().begin(), op->getUsers().end(), [](Operation* user){
return isa<SinkOp>(user); }) to use llvm::all_of with the same range and lambda
(i.e., llvm::all_of(op->getUsers(), [](Operation *user) { return
isa<SinkOp>(user); }) or llvm::all_of(op->getUsers().begin(),
op->getUsers().end(), ...)); keep the op->getUsers() range and the isa<SinkOp>
predicate intact and ensure the appropriate LLVM header is available.
---
Duplicate comments:
In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp`:
- Around line 44-46: Update the Doxygen summary for DeadGateElimination to
accurately describe its scope: replace "Remove dead measurements" with a brief
line stating that DeadGateElimination removes dead gates (or eliminates
operations) on UnitaryOpInterface instances rather than referring to MeasureOp;
ensure the summary references DeadGateElimination and UnitaryOpInterface (and
remove or avoid mentioning MeasureOp) so the comment matches the pattern's
actual target.
In `@mlir/lib/Dialect/QCO/IR/SCF/IfOp.cpp`:
- Around line 245-248: The matchAndRewrite implementation for IfOp currently
always calls checkAndRemoveDeadGate(op, rewriter) and can remove IfOp with
side-effecting branch ops; update the pattern to first check
isMemoryEffectFree(op) (like DeadGateElimination in QCOOps.cpp) and only call
checkAndRemoveDeadGate when the check passes (i.e., skip removal if
!isMemoryEffectFree(op)). Ensure you reference the IfOp instance passed into
matchAndRewrite and use the existing isMemoryEffectFree utility to guard the
removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcefdaa8-d6db-49c7-8ec3-d1552d3f463a
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/lib/Dialect/QCO/IR/Operations/MeasureOp.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QCO/IR/SCF/IfOp.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp
|
@burgholzer the implementation itself should be done now, but the tests will still fail until we fixed the ordering issue in the tester that we discussed internally. I guess this PR can either be reviewed now already or once that fix has been merged in here. |
Thanks! I'll try to review this asap. Let's see if that turns out to be before or after the fix. |
DRovara
added
feature
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
@DRovara Thanks for pushing this. I pushed very small cleanups directly. The bigger part of my review is in the comments attached with this review. This will need more work before we can merge.
One of the bigger points in the review comments is that this could probably all be folds instead of canonicalizations, which should generally be more efficient.
Additionally, and this is something not in the individual comments, this is currently breaking almost our entire testing pipeline. If you inspect the MLIR outputs of the Pipeline tests for example, then most test cases simply get optimized to empty programs. While most tests still pass, this is obviously not the point of these unit tests. We need to find a way to allow for this kind of optimization, but make sure that it only applies when it actually should apply. In particular, we need to make sure that the default programs generated by our builders are "complete" programs that do not all reduce to the empty program. My best guess would be, that we need some notion of the "output of a quantum program". In QIR terms, this would be the output recording. Possibilities here include adding a dedicated "record_output" operation (similar to what is currently being done in FullStaQD) or to (somewhat) have our main entry point actually return something, which would also make the values "used".
In its present state, I would argue the PR here does not work as intended.
@munich-quantum-toolkit/mqt-cc for awareness. Feel free to jump in if you have ideas.
|
|
||
| def MeasureOp : QCOOp<"measure"> { | ||
| def MeasureOp : QCOOp<"measure", [Pure]> { | ||
| let summary = "Measure a qubit in the computational basis"; |
There was a problem hiding this comment.
Instead of adding the Pure trait to all individual operations, wouldn't it make sense/be possible to simply add the trait to the UnitaryOpInterface in QCO? Then we wouldn't have to specify it everywhere and it would be even clearer that any unitary operation is considered Pure in QCO
There was a problem hiding this comment.
Apparently, it is not possible to do that. The only thing one could do is implement a second operation base class like
class QCOUnitaryOp<string mnemonic, list<Trait> traits = []>
: QCOOp<mnemonic, !listconcat(traits, [UnitaryOpInterface, Pure])> {
And then have the unitary operations inherit that rather than QCOOp. But it feels a bit weird to have a abstract base class and an interface for unitaries...
| //===----------------------------------------------------------------------===// | ||
| // 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); | ||
| } | ||
| }; | ||
| } // namespace |
There was a problem hiding this comment.
I am wondering whether this cannot easily be replaced with a fold instead of a canonicalization pattern.
From what I can tell from a conversation with Claude, a trait that defines the foldTrait method could do the trick here to avoid having to define the same fold over and over again. A boilerplate draft for this could be
include "mlir/IR/OpBase.td"
def DeadIfOnlySinksFoldTrait : NativeOpTrait<"DeadIfOnlySinksFold"> {
let description = [{
Folds unitary-like ops away when all users are qco.sink by forwarding
output qubits to corresponding input qubits.
}];
}
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.
There was a problem hiding this comment.
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.
| struct DeadMeasurementRemoval final : OpRewritePattern<MeasureOp> { | ||
| using OpRewritePattern::OpRewritePattern; | ||
|
|
||
| LogicalResult matchAndRewrite(MeasureOp op, | ||
| PatternRewriter& rewriter) const override { | ||
| return checkAndRemoveDeadGate(op, rewriter); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
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 checkDeadGate and 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 qco operation. Because that's essentially what's left in the current implementation after the checkIfDead check is done. But for now, I kept the individual implementations in the different operation files because it's not really an issue yet.
| if (!isMemoryEffectFree(op)) { | ||
| // This effectively ignores `IfOp`s with memory effects. | ||
| return failure(); | ||
| } |
There was a problem hiding this comment.
Probably just me being stupid, but isn't this always true now that the IfOp is marked as Pure? Same comment actually also applies for the general case.
There was a problem hiding this comment.
In addition to being marked as Pure, the IfOp is also marked as RecursiveMemoryEffects. That means that, if any operation inside it has memory effects, then the IfOp also has memory effects, independent of it being marked as Pure.
The "override" actually goes in the other direction:
If the IfOp was not marked as Pure, then even if all its child instructions were Pure and IfOp has RecursiveMemoryEffects, MLIR would still not be certain that the IfOp is memory-effect free because the region operation itself might also have memory effects.
The test case CheckIfOpDeadGateElimination actually shows exactly that situation, where one of the IfOps contains a GPhaseOp and is therefore no longer memory-effect-free.
| auto r0 = reference.allocQubit(); | ||
| auto r1 = reference.allocQubit(); | ||
| reference.sink(r0); | ||
| reference.sink(r1); |
There was a problem hiding this comment.
This feels like a slighlty pointless test. The consecutive pairs of alloc and sink would be eliminated by canonicalization, which would leave this as an empty program. I find it dangerous to define a reference that will actually (knowingly) be further transformed before the equivalence check.
There was a problem hiding this comment.
That's a good point. I have now slightly changed up the test so that some operations remain in the program, that's probably the cleanest solution.
| // 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(); |
There was a problem hiding this comment.
I am probably too dense to see it, but where does this if have memory effects? wouldn't it also simply be optimized away and both the reference program and the initial program optimize to the empty program?
There was a problem hiding this comment.
The gphase Operation is marked as having memory effects (which it needs to have, after all it has no return values).
Just to be sure I quickly dumped the outcome and it is as expected, with just a single if (while the other one gets eliminated):
module {
func.func @main() -> i64 attributes {passthrough = ["entry_point"]} {
%c0_i64 = arith.constant 0 : i64
%cst = arith.constant 5.000000e-01 : f64
%0 = qco.alloc : !qco.qubit
%1 = qco.alloc : !qco.qubit
%2 = qco.h %0 : !qco.qubit -> !qco.qubit
%qubit_out, %result = qco.measure %2 : !qco.qubit
%3 = qco.if %result args(%arg0 = %1) -> (!qco.qubit) {
%4 = qco.x %arg0 : !qco.qubit -> !qco.qubit
qco.gphase(%cst)
qco.yield %4 : !qco.qubit
} else args(%arg0 = %1) {
%4 = qco.h %arg0 : !qco.qubit -> !qco.qubit
qco.yield %4 : !qco.qubit
}
qco.sink %qubit_out : !qco.qubit
qco.sink %3 : !qco.qubit
return %c0_i64 : i64
}
}
|
@burgholzer Thanks for the review! Some comments:
That was my original plan, but I had some discussions with LLMs and they all agreed this should be a canonicalisation for API reasons. For instance, Gemini says:
Ah right, I completely forgot about that. I actually already found a solution for that, I just didn't implement it for the existing tests. If you view the QCOProgramBuilder in #1756, I have extended the |
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Damian Rovara <93778306+DRovara@users.noreply.github.com>
…m-toolkit/core into mlir/dead-gate-elimination
2 participants
Description
This PR implements Dead Gate Elimination for unused operations on qubits.
An operation is considered unused if all of its outputs' users are dealloc operations.
This means a measurement can also be dead if its classical outcome is not used.
Similarly,
resetandifoperations can also be considered unused.This PR marks all quantum gates as
Pure(and marks theRecursiveMemoryEffectsoperations asPureas well, as otherwise, any recursive memory effects would be overwritten by the compiler not knowing whether these region operations are pure to begin with). This allows us to simply check for memory effects before removing region operations.Currently, this only considers individual qubits, rather than
qtensorcases to keep this PR contained.An extension to qtensors should also be possible, though it may require some additional assumptions to maintain overview on what specific qubits end up going unused.
AI Usage note: Code was written manually with minor assistance from GitHub Copilot. Some design decisions were taken after discussions with Gemini 3.1 Pro.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.