Add WireIterator for the QCO Dialect#1510
Conversation
MatthiasReumann
added
feature
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h`:
- Around line 28-41: The default constructor WireIterator() is marked explicit
which blocks brace or copy-list initialization (e.g., WireIterator it = {});
remove the explicit specifier from the default ctor definition so WireIterator()
is a conventional default-constructible iterator; update the declaration of
WireIterator::WireIterator() (and any matching implementation) to be
non-explicit while keeping the other constructor explicit as-is.
- Line 152: The file ends the namespace with an extraneous semicolon after the
closing brace for namespace mlir::qco in WireIterator.h; remove the trailing
semicolon following the closing brace so the namespace is closed as "}" only
(i.e., edit the namespace closing line associated with mlir::qco in
WireIterator.h to drop the semicolon).
- Around line 1-12: The header is missing include guards causing
multiple-definition errors for WireIterator and the static_asserts; add a header
guard or a `#pragma` once at the top of the file and the matching `#endif` at the
bottom so the file is only included once. Specifically, wrap the contents that
define WireIterator and the static_assert lines with either a single-line
`#pragma once` at the very top, or a conventional guard (e.g., `#ifndef
MLIR_DIALECT_QCO_UTILS_WIREITERATOR_H` / `#define
MLIR_DIALECT_QCO_UTILS_WIREITERATOR_H` ... `#endif`) to prevent redefinition.
Ensure the chosen macro name is unique and matches file-based naming conventions
and that no other code is accidentally left outside the guard.
- Around line 109-142: The backward() method currently returns early when
qubit_.getDefiningOp() is nullptr, leaving op_==nullptr and causing subsequent
backward() calls to silently stall; modify backward() in WireIterator so that
when op_ is nullptr (qubit_ is a BlockArgument) it treats this as a
begin-sentinel: set isSentinel_ = true (and keep or reset op_ as nullptr) and
return, so further decrements are recognized as at-begin rather than stuck;
update any comments to reflect that nullptr == begin-sentinel and reference the
symbols backward(), qubit_.getDefiningOp(), op_, and isSentinel_.
In `@mlir/unittests/Dialect/QCO/Utils/test_wireiterator.cpp`:
- Around line 156-260: Add a short inline comment near the assertions that use
q04.getUsers().begin() (and any loops that rely on the dealloc user) explaining
that builder.finalize() implicitly inserts qco.dealloc for remaining static
qubits; mention finalize() by name and that the test relies on that
auto-deallocation rather than an explicit builder.dealloc(...) call so readers
understand why a DeallocOp user exists for q04.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h`:
- Around line 1-21: The header currently includes an unused include
"<llvm/Support/Debug.h>" in WireIterator.h; remove that include line so the file
no longer depends on Debug.h (keep other includes like QCODialect.h,
TypeSwitch.h, ErrorHandling.h, Operation.h, and LLVM.h intact) and run a quick
build to ensure nothing else in WireIterator.h used LLVM_DEBUG/dbgs/DEBUG_TYPE.
- Line 89: The conditional uses separate mlir::isa checks combined with ||;
replace them with the variadic form for conciseness and consistency: change the
condition around op_ (currently using mlir::isa<qco::AllocOp>(op_) ||
mlir::isa<qco::StaticOp>(op_)) to use mlir::isa<qco::AllocOp,
qco::StaticOp>(op_), and make the analogous replacement for the second
occurrence referenced near the later check (around Line 128) in the same
WireIterator-related code so both spots use the variadic isa.
In `@mlir/unittests/Dialect/QCO/Utils/test_wireiterator.cpp`:
- Around line 48-260: Add tests that iterate the second/target wire and start
from a mid-chain SSA value: create assertions walking the q10 wire through the
CX to q11 and its dealloc (verifying WireIterator.operation() and .qubit() for
q10, CX op, q11 and dealloc) and add a WireIterator constructed from the
interior value q02 to exercise forward and backward traversal from a mid-wire
start; also assert that the iterator's resolution utilities
(getOutputForInput/getInputForOutput or equivalent behavior in
qco::WireIterator) return the correct target qubit mapping when crossing CX
(i.e., ensure the iterator resolves the target, not only the control).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h`:
- Around line 40-46: The qubit() method calls mlir::isa<qco::DeallocOp>(op_)
without checking op_ for nullptr, which is UB when the iterator is
default-constructed or when backward() has set op_ to nullptr; update qubit() to
first check if op_ is nullptr and return nullptr in that case, then perform the
isa check and return qubit_ as before (i.e., guard the
mlir::isa<qco::DeallocOp>(op_) call with an if (op_ == nullptr) return nullptr;
before the existing dealloc handling).
In `@mlir/unittests/Dialect/QCO/Utils/test_wireiterator.cpp`:
- Line 64: The local variable module returned by builder.finalize() is unused
and triggers warnings; mark it as intentionally unused by either annotating the
declaration with [[maybe_unused]] (e.g., [[maybe_unused]] auto module =
builder.finalize();) or immediately cast it to void (e.g., (void)module;) to
silence the warning while keeping the module object alive; update the
declaration in test_wireiterator.cpp where builder.finalize() is called.
- Line 17: Remove the unused include for llvm/Support/Debug.h in
test_wireiterator.cpp: the file does not reference any symbols from Debug.h (no
LLVM_DEBUG, dbgs(), or DEBUG_TYPE), so delete the line '#include
<llvm/Support/Debug.h>' to avoid an unnecessary dependency and keep includes
minimal.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Hey @MatthiasReumann 👋🏼
Thanks for the quick action on this! Great to see. I only have some minor comments here.
I was wondering whether we could already use the wire iterator somewhere, e.g., the QCO builder, so that it doesn't stand as alone as it does right now. But it is not clear to me whether that is even reasonable. A short comment would be appreciated.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: matthias <matthias@bereumann.com>
|
@burgholzer Thanks for the review. I've changed the necessary bits. Regarding using the WireIterator: I've quickly glanced over the |
Nice, that was quick. |
|
Hey @burgholzer! I've merged main into this one. I think I adapted the CMakeLists.txt accordingly. Maybe there is some alternative way to integrate these tests into the bigger ones - just let me know if you have any thoughts! |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Two small requests and a comment, then this should be good to go 🚀
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
LGTM with two minor comments.
2 participants
Description
This pull request migrates the
WireIteratorto the new QCO dialect. Unit-Tests ensure the correctness of the implementation.Note
Because we haven't properly defined yet how we want to handle structured control flow and other pull requests tackling this issue (such as #1506) are still open, the current
WireIteratorimplementation only supports quantum operations.Checklist: