✨ Add --arch option to MLIR Routing Pass
#1279
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just some very quick drive-by comments because I had some time during taxiing
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Architecture.h
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactors architecture selection from enum-based to string-based ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Pass as MLIR Pass
participant Preflight as preflight()
participant Arch as getArchitecture(llvm::StringRef)
participant Mapper as Mapper / Verifier
User->>Pass: run (with --arch=<name>)
Pass->>Preflight: preflight()
Preflight-->>Pass: success / failure
alt preflight fails
Pass->>User: emit error, signalPassFailure
else preflight succeeds
Pass->>Arch: getArchitecture(archName)
alt arch found
Arch-->>Pass: unique_ptr<Architecture>
Pass->>Mapper: construct with architecture
Mapper-->>User: run transforms / verification (success)
else arch not found
Arch-->>Pass: nullptr
Pass->>User: emit error, signalPassFailure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-09T13:25:36.887ZApplied to files:
🧬 Code graph analysis (2)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/PlacementPass.cpp (3)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingPass.cpp (3)
🔇 Additional comments (8)
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 |
📝 WalkthroughWalkthroughThis pull request transitions architecture specification from compile-time enum-based to runtime string-based configuration across MLIR quantum optimization passes. ArchitectureName enum is removed; passes now accept architecture names as string options; Architecture lookup uses llvm::StringRef; preflight validation ensures required arch parameters are provided. Changes
Sequence DiagramsequenceDiagram
participant Pass as Pass (Placement/Routing/Verification)
participant Preflight as preflight()
participant GetArch as getArchitecture()
participant Context as VerificationContext/<br/>Mapper
Pass->>Preflight: preflight() — validate archName
alt archName missing
Preflight-->>Pass: error + failure
Pass->>Pass: emitError, signal failure
else archName present
Preflight-->>Pass: success
Pass->>GetArch: getArchitecture(archName)
alt valid architecture
GetArch-->>Pass: unique_ptr<Architecture>
Pass->>Context: initialize with arch
Context-->>Pass: ready
else invalid architecture
GetArch-->>Pass: nullptr
Pass->>Pass: emitError, signal failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.md(2 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h(0 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.td(3 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Architecture.h(2 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp(2 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/PlacementPass.cpp(3 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingPass.cpp(3 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp(4 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/basics.mlir(7 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/grover_5.mlir(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/invalid-arch-option.mlir(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/missing-arch-option.mlir(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlir(1 hunks)mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-verification.mlir(1 hunks)
💤 Files with no reviewable changes (1)
- mlir/include/mlir/Dialect/MQTOpt/Transforms/Passes.h
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlirmlir/test/Dialect/MQTOpt/Transforms/Transpilation/basics.mlir
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlir
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlirmlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-verification.mlir
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlir
📚 Learning: 2025-10-07T15:58:19.247Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h:202-214
Timestamp: 2025-10-07T15:58:19.247Z
Learning: In the QMAP routing algorithm (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h), the heuristic function h() intentionally uses a non-admissible heuristic that sums per-gate shortest-path distances. This is a deliberate design choice from the QMAP paper, where admissibility is dropped because locally optimal solutions can negatively affect the overall routing quality. This approach prioritizes global solution quality over guaranteed local optimality.
Applied to files:
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/basics.mlirmlir/test/Dialect/MQTOpt/Transforms/Transpilation/grover_5.mlirmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingPass.cpp
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingPass.cpp
🔇 Additional comments (11)
CHANGELOG.md (2)
14-14: LGTM! Proper changelog attribution.The addition of PR #1279 to the routing algorithm entry correctly tracks this contribution.
223-223: LGTM! PR link correctly added.The PR reference follows the established format and points to the correct location.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/grover_5.mlir (1)
11-14: LGTM! Comprehensive architecture-specific test coverage.The four RUN lines effectively test both architectures (MQTTest and IBMFalcon) with both routing methods (naive and astar), and correctly thread the arch parameter through all three passes in each pipeline.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/basics.mlir (2)
11-14: LGTM! Consistent architecture-specific test invocation.The RUN lines follow the same pattern as grover_5.mlir, providing comprehensive test coverage across architectures and routing methods.
17-368: LGTM! Simplified CHECK-LABEL directives.The consolidation to a single CHECK-LABEL pattern (removing NAIVE-/ASTAR-specific checks) is appropriate since the test validates that both routing methods produce valid output, not that they produce identical output.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-verification.mlir (1)
9-9: LGTM! Architecture parameter added to verification pass.The addition of arch=MQTTest properly tests the architecture-aware verification introduced in this PR.
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Architecture.h (2)
19-19: LGTM! Required include added.The inclusion of llvm/ADT/StringRef.h is necessary for the updated getArchitecture signature.
109-109: LGTM! Proper use of StringRef for string parameter.The transition from enum-based to StringRef-based architecture lookup enables runtime architecture specification from command-line options, which is the core objective of this PR. StringRef is the appropriate LLVM type for string parameters that don't require ownership.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/routing-placement.mlir (1)
9-9: LGTM! Architecture parameter added to placement pass.The addition of arch=MQTTest properly tests the architecture-aware placement introduced in this PR.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/invalid-arch-option.mlir (1)
11-15: LGTM! Good negative test for invalid architecture.This test properly validates that all three passes (placement-sc, route-sc, verify-routing-sc) correctly reject invalid architecture names with a clear error message.
mlir/test/Dialect/MQTOpt/Transforms/Transpilation/missing-arch-option.mlir (1)
11-15: LGTM! Good negative test for missing required parameter.This test properly validates that all three passes require the arch parameter and provide a clear error message when it's missing. This complements the invalid-arch-option.mlir test to provide comprehensive error handling coverage.
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp
Show resolved
Hide resolved
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
LGTM. I like that this marks a first step towards QDMI support.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: matthias <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp
🧬 Code graph analysis (1)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp (3)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingPass.cpp (3)
arch(520-520)preflight(500-517)archName(537-544)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/PlacementPass.cpp (3)
arch(453-453)preflight(430-449)archName(471-478)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp (2)
getArchitecture(88-152)getArchitecture(88-88)
🔇 Additional comments (5)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp (5)
22-33: LGTM: Header additions support the refactoring.The new headers appropriately support the architecture ownership changes (unique_ptr), error reporting enhancements, and move semantics.
50-53: LGTM: Proper ownership semantics with unique_ptr.The refactoring from raw pointer to
std::unique_ptr<Architecture>provides clear ownership semantics and automatic resource management.
227-228: LGTM: Standard MLIR pass pattern.The base class aliasing correctly exposes base constructors for pass option initialization.
244-244: LGTM: Proper move semantics.The
std::move(arch)correctly transfers ownership to theVerificationContext, ensuring the architecture object is properly managed.
284-291: LGTM: Appropriate validation for required option.The
preflight()method correctly validates that the requiredarchoption is provided, with a clear error message. The pattern is consistent with the other passes.
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp
Show resolved
Hide resolved
burgholzer
added
enhancement
Description
This pull request adds an option
--archto the placement, routing, and verification pass which specifies the name of the targeted architecture. I've also updated the interface s.t. thegetArchitecturefunction takes a string instead of an enum to prepare for the eventual use of QDMI. The tests are updated accordingly.Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores