♻️ Apply CRTP to Unit class #1379
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUnit was converted to a CRTP template and LayeredUnit/SequentialUnit now inherit from Changes
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 (8)
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.h(2 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.h(1 hunks)mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Unit.h(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp(3 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp(3 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp(1 hunks)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/RoutingVerificationPass.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cppmlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.hmlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Unit.hmlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.hmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cpp
📚 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/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cpp
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cppmlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp
📚 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/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.hmlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Unit.hmlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.h
🧬 Code graph analysis (4)
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.h (3)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp (6)
SequentialUnit(42-54)fromEntryPointFunction(32-40)fromEntryPointFunction(33-34)layout(35-35)nextImpl(56-79)nextImpl(56-56)mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h (7)
nodiscard(34-115)nodiscard(58-62)nodiscard(69-73)nodiscard(136-141)nodiscard(160-164)nodiscard(183-187)nodiscard(206-208)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp (5)
fromEntryPointFunction(135-142)fromEntryPointFunction(135-136)layout(137-137)nextImpl(275-298)nextImpl(275-275)
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.h (2)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp (6)
LayeredUnit(144-273)fromEntryPointFunction(135-142)fromEntryPointFunction(135-136)layout(137-137)nextImpl(275-298)nextImpl(275-275)mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h (7)
nodiscard(34-115)nodiscard(58-62)nodiscard(69-73)nodiscard(136-141)nodiscard(160-164)nodiscard(183-187)nodiscard(206-208)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp (2)
mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.h (1)
LayeredUnit(46-72)mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp (4)
layout(35-35)nextImpl(56-79)nextImpl(56-56)forLayout(64-64)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp (1)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp (10)
layout(137-137)nextImpl(275-298)nextImpl(275-275)forLayout(283-283)op(52-52)op(52-52)op(55-59)op(55-55)op(62-75)op(62-63)
🔇 Additional comments (10)
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/AStarRoutingPass.cpp (1)
168-172: LGTM!The unconditional SWAP insertion at
YieldOpcorrectly usesrewriter.setInsertionPoint(op), which is the preferred approach per retrieved learnings. The reversed history properly restores the qubit mapping.mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp (2)
144-145: LGTM!Constructor correctly initializes the CRTP base class with simplified signature, removing the
restoreflag.
275-298: LGTM!The
nextImpl()implementation correctly follows the CRTP pattern and aligns withSequentialUnit::nextImpl(). Child unit construction consistently uses the simplified two-argument constructor without the restore flag.mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Unit.h (1)
20-50: Clean CRTP implementation.The template-based Unit class correctly applies CRTP with:
- Public interface methods (
next(),begin(),end()) delegating to derived*Impl()methods- Protected constructor preventing direct instantiation
- Protected members accessible to derived classes
This design eliminates virtual table overhead while maintaining a clean interface, consistent with LLVM patterns.
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.cpp (2)
42-54: LGTM!Constructor correctly initializes the CRTP base class and removes the
restoreparameter, aligning with the broader refactor.
56-79: The two-argument constructor is correctly defined and delegating.The SequentialUnit class has an overloaded two-argument constructor in the header file that delegates to the three-argument constructor, defaulting the start iterator to
region->op_begin(). This explains why lines 68, 71-72, and 74 can construct units with only layout and region arguments. The implementation is correct.mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.h (2)
46-51: CRTP inheritance and simplified constructor look good.The CRTP pattern is correctly applied—
LayeredUnitinherits fromUnit<LayeredUnit>, enabling static polymorphism without virtual dispatch overhead. Removing therestoreflag aligns with the PR objective of moving restoration logic to the routing pass level.
62-71: Private CRTP machinery is well-structured.The friend declaration properly grants
Unit<LayeredUnit>access to the private*Impl()methods. MovingLayersandconst_iteratortype aliases to private scope is appropriate since external code should use the public interface exposed by the CRTP base class.mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/SequentialUnit.h (2)
25-34: CRTP inheritance and constructor design are appropriate.The CRTP pattern mirrors
LayeredUnitfor consistency. Retaining the 3-parameter constructor is correct—SequentialUnit::nextImpl()needs to create child units starting from arbitrary iterator positions within a region. The delegating 2-parameter constructor provides a clean convenience API for the entry-point case.
36-44: Private CRTP machinery follows the established pattern.The friend declaration and
*Impl()methods are consistent withLayeredUnit, making the two unit types symmetric in their CRTP structure. The return typemlir::Region::OpIteratorfrombeginImpl()/endImpl()correctly matches the sequential traversal semantics.
mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp
Show resolved
Hide resolved
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.
Nice! Just one clarification question on the CRTP implementation.
Description
This pull requests updates the unit implementations such that the base class
Unitis an actual interface. For that purpose, I implemented the Curiously Recurring Template Pattern (CRTP) to avoid virtual tables. This pattern is commonly used in the LLVM code base so it is also a good fit here, I think.I've also removed the
restoreflag from the Unit because the routing pass already knows when to restore and when not to. It didn't really serve any purpose in the unit itself.Rationale
While I was writing the respective section for my thesis, I noticed that the
Unitclass is semantically an interface - but the implementation isn't. Hence, this PR. I think units could be useful elsewhere also, with potentially different strategies, so refactoring the class to an interface makes sense imo.Checklist: