✨ 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>
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.
| //===----------------------------------------------------------------------===// | ||
| // 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.
There was a problem hiding this comment.
I tried to look around a little because I simply was not quite trusting the LLMs.
After a bit of digging, I conclude that you/they are right. From what I could gather
A fold is meant to be:
- Purely local to the operation
- Cheap and side-effect-free
- Context-independent
- Deterministic based only on operands and attributes
Concretely, a fold implementation:
Operates only on:
- The operation itself
- Its operands (via FoldAdaptor)
- Its attributes
Returns:
- Existing values, or
- Constants (Attribute → constant materialization)
Must not mutate IR directly (no erasing/rewriting arbitrary ops)
Must not depend on analysis or global structure
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.
This got me thinking though: Couldn't this be a single canonicalization that is implemented for the qco.sink operation, which would look at the defining op of the incoming qubit value?
There was a problem hiding this comment.
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 qco.sink and then only modifying the predecessors is not best practise.
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.
There was a problem hiding this comment.
I see your point. I believe my main concern was that this feels so repetitive.
And matching the sink operation would centralize all the code, but also the pattern matching itself. Instead of matching every single unitary and having to check its successor, it would only match a single operation and check the predecessor.
In terms of efficiency, this sounds way better to me.
And yeah, if another type of operation comes up, this would mean some duplication; but I would argue that even in that case, it seems more efficient to match one of two operations than to match against every single one.
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?
| 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.
There was a problem hiding this comment.
Related to the comment above: Wouldn't it be simpler if this were simply a canonicalization of the qco.sink op? would be a fairly central place for all related code.
There was a problem hiding this comment.
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 IfOp right now" etc. and you don't have to chain a bunch of if (isa<qco::IfOp>(op)) ...
There was a problem hiding this comment.
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.
|
@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
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
If you inspect the MLIR outputs of the Pipeline tests for example, then most test cases simply get optimized to empty programs.
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
initializeandfinalizemethods so that they can also take arbitrary return types and values. That way, if we just measure and return all qubits, all values will remain used. Does that sound like a good solution?
Generally sounds good to me. You might have meant this, but I would have expected the classical measurement results to be returned instead of the qubits.
This may also be a nice way of improving the QIR conversion, because classical return values could be transformed to output recordings in QIR. @denialhaag would that make sense?
| //===----------------------------------------------------------------------===// | ||
| // 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 tried to look around a little because I simply was not quite trusting the LLMs.
After a bit of digging, I conclude that you/they are right. From what I could gather
A fold is meant to be:
- Purely local to the operation
- Cheap and side-effect-free
- Context-independent
- Deterministic based only on operands and attributes
Concretely, a fold implementation:
Operates only on:
- The operation itself
- Its operands (via FoldAdaptor)
- Its attributes
Returns:
- Existing values, or
- Constants (Attribute → constant materialization)
Must not mutate IR directly (no erasing/rewriting arbitrary ops)
Must not depend on analysis or global structure
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.
This got me thinking though: Couldn't this be a single canonicalization that is implemented for the qco.sink operation, which would look at the defining op of the incoming qubit value?
| 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.
Related to the comment above: Wouldn't it be simpler if this were simply a canonicalization of the qco.sink op? would be a fairly central place for all related code.
Yeah, returning classical values from the main function and using that information for the output recording sounds sensible to me! Two thoughts:
|
I would argue that this PR here is currently blocking more PRs than the QIR one is. So my tendency would be to merge this here first and afterwards adjust the QIR one.
Yeah; there is almost no way around this. Generally, I believe this very much improves the test programs though. It makes them more complete quantum programs. |
Makes sense to me! It might make sense to have a look at the implementation in #1710 and fix the output recording directly before we have another version of a not-exactly-correct implementation. 🤔
Agreed! |
|
Alright, @burgholzer, @denialhaag, I have now updated all the IR tests to return the values of the measured qubits. That being said, this probably still needs a second look from you, as I was previously not too involved in the tests, so I'm not sure if I handled any situations incorrectly, and I am quite sure that there could also be other ways to implement all of this cleaner. In any case, the tests now still cause errors because of the ordering issue during the equivalence check, so that still needs to be fixed somehow for all of this to work. Also, I have not touched the transformation tests. But most transformation tests don't even touch the canonicalisations, so it should not be an issue to begin with. The only exception is |
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks a lot for the changes, @DRovara! I like where this is going!
Besides the comments below, I still see one major issue: Don't we also need to return the measurement results in QC? At the very least, the QC-to-QCO conversion would need to adjust the main function signature. Otherwise, the converted QCO programs should be empty right now (after canonicalization, that is).
(For some reason, the build is currently failing. Could it be that you only built the mqt-core-mlir-unittest-qco-ir target locally? 🤔)
|
|
||
| #pragma once | ||
|
|
||
| #include <llvm/ADT/TypeSwitch.h> |
There was a problem hiding this comment.
Is this include still needed? 🤔
There was a problem hiding this comment.
You're right, I removed it now
| } | ||
|
|
||
| void QCOProgramBuilder::retype(TypeRange returnTypes) { | ||
| auto mainFunc = dyn_cast<ModuleOp>(module).lookupSymbol<func::FuncOp>("main"); |
There was a problem hiding this comment.
The entry point is generally defined as the function that has the entry_point attribute (see, e.g., getEntryPoint()).
There was a problem hiding this comment.
Done, thanks for the suggestion!
| "Cannot mix dynamic and static qubit allocation modes"); | ||
| } | ||
|
|
||
| TEST_F(QCOTest, CheckDeadGateElimination) { |
There was a problem hiding this comment.
I understand that it does not necessarily make sense to define such specialized programs in qco_programs.h, but would it be possible to define the programs via similar functions in this file and then use QCOTestCase? 🤔
There was a problem hiding this comment.
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.
| /** | ||
| * @brief Convenience method for building quantum programs with returns. | ||
| * @param context The MLIR context to use for building the program | ||
| * @param returnTypes The types of the values to be returned by the program. | ||
| * @param buildFunc A function that takes a reference to a QCOProgramBuilder | ||
| * and uses it to build the desired quantum program. The builder will be | ||
| * properly initialized before calling this function, and the resulting module | ||
| * will be finalized using the returned ValueRange after this function | ||
| * completes. | ||
| * @return The module containing the quantum program built by buildFunc. | ||
| */ | ||
| static OwningOpRef<ModuleOp> buildWithReturn( | ||
| MLIRContext* context, | ||
| const function_ref<std::pair<SmallVector<Value>, SmallVector<Type>>( | ||
| QCOProgramBuilder&)>& buildFunc); |
There was a problem hiding this comment.
Without having thought about it too much right now, is there still a real need for the old build() method, or should any real QCO program return something (other than the exit code)? 🤔
There was a problem hiding this comment.
Good question. I didn't feel confident enough to remove the original build function.
I can imagine scenarios where it would still be useful, the question is just (1) are they valid/important enough? and (2) are they maybe that rare that users could just use buildWithReturn with a function pointer that just returns an empty type range or a single ì64` exit code instead?
Example:
A program that is supposed to be translated down to QIR and should then run the record_outputs (don't remember the exact name) operation. Or, if that's what the new returns will be translated into, then maybe programs with intermediate print operations.
In any case, I have no problem with just removing the original build method, but I feel like that should be your call.
There was a problem hiding this comment.
I'd be fine with dropping the original build method. One can always emulate it with the existing builder by returning an exit code. So the new builder is strictly more expressive.
Edit: the return values of entry point functions will be translated to output recording calls in QIR.
|
|
||
| #pragma once | ||
|
|
||
| #include <mlir/IR/ValueRange.h> |
There was a problem hiding this comment.
In header files, we try to only include mlir/Support/LLVM.h, which mainly contains forward declarations.
| #include <mlir/IR/ValueRange.h> | |
| #include <mlir/Support/LLVM.h> |
There was a problem hiding this comment.
Gotcha, thanks for clarifying!
There was a problem hiding this comment.
Turns out that's not enough, as it does not declare mlir::Value I need <mlir/IR/Value-h> for that as well
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: Damian Rovara <93778306+DRovara@users.noreply.github.com>
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: Damian Rovara <93778306+DRovara@users.noreply.github.com>
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: Damian Rovara <93778306+DRovara@users.noreply.github.com>
Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Signed-off-by: Damian Rovara <93778306+DRovara@users.noreply.github.com>
Yeah you're absolutely right. My initial though was "This canonicalization only happens in QCO so we don't have to worry about QC programs", but of course if we start from QC and translate down to QCO, then we also need return values there. Though, I guess this thought can also be extended in other directions. The translation from other input formats would also require to track some return values then.
Ah you're right. Turns out the build error is related to the other point: Currently, the |
I would argue it is quite natural for the QC programs to also return results from the main entry points. OpenQASM has the concept of outputs and defaults to output all values of classical registers in the order of definition if no explicit output statements are present.
I have no real other ideas. A large PR isn't bad per-se, if most of the changes are mechanical (updating the test programs). |
Yeah, Edit: This Catalyst example, for example, returns a measurement result from the entry-point function: https://github.com/unitaryfoundation/jeff/blob/main/examples/catalyst_simple/catalyst_simple.txt |
|
@DRovara The QIR PR is in. The Verifier PR looks practically ready. So I would argue work on this can continue 🙂 |
|
@DRovara FYI: The new verifier is now merged into |
4 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.