Skip to content

✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix#1850

Open
simon1hofmann wants to merge 8 commits into
mainfrom
compose-n-target-body-matrix
Open

✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix#1850
simon1hofmann wants to merge 8 commits into
mainfrom
compose-n-target-body-matrix

Conversation

@simon1hofmann

@simon1hofmann simon1hofmann commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Description

Extend InvOp and CtrlOp::getUnitaryMatrix() to compose multi-operation modifier bodies on N ≥ 1 target wires, not only single-qubit sequences on one target.

Replace composeSingleQubitBodyMatrix with composeNTargetBodyMatrix(Block&, numTargets), which:

  • Maps block arguments to wire indices 0..N-1 (MSB-first, matching Matrix2x2::embedInNqubit)
  • Walks the body in program order, tracking SSA wires through barriers and unitary ops
  • Embeds each 1Q/2Q compile-time unitary into a 2^N DynamicMatrix and accumulates via premultiplyBy
  • Handles GPhaseOp as a global phase factor
  • Rejects bodies with >2-qubit ops, unknown qubit-touching ops, or numTargets > 10

InvOp returns the adjoint of the composed matrix; CtrlOp wraps it with the existing I_{2^c} ⊗ U controlled embed.

Also adds DynamicMatrix::premultiplyBy (const-ref and rvalue overloads) to support efficient in-place accumulation.

Prerequisite for #1812

AI Assistance

Used Composer 2.5 via Cursor for parts of this change. I reviewed the full
diff and take responsibility for everything in this PR.

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.

@simon1hofmann simon1hofmann changed the title ✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix ✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix Jul 3, 2026
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@simon1hofmann simon1hofmann self-assigned this Jul 3, 2026
@simon1hofmann simon1hofmann added enhancement Improvement of existing feature c++ Anything related to C++ code MLIR Anything related to MLIR labels Jul 3, 2026
@simon1hofmann simon1hofmann added this to the MLIR Support milestone Jul 3, 2026
@simon1hofmann

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR generalizes modifier body matrix composition to N targets, updates Ctrl/Inv modifier unitary computation to use it, adds supporting DynamicMatrix premultiplication helpers, and expands tests plus changelog entries.

Changes

N-target composition feature

Layer / File(s) Summary
Header contracts
mlir/include/mlir/Dialect/QCO/QCOUtils.h, mlir/include/mlir/Dialect/QCO/Utils/Matrix.h
Declares kMaxModifierTargetQubits, composeNTargetBodyMatrix, and the new DynamicMatrix::premultiplyBy(const DynamicMatrix&) plus embedded premultiply declarations.
Composition and matrix helpers
mlir/lib/Dialect/QCO/IR/QCOUtils.cpp, mlir/lib/Dialect/QCO/Utils/Matrix.cpp
Implements N-target body composition with wire mapping, barrier/global-phase handling, and unitary embedding, and defines the DynamicMatrix premultiplication helpers.
CtrlOp and InvOp wiring
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp, mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp
Switches composed-body unitary computation to composeNTargetBodyMatrix for both controlled and inverse modifiers.
Tests and changelog
mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp, mlir/unittests/Dialect/QCO/IR/CMakeLists.txt, mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp, CHANGELOG.md
Adds inverse/composition test coverage, DynamicMatrix premultiply tests, test target formatting changes, and the changelog update.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: burgholzer

Poem

I’m a rabbit with matrices bright,
Hopping through qubits left and right.
Barriers shift, inverses sing,
Premultiply makes the whole board swing.
N targets now dance in one line—
🐇✨ a tidy quantum alignment.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the core change: composing multi-operation modifier bodies on N targets in getUnitaryMatrix.
Description check ✅ Passed The description includes summary, context, dependency note, AI disclosure, and checklist, matching the template well.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch compose-n-target-body-matrix

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: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp (1)

244-257: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Missing ASSERT_TRUE(matrix) before dereference.

Unlike every other test in this file using the same getUnitaryMatrix()std::optional<DynamicMatrix> pattern (e.g. ControlledHOpMatrix, ControlledXHOpMatrix, ControlledInverseHTOpMatrix), CXOpMatrix dereferences matrix->isApprox(...) without first asserting matrix is engaged. std::optional::operator-> does not check engagement, so an unexpected nullopt here would be UB rather than a clear test failure.

🛡️ Proposed fix
   const auto matrix = firstCtrlOp(*moduleOp).getUnitaryMatrix();
+  ASSERT_TRUE(matrix);
 
   const Matrix4x4 expected =
🤖 Prompt for 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.

In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp` around lines 244 - 257,
The CXOpMatrix test is missing an explicit engagement check before dereferencing
the optional returned by getUnitaryMatrix(). In QCOMatrixTest::CXOpMatrix, add
an ASSERT_TRUE on the matrix result before calling matrix->isApprox(...),
matching the pattern used by the other unit tests in this file such as
ControlledHOpMatrix and ControlledXHOpMatrix. This keeps the test from
dereferencing a disengaged std::optional and makes failures clear.
🤖 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/IR/QCOUtils.cpp`:
- Around line 33-80: Add brief Doxygen-style `///` comments for the new internal
helpers `lookupWireId`, `propagateWireIds`, and `embedUnitaryInBody` in
`QCOUtils.cpp`, matching the style used by sibling static helpers like
`entryIsApprox` and `checkedDim`. Keep the comments short but descriptive enough
to explain each helper’s role so the file complies with the C++ Doxygen
guideline.
- Around line 82-154: The current composeNTargetBodyMatrix path in QCOUtils.cpp
scales poorly because each UnitaryOpInterface is embedded into a full
DynamicMatrix and then premultiplyBy does a dense multiply, which becomes cubic
near the kMaxModifierTargetQubits cap. Update composeNTargetBodyMatrix and its
unitary handling so 1Q/2Q gates apply directly to the accumulator using
index-based stride updates instead of building a full embedded matrix, while
keeping the existing BarrierOp, GPhaseOp, and wireIds propagation behavior
intact.

---

Outside diff comments:
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp`:
- Around line 244-257: The CXOpMatrix test is missing an explicit engagement
check before dereferencing the optional returned by getUnitaryMatrix(). In
QCOMatrixTest::CXOpMatrix, add an ASSERT_TRUE on the matrix result before
calling matrix->isApprox(...), matching the pattern used by the other unit tests
in this file such as ControlledHOpMatrix and ControlledXHOpMatrix. This keeps
the test from dereferencing a disengaged std::optional and makes failures clear.
🪄 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: e420ba71-f155-4491-92ce-20bacefe05ec

📥 Commits

Reviewing files that changed from the base of the PR and between 26a0d98 and 0c4463e.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • mlir/include/mlir/Dialect/QCO/QCOUtils.h
  • mlir/include/mlir/Dialect/QCO/Utils/Matrix.h
  • mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp
  • mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp
  • mlir/lib/Dialect/QCO/IR/QCOUtils.cpp
  • mlir/lib/Dialect/QCO/Utils/Matrix.cpp
  • mlir/unittests/Dialect/QCO/IR/CMakeLists.txt
  • mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp
  • mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp

Comment thread mlir/include/mlir/Dialect/QCO/Utils/Matrix.h
Comment thread mlir/lib/Dialect/QCO/IR/QCOUtils.cpp
Comment thread mlir/lib/Dialect/QCO/IR/QCOUtils.cpp
@simon1hofmann

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@simon1hofmann

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@simon1hofmann simon1hofmann marked this pull request as ready for review July 3, 2026 15:32
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