✨ Add QIRSetAttributesAndMetadata Pass#1787
Open
MatthiasReumann wants to merge 35 commits into
Open
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
MatthiasReumann
changed the title
AttachEntryPointAttributes PassAttachQIRAttributes Pass
MatthiasReumann
changed the title
AttachQIRAttributes PassQIRSetAttributesAndMetadata Pass
…h-quantum-toolkit/core into feat/set-qir-metadata-pass
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughThe ChangesQIR Metadata Refactor
Sequence Diagram(s)sequenceDiagram
participant CompilerPipeline
participant QCToQIRAdaptive_Base as QCToQIRAdaptive / QCToQIRBase
participant runQIRCleanupPipeline
participant populateQIRCleanupPipeline
participant QIRSetAttributesAndMetadata
CompilerPipeline->>QCToQIRAdaptive_Base: runStage: add adaptive or base pass (no metadata flags set)
CompilerPipeline->>runQIRCleanupPipeline: runQIRCleanupPipeline(module, useAdaptive)
runQIRCleanupPipeline->>populateQIRCleanupPipeline: populateQIRCleanupPipeline(pm, useAdaptive)
populateQIRCleanupPipeline->>QIRSetAttributesAndMetadata: createQIRSetAttributesAndMetadata({useAdaptive})
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: getNumQubits() trace constants→inttoptr→QIR calls
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: getNumResults() trace QIR_RECORD_OUTPUT
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: usesBackwardsBranching() DominanceInfo + CondBrOp
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: usesDynamic() callee name matching
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: removeExistingModuleFlags()
QIRSetAttributesAndMetadata->>QIRSetAttributesAndMetadata: setMetadata(): write passthrough + ModuleFlagsOp
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mlir/lib/Conversion/QCToQIR/QIRAdaptive/QCToQIRAdaptive.cpp (1)
475-483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation lists removed stage.
The class-level docstring still references "6. Set QIR metadata attributes" which has been removed from the implementation. The stage numbers in the docstring (1-8) don't match the actual implementation stages (1-7).
📝 Suggested fix
* Conversion stages: * 1. Convert scf dialect to cf -* 2. Cpmvert func dialect to LLVM +* 2. Convert func dialect to LLVM * 3. Ensure proper block structure for QIR Adaptive Profile * 4. Add QIR initialization call * 5. Convert QC and memref operations to QIR calls -* 6. Set QIR metadata attributes -* 7. Convert arith and cf dialects to LLVM -* 8. Reconcile unrealized casts +* 6. Convert arith and cf dialects to LLVM +* 7. Reconcile unrealized casts🤖 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/Conversion/QCToQIR/QIRAdaptive/QCToQIRAdaptive.cpp` around lines 475 - 483, The docstring in the QCToQIRAdaptive.cpp file describes eight conversion stages but the implementation only contains seven stages. Update the comment block by removing the line "6. Set QIR metadata attributes" and renumbering the subsequent stage from "7. Convert arith and cf dialects to LLVM" to "6. Convert arith and cf dialects to LLVM" and "8. Reconcile unrealized casts" to "7. Reconcile unrealized casts" to ensure the documented stages match the actual implementation.mlir/lib/Conversion/QCToQIR/QIRBase/QCToQIRBase.cpp (1)
288-296:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClass-level docstring still lists removed metadata stage.
The class documentation lists "5. Set QIR metadata attributes" which has been removed from the implementation. Update the stage list to match the actual 6-stage implementation.
📝 Suggested fix
* Conversion stages: * 1. Convert func dialect to LLVM * 2. Ensure proper block structure for QIR base profile * 3. Add QIR initialization call * 4. Convert QC operations to QIR calls -* 5. Set QIR metadata attributes -* 6. Convert arith and cf dialects to LLVM -* 7. Reconcile unrealized casts +* 5. Convert arith and cf dialects to LLVM +* 6. Reconcile unrealized casts🤖 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/Conversion/QCToQIR/QIRBase/QCToQIRBase.cpp` around lines 288 - 296, The class-level docstring in the conversion function lists a seven-stage conversion process including "5. Set QIR metadata attributes", but this stage has been removed from the actual implementation. Update the docstring to remove stage 5 and renumber the remaining stages so that "Convert arith and cf dialects to LLVM" becomes stage 5 and "Reconcile unrealized casts" becomes stage 6, making the documented process match the actual six-stage implementation.mlir/unittests/Conversion/QCToQIR/QCToQIRAdaptive/test_qc_to_qir_adaptive.cpp (1)
103-114: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit assertions for QIR metadata deduplication.
ProgramEquivalencecleans bothprogramandreferencewithrunQIRCleanupPipeline(..., true). A symmetric regression in cleanup (e.g., duplicatedrequired_num_qubits/required_num_resultson both sides) can still pass equivalence. Add direct post-cleanup assertions that these keys occur exactly once in entry-point passthrough metadata.🤖 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/unittests/Conversion/QCToQIR/QCToQIRAdaptive/test_qc_to_qir_adaptive.cpp` around lines 103 - 114, The test currently relies on ProgramEquivalence to verify both programs match, but symmetric regressions (where duplicated metadata appears on both sides) would still pass equivalence checks. After calling runQIRCleanupPipeline on both program and reference (which occurs after line 103 and before line 114), add explicit assertions to verify that metadata deduplication worked correctly. Specifically, add assertions to check that metadata keys like required_num_qubits and required_num_results appear exactly once in the entry-point passthrough metadata for both the program and reference objects to catch any symmetric metadata duplication issues.
🤖 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/lib/Dialect/QIR/Transforms/AttachQIRAttributes.cpp`:
- Around line 65-70: The runOnOperation method calls getMainFunction which can
return nullptr when no function with the entry_point attribute exists, but does
not check for null before passing the result to setMetadata, getAdaptive, or
getBase. Add a null check on the main variable immediately after the
getMainFunction call and return early from runOnOperation if main is nullptr to
prevent null pointer dereference.
- Around line 374-375: Fix the Doxygen comment for the getAdaptive method which
currently incorrectly states "QIR base profile compliant program" but should
state "QIR adaptive profile compliant program" since the method name and
functionality are related to the adaptive profile, not the base profile. Update
the comment text to accurately reflect that this method returns metadata for the
adaptive profile.
- Around line 247-255: In the classifyCondBrOp function, the call to
condition.getDefiningOp() can return nullptr when the condition is a block
argument rather than an operation result, and passing nullptr to dyn_cast will
cause a crash. Add a null check on the result of getDefiningOp() before
attempting the dyn_cast to LLVM::CallOp, and if it is null, handle that case
appropriately (the existing comment suggests returning true for non-measurement
conditions).
---
Outside diff comments:
In `@mlir/lib/Conversion/QCToQIR/QIRAdaptive/QCToQIRAdaptive.cpp`:
- Around line 475-483: The docstring in the QCToQIRAdaptive.cpp file describes
eight conversion stages but the implementation only contains seven stages.
Update the comment block by removing the line "6. Set QIR metadata attributes"
and renumbering the subsequent stage from "7. Convert arith and cf dialects to
LLVM" to "6. Convert arith and cf dialects to LLVM" and "8. Reconcile unrealized
casts" to "7. Reconcile unrealized casts" to ensure the documented stages match
the actual implementation.
In `@mlir/lib/Conversion/QCToQIR/QIRBase/QCToQIRBase.cpp`:
- Around line 288-296: The class-level docstring in the conversion function
lists a seven-stage conversion process including "5. Set QIR metadata
attributes", but this stage has been removed from the actual implementation.
Update the docstring to remove stage 5 and renumber the remaining stages so that
"Convert arith and cf dialects to LLVM" becomes stage 5 and "Reconcile
unrealized casts" becomes stage 6, making the documented process match the
actual six-stage implementation.
In
`@mlir/unittests/Conversion/QCToQIR/QCToQIRAdaptive/test_qc_to_qir_adaptive.cpp`:
- Around line 103-114: The test currently relies on ProgramEquivalence to verify
both programs match, but symmetric regressions (where duplicated metadata
appears on both sides) would still pass equivalence checks. After calling
runQIRCleanupPipeline on both program and reference (which occurs after line 103
and before line 114), add explicit assertions to verify that metadata
deduplication worked correctly. Specifically, add assertions to check that
metadata keys like required_num_qubits and required_num_results appear exactly
once in the entry-point passthrough metadata for both the program and reference
objects to catch any symmetric metadata duplication issues.
🪄 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: bbb22f46-55eb-405d-af7d-6e976a1662b3
📒 Files selected for processing (21)
CHANGELOG.mdmlir/include/mlir/Conversion/QCToQIR/QIRCommon/QIRCommon.hmlir/include/mlir/Dialect/QIR/Builder/QIRProgramBuilder.hmlir/include/mlir/Dialect/QIR/Transforms/Passes.tdmlir/include/mlir/Dialect/QIR/Utils/QIRMetadata.hmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.hmlir/include/mlir/Support/Passes.hmlir/lib/Compiler/CMakeLists.txtmlir/lib/Compiler/CompilerPipeline.cppmlir/lib/Conversion/QCToQIR/QIRAdaptive/QCToQIRAdaptive.cppmlir/lib/Conversion/QCToQIR/QIRBase/QCToQIRBase.cppmlir/lib/Conversion/QCToQIR/QIRCommon/QIRCommon.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/QIR/Transforms/AttachQIRAttributes.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/lib/Support/IRVerification.cppmlir/lib/Support/Passes.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Conversion/QCToQIR/QCToQIRAdaptive/test_qc_to_qir_adaptive.cppmlir/unittests/Conversion/QCToQIR/QCToQIRBase/test_qc_to_qir_base.cppmlir/unittests/Dialect/QIR/IR/test_qir_ir.cpp
💤 Files with no reviewable changes (4)
- mlir/include/mlir/Dialect/QIR/Utils/QIRMetadata.h
- mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
- mlir/lib/Conversion/QCToQIR/QIRCommon/QIRCommon.cpp
- mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp
…h-quantum-toolkit/core into feat/set-qir-metadata-pass
burgholzer
approved these changes
Jun 17, 2026
burgholzer
left a comment
burgholzer
left a comment
Member
There was a problem hiding this comment.
This looks very clean! Nice work.
I just have one minor question and a typo left here.
Otherwise, this is good to go!
Comment on lines
+138
to
+147
| /// Remove existing module flag operations from module. | ||
| /// Note that this might also erase non-QIR module flag operations, but for | ||
| /// now, we assume that there are no others. | ||
| static void removeExistingModuleFlags(ModuleOp m, IRRewriter& rewriter) { | ||
| SmallVector<Operation*> flagOps; | ||
| m->walk([&](LLVM::ModuleFlagsOp op) { flagOps.emplace_back(op); }); | ||
| for (Operation* op : llvm::make_early_inc_range(flagOps)) { | ||
| rewriter.eraseOp(op); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Does this have to be a two-step process? Isn't there some kind of functionality that would simply erase all LLVM::ModuleFlagsOp?
| return seen.size(); | ||
| } | ||
|
|
||
| /// Determine whether an loop (as a set of blocks) is an iterative loop (true) |
Member
There was a problem hiding this comment.
Suggested change
| /// Determine whether an loop (as a set of blocks) is an iterative loop (true) | |
| /// Determine whether a loop (as a set of blocks) is an iterative loop (true) |
burgholzer
added
usability
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
2 participants
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.
Changes
QIRSetAttributesAndMetadatapass which sets the meta data of the QIR entry point function. Resolves 🐛 Invalid / Duplicated Metadata Produced By QIR Builder / Translation #1777.QIRMetadata.hinto the new pass file.qirbuilder.finalize()and after the cleanup pipeline.Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.