Skip to content

Add SCF support to WireIterator#1806

Open
MatthiasReumann wants to merge 6 commits into
mainfrom
enh/wire-iterator-scf-ops
Open

Add SCF support to WireIterator#1806
MatthiasReumann wants to merge 6 commits into
mainfrom
enh/wire-iterator-scf-ops

Conversation

@MatthiasReumann

@MatthiasReumann MatthiasReumann commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Changes

✨Add SCF op switch cases in WireIterator.cpp and add unit tests

(Extracted from #1805 to decrease that PR size and ease reviewing)

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.

If PR contains AI-assisted content:

  • I have disclosed the use of AI tools in the PR description as per our AI Usage Guidelines.
  • AI-assisted commits include an Assisted-by: [Model Name] via [Tool Name] footer.
  • I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it.

@MatthiasReumann MatthiasReumann self-assigned this Jun 23, 2026
@MatthiasReumann MatthiasReumann added enhancement Improvement of existing feature c++ Anything related to C++ code MLIR Anything related to MLIR labels Jun 23, 2026
@MatthiasReumann MatthiasReumann added this to the MLIR Support milestone Jun 23, 2026
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mlir/lib/Dialect/QCO/Utils/WireIterator.cpp 92.3% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@MatthiasReumann

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@MatthiasReumann MatthiasReumann marked this pull request as ready for review June 23, 2026 13:28
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

WireIterator gains a isFinal_ state flag and two static helpers (isSinkLikeOperation, isSourceLikeOperation) to centralize boundary detection. forward() and backward() are refactored to use these helpers and to map tied results for scf::ForOp, scf::WhileOp, and qco::IfOp via TypeSwitch. Unit tests are updated with an SCF-based circuit and a new block-argument traversal subtest.

Changes

WireIterator SCF traversal refactor

Layer / File(s) Summary
WireIterator header: isFinal_ flag and helper declarations
mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h
Adds mlir/IR/Value.h include, initializes isFinal_ = false in both constructors, and extends the private section with isSinkLikeOperation/isSourceLikeOperation static declarations and the bool isFinal_ data member.
WireIterator implementation: helpers, forward, and backward
mlir/lib/Dialect/QCO/Utils/WireIterator.cpp
Introduces isSinkLikeOperation and isSourceLikeOperation; updates qubit() to use the former; reworks forward() so the sink-like boundary sets isFinal_ (not isSentinel_) and the next step promotes to sentinel, with a TypeSwitch for scf::ForOp/scf::WhileOp/qco::IfOp tied results; reworks backward() with symmetric sentinel→isFinal_ transition, isSinkLikeOperation-based detection, the same TypeSwitch for tied-init operand resolution, fatal error paths, and isFinal_ = false reset.
Unit tests: SCF circuit and block-argument traversal
mlir/unittests/Dialect/QCO/Utils/test_wireiterator.cpp
Adds SCF headers and scf::SCFDialect; replaces the flat test circuit with an scf.for + qco.if qubit-carrying circuit and updates forward/backward traversal assertions; adds a new subtest verifying iteration from a loop block argument through internal ops, scf.yield, sentinel, and reverse.
CHANGELOG: reference PR #1806
CHANGELOG.md
Adds #1806 to the Unreleased/Added dialect infrastructure bullet and its link definition.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • munich-quantum-toolkit/core#1510: Introduced the mlir::qco::WireIterator class and its initial test_wireiterator.cpp; this PR directly extends that implementation.
  • munich-quantum-toolkit/core#1569: Modified WireIterator terminal-operation handling (switching from DeallocOp to SinkOp), overlapping the same boundary-detection code refactored here.
  • munich-quantum-toolkit/core#1674: Updated forward()/backward() traversal and qubit() sentinel handling in WireIterator.cpp, directly adjacent to the changes in this PR.

Suggested reviewers

  • burgholzer

Poem

🐇 Hop through the qubit wire with glee,
Past scf.for loops and qco.if — wheee!
isFinal_ marks where the wire must end,
Then backward we bounce around every bend.
No inline isa<> lists left to dread,
Clean helper methods lead forward instead! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding SCF support to WireIterator, which aligns with all the substantive changes in the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR provides a clear summary of changes and completes all checklist items, though some sections lack detail.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch enh/wire-iterator-scf-ops

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/QCO/Utils/WireIterator.cpp`:
- Around line 134-145: In the backward() method's qco::IfOp case, fix the mixed
iterator sources by replacing the llvm::find and std::distance calls with
getResults().begin() and getResults().end() to match the forward() method's
pattern for the same qco::IfOp case. Correct the assertion's result_end() call
from arrow notation (op->result_end()) to dot notation (op.result_end()) to
properly access the Operation members. Finally, update the error message in the
llvm::reportFatalInternalError call from "expected scf.for result for tied init
lookup" to reference qco.if instead of scf.for.
🪄 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: 8f75e50f-3377-4108-8602-ca040e5d2c9e

📥 Commits

Reviewing files that changed from the base of the PR and between 4065349 and 43b7539.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h
  • mlir/lib/Dialect/QCO/Utils/WireIterator.cpp
  • mlir/unittests/Dialect/QCO/Utils/test_wireiterator.cpp

Comment thread mlir/lib/Dialect/QCO/Utils/WireIterator.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Anything related to C++ code enhancement Improvement of existing feature MLIR Anything related to MLIR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant