✨ Add pass and patterns for measurement lifting#1705
Draft
DRovara wants to merge 23 commits into
Draft
Conversation
DRovara
added
feature
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
…toolkit/core into mlir/measurement-lifting
Contributor
📝 WalkthroughWalkthroughThis PR adds two quantum circuit optimization passes to the QCO dialect: dead gate elimination removes quantum operations whose outputs are only consumed by sink operations, while measurement lifting moves measurements above certain gate patterns. The implementation includes trait/memory-effect updates marking operations as Pure and memory-effect-free, a dead-gate checking utility, canonicalization patterns, a measurement-lifting transformation pass with three rewrite strategies, program builder extensions, and comprehensive unit test coverage for both optimizations. ChangesDead Gate Elimination and Measurement Lifting Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (1)
99-139:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t model recorded
qco.measureas unconditionallyPure; it can be erased while still writing to classical registers.
MeasureOpis defined with[Pure], and canonicalization can drop it when its qubit result is only used byqco.sink(viacheckAndRemoveDeadGate, which replaces the qubit output with the input and erases the op, explicitly ignoring the classical outcome). When anyregister_*attribute is present, theQCToQIRlowering allocates a result array and passes a result-pointer into__quantum__qis__mz__body, so the recorded measurement produces observable classical output even if the SSAi1result is unused. Marking the opPuretherefore makes those register writes incorrectly erasable.Remove
Purefor recorded measurements (or otherwise make the operation report a write effect wheneverregister_name/register_size/register_indexare present), while keeping it effect-free only for the non-recording case.mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
1108-1160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-
i64exit codes in the new overload.
initialize()hard-codesmainto returni64, butfinalize(Value exitCode)accepts anyValue. Passing ani1here will build afunc.returnwith the wrong type and leave invalid IR; the new measurement-lifting tests already have to calli1ToI64(...)before using this overload for exactly that reason (mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_measurement_lifting.cpp:272-294). Add an early type check and document thei64requirement in the header.Suggested guard
OwningOpRef<ModuleOp> QCOProgramBuilder::finalize(Value exitCode) { checkFinalized(); + if (exitCode.getType() != getI64Type()) { + llvm::reportFatalUsageError("Exit code must have type i64"); + } // Ensure that main function exists and insertion point is valid auto* insertionBlock = getInsertionBlock();🤖 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/Builder/QCOProgramBuilder.cpp` around lines 1108 - 1160, The finalize(Value exitCode) overload accepts any Value but main() is hard-coded to return i64, so add an early runtime/type check in QCOProgramBuilder::finalize to reject non-i64 exitCode Values (e.g., verify exitCode.getType() is an IntegerType of width 64 and call llvm::reportFatalUsageError or similar if not) before creating func::ReturnOp; also update the QCOProgramBuilder::finalize declaration comment in the header to document the required i64 exit code and mirror the behavior of initialize() which sets main's return type to i64.
🤖 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 `@CHANGELOG.md`:
- Line 14: In CHANGELOG.md fix the broken contributor reference by replacing the
incorrect label "[**DRovara**]" with the correct label "[**`@DRovara`**]" so it
matches the existing contributor reference definitions; update the line
containing "✨ Add Dead Gate Elimination Pattern ([`#1755`]) ([**DRovara**])" to
use "[**`@DRovara`**]" instead.
In `@mlir/include/mlir/Dialect/QCO/QCOUtils.h`:
- Around line 258-263: The MeasureOp removal currently replaces uses with
m.getQubitIn() and erases the op unconditionally; update the branch handling
MeasureOp so it first checks the measurement's classical-recording fields (e.g.,
register_name, register_size, register_index) and only perform
rewriter.replaceAllUsesWith(m.getQubitOut(), m.getQubitIn()) followed by
rewriter.eraseOp(op) when those fields indicate the measurement is not recorded
(empty/default). If any of register_name/register_size/register_index show the
measurement is recorded, skip the erase (return failure/skip) to preserve the
classical output.
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 181-183: Fix the typos in the MLIR pass summary string: update the
TD variable 'summary' (let summary = ...) to replace "shiftling" with "shifting"
and ensure there's a space between "as" and "possible" so the concatenated
strings read "as far up as possible, shifting them above...". Edit the two
adjacent string literals so they join correctly and contain the corrected word.
- Around line 184-188: The description string assigned to let description in
mlir/include/mlir/Dialect/QCO/Transforms/Passes.td contains a copy-paste error
("lifts measurements gates away from the measurements") likely from
HadamardLifting; update the description to accurately describe this pass's
behavior (measurement lifting as part of qubit reuse: moving/earlier placement
of measurement operations to enable qubit reuse and gate removal), editing the
let description block for the relevant pass (compare with the HadamardLifting
description to avoid repeating its phrasing) so the text clearly states that
measurement lifting moves measurements earlier to free qubits for reuse and
optimize/remove subsequent quantum gates.
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 1103-1105: The no-arg QCOProgramBuilder::finalize() currently
calls intConstant(0) before running the validation that finalize(Value)
performs, which can create ops or mutate the wrong block if the builder wasn't
initialized or the insertion point isn't main's entry; change
QCOProgramBuilder::finalize() to first validate that 'main' exists and that the
current insertion point is inside main's entry block (reusing the same checks
used by finalize(Value)), and only after those checks succeed call
intConstant(0) and forward to finalize(Value); reference the methods
QCOProgramBuilder::finalize(), QCOProgramBuilder::finalize(Value), and the
helper intConstant to locate where to reorder the work.
In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/MeasurementLifting.cpp`:
- Line 44: The free helper functions are currently relying on
anonymous-namespace linkage but should be declared with static linkage per
project convention; change the declarations of the free functions isInverting,
isDiagonal, and swapGateWithMeasurement to be marked static (e.g., static bool
isInverting(...), static bool isDiagonal(...), static Operation*
swapGateWithMeasurement(...)) so they use internal linkage instead of an
anonymous namespace, leaving their bodies and call sites unchanged.
- Line 29: The file includes the unused header <numbers> which should be
removed; edit MeasurementLifting.cpp to delete the line '`#include` <numbers>'
(ensure no other uses of std::numbers remain in functions such as any helpers or
transformations in MeasurementLifting-related functions) so the build has no
superfluous include.
- Line 51: The isDiagonal predicate currently checks for ZOp, SOp, TOp, POp,
RZOp but omits diagonal gate types used in tests; update the isDiagonal function
to also recognize SdgOp, TdgOp, and IdOp so measurements can be lifted over id,
sdg, and tdg gates (i.e., include SdgOp, TdgOp, IdOp alongside the existing
isa<ZOp, SOp, TOp, POp, RZOp> check in the isDiagonal helper).
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp`:
- Around line 117-223: Add a new unit test (e.g., in QCOTest alongside
CheckIfOpDeadGateElimination) that creates a measurement with a
recorded/classical name (call builder.measure with the recorded-name overload
like measure("c", 1, 0) or the API that records the result), uses those qubit
outputs and sinks them (so both qubits are later sunk), and constructs a
reference module where the measurement is preserved; then run
verify/runQCOCleanupPipeline on both and assert
areModulesEquivalentWithPermutations(module, refModule). This ensures the
cleanup pass does not eliminate measurements whose classical result is
externally recorded; reference the existing QCOProgramBuilder usage,
builder.measure, builder.qcoIf, and areModulesEquivalentWithPermutations to
locate where to add the test.
In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_measurement_lifting.cpp`:
- Around line 318-342: The isDiagonal predicate in MeasurementLifting.cpp
currently checks only ZOp, SOp, TOp, POp, RZOp, so id, sdg and tdg gates are not
treated as diagonal; update the isDiagonal implementation to also return true
for the identity, sdg and tdg ops (e.g., IdOp, SdgOp, TdgOp or whatever the MLIR
op class names are in your codebase) so MeasurementLifting::lift (and related
code using isDiagonal) will permit lifting measurements over id, sdg and tdg
gates.
- Around line 296-316: The reference test liftMeasurementOverSingleY creates
xorOp but never uses its result, so canonicalization can drop the dead XOR;
update the referenceBuilder finalization to include xorOp's result (e.g.,
convert the xor i1 to i64 like i1ToI64 or otherwise pass it into
referenceBuilder.finalize()) so the XOR is live, and likewise ensure the
programBuilder path passes its measurement result to finalize() so the test
actually verifies the inversion inserted by runMeasurementLiftingPass; modify
usages around xorOp, referenceBuilder.finalize(), and programBuilder.finalize()
accordingly.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp`:
- Around line 1108-1160: The finalize(Value exitCode) overload accepts any Value
but main() is hard-coded to return i64, so add an early runtime/type check in
QCOProgramBuilder::finalize to reject non-i64 exitCode Values (e.g., verify
exitCode.getType() is an IntegerType of width 64 and call
llvm::reportFatalUsageError or similar if not) before creating func::ReturnOp;
also update the QCOProgramBuilder::finalize declaration comment in the header to
document the required i64 exit code and mirror the behavior of initialize()
which sets main's return type to i64.
🪄 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: 285fd211-de81-448a-8fe6-648bb7642bb9
📒 Files selected for processing (15)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/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/lib/Dialect/QCO/Transforms/Optimizations/MeasurementLifting.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_measurement_lifting.cpp
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Part 1 out of 3 of the Qubit Reuse Implementation
This PR ports the previous implementation of the Measurement Lifting pass to the new dialect infrastructure.
Measurements can be lifted over:
This is one of 3 PRs that will, in total, constitute qubit reuse:
Requires #1755 to be merged first.
AI Notice: Gemini 3.1 and Claude Sonnet 4.6 were used for the generation of some code.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.