🐛 shortestSWAPsBetween returns one SWAP too many

[MatthiasReumann]

Description

#1301 introduced the shortestSWAPsBetween method to the Architecture class. Unfortunately, there is a bug: It returns one SWAP too many. Semantically, this is still correct but unnecessary since the qubits are already adjacent. In other words, the last SWAP swaps already adjacent qubits.

Checklist:

• The pull request only contains commits that are focused and relevant to this change.
• I have added appropriate tests that cover the new/changed functionality.
• I have updated the documentation to reflect these changes.
• I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.
• I have added migration instructions to the upgrade guide (if needed).
• The changes follow the project's style guidelines and introduce no new warnings.
• The changes are fully tested and pass the CI checks.
• I have reviewed my own code changes.

[MatthiasReumann]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Corrected SWAP sequence reconstruction in qubit routing so the generated swap path is built in the correct order, improving routing accuracy and potentially yielding more efficient quantum circuit layouts.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

The path-reconstruction loop in Architecture::shortestSWAPsBetween now initializes curr from prev_[u][v] instead of v, inserts the SWAP pair (last, curr) before updating curr inside the loop, and adds a clarifying comment about inserting SWAP(last, curr).

Changes

Cohort / File(s)	Summary
Path-reconstruction loop logic mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp	Initialize curr from prev_[u][v] (was v); in loop append SWAP pair (last, curr) before updating curr; add comment Insert SWAP(last, curr)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify correctness of path reconstruction invariant with prev_ semantics.
• Check that SWAP sequence ordering matches expected mapping transforms.
• Ensure unit/integration tests cover affected path shapes (length-1, cycles, and longer paths).

Poem

🐇 I hop along the prev_ trail,

Curr now starts where stories sail;
I pair each swap before I leap,
Path rebuilt—no secrets keep. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the bug being fixed: the shortestSWAPsBetween method returning one SWAP too many.
Description check	✅ Passed	The PR description covers the issue being fixed with context, and all checklist items are marked complete, indicating thorough preparation.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch bug/naive-router-shortest-swaps-between

---

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bcf557c and add7750.

📒 Files selected for processing (1)

• mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📚 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/Architecture.cpp

📚 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/Architecture.cpp

📚 Learning: 2025-12-08T23:44:39.930Z

Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.

Applied to files:

• mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp

📚 Learning: 2025-12-17T11:32:45.843Z

Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.

Applied to files:

• mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: 🐉 Test / 🐧 ubuntu-24.04-arm
• GitHub Check: 🐉 Test / 🐧 Coverage
• GitHub Check: 🐉 Test / 🍎 macos-15-intel
• GitHub Check: 🐉 Test / 🏁 windows-11-arm
• GitHub Check: 🐉 Test / 🏁 windows-2025
• GitHub Check: 🐉 Test / 🐧 ubuntu-24.04
• GitHub Check: 🐉 Test / 🍎 macos-15

🔇 Additional comments (1)

mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/Architecture.cpp (1)

42-48: LGTM! Bug fix correctly eliminates redundant SWAP.

The changes fix the off-by-one bug by:

1. Initializing curr from prev_[u][v] instead of v (line 42), starting path reconstruction one step earlier
2. Inserting the SWAP before updating curr (lines 45-47), ensuring the loop terminates when qubits become adjacent

For a path u → a → v, the old code would generate SWAPs [(v,a), (a,u)], but after SWAP(v,a), the qubits are already adjacent at positions u and a. The new code correctly returns [(v,a)], eliminating the redundant final SWAP.

The comment on line 45 helpfully clarifies the SWAP insertion point.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!