Conversation
MatthiasReumann
added
c++
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a QCO qubit state tracker Changes
Sequence Diagram(s)sequenceDiagram
participant Commit as commit()
participant WalkUnit as walkUnit(Region)
participant TypeSwitch as TypeSwitch
participant Qubits as Qubits
participant IR as IR
Commit->>WalkUnit: traverse region using anchors
loop per operation
WalkUnit->>TypeSwitch: dispatch op kind
alt StaticOp
TypeSwitch->>Qubits: add(q, static_index)
else AllocOp
TypeSwitch->>Qubits: add(new_q)
else UnitaryOpInterface
TypeSwitch->>Qubits: remap(inputs → outputs)
else ResetOp/MeasureOp
TypeSwitch->>Qubits: remap(in → out)
else DeallocOp
TypeSwitch->>Qubits: remove(q)
end
WalkUnit->>IR: insert SWAPs at anchor (if required)
WalkUnit->>Qubits: update mappings
end
WalkUnit-->>Commit: SWAPs inserted, Qubits updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/lib/Dialect/QCO/Utils/Driver.cpp`:
- Around line 32-44: Add a defensive assertion in Qubits::remap to ensure prev
is tracked before calling valueToIndex_.lookup: check
valueToIndex_.contains(prev) (or find(prev) != end()) and assert/fail if not,
mirroring the existence check in remove(); then proceed with the existing
lookup/erase/try_emplace logic so you never operate on a default-constructed
pair when prev is missing.
🪄 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: 48039b74-0489-42d4-b2bf-b05e276017ea
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QCO/Utils/Driver.cpp
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/lib/Dialect/QCO/Utils/Driver.cpp`:
- Around line 21-30: The add methods in Qubits
(Qubits::add(TypedValue<QubitType>) and Qubits::add(TypedValue<QubitType>,
std::size_t)) can create duplicate keys and ignore insertion failures; replace
using programToValue_.size() with a monotonic counter (e.g., nextProgramIndex_)
to generate unique program indices and check the return of try_emplace for both
programToValue_/hardwareToValue_ and valueToIndex_ insertions; if any
try_emplace fails, handle it deterministically (assert, throw, or log+abort) to
avoid silent divergence between programToValue_/hardwareToValue_ and
valueToIndex_, and ensure both maps are only updated when both insertions
succeed (or roll back the first on failure).
🪄 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: e1e86a5f-f880-46d3-b476-00564d1b2ba3
📒 Files selected for processing (4)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Utils/Driver.cppmlir/unittests/Dialect/QCO/Utils/test_drivers.cpp
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/Dialect/QCO/Utils/test_drivers.cpp`:
- Line 66: The test currently dereferences
module->getOps<mlir::func::FuncOp>().begin() into auto func without checking the
range; change the code to guard that getOps<mlir::func::FuncOp>() is not empty
before dereferencing (e.g., check begin() != end() or use an iterator and ensure
it != end()), and if empty emit a clear test assertion/failure message
indicating the expected FuncOp was not found; update the variable acquisition
around module and func to only dereference when the iterator is valid.
🪄 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: ab68fab5-c388-45de-8162-545a355225ec
📒 Files selected for processing (1)
mlir/unittests/Dialect/QCO/Utils/test_drivers.cpp
|
@burgholzer Any idea why the coverage test fails even though there is a unit test which calls the "missed" line functions? 🤔 |
Haven't looked too deeply yet. Is the test executed? |
Sorry, I'm an idiot. Forgot to commit and push the |
Always glad to help 😉 |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This looks pretty clean to me.
Some of the comments are more general questions. Most actual feedback is on small details.
Should be pretty quick to address.
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/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 743-745: The function declaration for place is annotated with
[[nodiscard]] but returns void; remove the [[nodiscard]] attribute from the
place function declaration/definition to avoid meaningless attribute warnings.
Locate the static function named place(ArrayRef<QubitValue>, const Layout&,
Region&, IRRewriter&) in Mapping.cpp and delete the [[nodiscard]] token from its
signature so the function is a plain void return without the attribute.
🪄 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: 36d817e0-4d5a-401e-b666-2eb64d69bc07
📒 Files selected for processing (2)
mlir/include/mlir/Dialect/QCO/Utils/Drivers.hmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 765-769: In commit(ArrayRef<SmallVector<IndexGate>> swaps,
ArrayRef<Operation*> anchors, Region& funcBody, IRRewriter& rewriter) add a
defensive assertion at the start that swaps.size() == anchors.size() with a
clear message; this ensures the function invariant (both arrays derived from the
same layering) is validated before using anchorIt/swapIt and will fail fast if
they diverge. Reference the commit function and the swaps and anchors parameters
when adding the check.
- Around line 752-759: The synthesized StaticOp and its DeallocOp are created
with rewriter.getUnknownLoc(), which loses traceability; change the creation to
use a descriptive location (e.g., the function's location or a fused location
combining the function and current insertion point). Specifically, in the loop
that iterates from dynQubits.size() to layout.nqubits() where you call
StaticOp::create(...) and DeallocOp::create(...), replace
rewriter.getUnknownLoc() with a meaningful MLIR Location (for example use
func.getLoc() or rewriter.getFusedLoc({func.getLoc(), someOp.getLoc()})) so both
StaticOp::create and DeallocOp::create carry that location for better debugging
and IR inspection. Ensure you import/derive the location value before the create
calls and pass it into both create invocations.
🪄 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: dd9e62f8-9750-4e8d-a372-3b0b898e38b6
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for the additional changes. LGTM 👍🏼
2 participants
Description
This pull request adds a utility function
walkUnitwhich traverses a quantum IR top down (analogously to the built-in walk functions of MLIR) and maintains aQubitsobject that keeps track of the current SSA values. We use this function and the datastructure to solve the SSA Dominance issues once and for all by inserting SWAPs from a textual perspective (instead of a circuit def-use one).Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.