✨ Canonicalize equivalent qc.static operations#1567
✨ Canonicalize equivalent qc.static operations#1567simon1hofmann wants to merge 5 commits intomainfrom
qc.static operations#1567Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Common Subexpression Elimination (CSE) into the compiler cleanup/canonicalization pass sequence and extends unit tests and program helpers to cover duplicate static-qubit allocations across QC/QIR/QCO test suites; updates documentation and CHANGELOG accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CompilerPipeline as CompilerPipeline
participant PassManager as PassManager
participant Canonicalizer as Canonicalizer
participant CSE as CSE
participant RemoveDeadValues as RemoveDeadValues
CompilerPipeline->>PassManager: addCleanupPasses()
PassManager->>Canonicalizer: addPass(createCanonicalizer())
PassManager->>CSE: addPass(createCSEPass())
PassManager->>RemoveDeadValues: addPass(createRemoveDeadValuesPass())
CompilerPipeline->>PassManager: run(module)
PassManager->>Canonicalizer: run(module)
Canonicalizer-->>PassManager: canonicalized IR
PassManager->>CSE: run(module)
CSE-->>PassManager: eliminated common subexpressions
PassManager->>RemoveDeadValues: run(module)
RemoveDeadValues-->>PassManager: removed dead values
PassManager-->>CompilerPipeline: finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/unittests/Compiler/test_compiler_pipeline.cpp`:
- Around line 188-192: The test case uses mlir::qc::staticQubits as the
canonical reference while the input builder
MQT_NAMED_BUILDER(mlir::qc::staticQubitsWithDuplicates) emits extra h/x gates;
update the canonical comparator to the actual canonical builder by replacing
mlir::qc::staticQubits with mlir::qc::staticQubitsCanonical in the
CompilerPipelineTestCase entry so the test compares against the correct
canonical QC implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3faf48d-3418-4128-9d82-ad8df55b2646
📒 Files selected for processing (7)
mlir/include/mlir/Compiler/CompilerPipeline.hmlir/lib/Compiler/CompilerPipeline.cppmlir/lib/Support/Passes.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.h
simon1hofmann
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 18: Add an UPGRADING.md entry documenting the canonicalization behavior
change introduced by PR `#1567`: describe that qc.static equivalence and
deduplication semantics have changed, explain the effect on downstream IR
expectations and tests (what may break or need updating), and provide brief
migration guidance or examples for updating tests and IR consumers to the new
canonicalization rules; place the note in UPGRADING.md and reference PR `#1567`
and the qc.static/deduplication behavior so maintainers can find the relevant
changelog entry.
|
@simon1hofmann, thanks a lot for updating the pipeline and the tests! 🙂 I left a couple of comments. After they are addressed, this should be ready to go in! |
There was a problem hiding this comment.
Sorry for the somewhat confused initial review... The improved examples look good to me now!
Below, I'm proposing a simplification that feels closer to how we do things in the QCO tests. Feel free to resolve the comments if you disagree.
I'd propose we leave this PR open for a bit longer so @burgholzer can give a final approval.
…R, and QCO programs to remove canonical versions and enhance functionality with controlled operations.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
As per the comment on Discord, this will first need another fix before it can go in.
And with that fix, the tests here can be made significantly simpler (hopefully).
Specifically, the QCO programs being generated for static qubits are currently not valid as there is no "sink" for the qubit values corresponding to static qubits. Similarly to dynamic qubits, the static allocations need to be deallocated at the end of the program.
This is also related, and the fix probably closes, the issue that @MatthiasReumann raised regarding static qubits.
I'd prefer that to be fixed in a separate PR and have this PR be rebased afterwards, if that is fine with you.
I opened another PR (#1569) to try and fix that behavior. |
3 participants
Description
In the QC dialect two
qc.staticoperations using the same static index represent the equivalent reference to a hardware qubit. Likewise, two (static) registers in the QC dialect consisting of the same static indices are also equivalent.Remove all but the first
sc.staticoperation and replace the uses accordingly using MLIR's CSE pass (https://mlir.llvm.org/docs/Passes/#-cse)Fixes #1514
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.