🐛 Fix CtrlOp::getBodyUnitary() for operations with parameters

[taminob]

Description

While debugging test cases in #1426, I finally figured out why CtrlOp::getBodyUnitary() sometimes returned an invalid UnitaryOpInterface.
Whenever an operation has at least one parameter, the first operation in the modifier's body will be a arith.constant and not the unitary operation like it is currently assumed in the function.

This PR resolves this by iterating over all operations in the body and only returning when the first UnitaryOpInterface operation has been found or there is none.
It affects all crx, crzz, cu, ... operations.

Required for #1426

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.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Tests

  • Added new test case for body unitary parameter handling in QCO dialect.

• Chores

  • Updated changelog with PR references.

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

Walkthrough

The pull request updates parameter handling in QC and QCO program builders to use a variantToValue utility function, adds a test case for unitaries with parameters in QCO ctrl operations, and updates the changelog with relevant PR references.

Changes

Cohort / File(s)	Summary
Changelog CHANGELOG.md	Added PR #1464 to the Unreleased Added section alongside MLIR dialect infrastructure PRs and updated GitHub PR link references.
Builder Implementations mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp, mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp	Integrated mlir/Dialect/Utils/Utils.h and systematically replaced direct PARAM usage with variantToValue conversions across parameter-related macros (DEFINE_TARGET_PARAMETER variants), updating lambda closures and operation creation calls to use converted param values.
Tests mlir/unittests/Dialect/QCO/IR/Modifiers/test_qco_ctrl.cpp	Added new test case bodyUnitaryWithParameter validating retrieval of body unitaries from CtrlOp operations with parameterized gates (CRX).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• PR #1465 — Refactors variantToValue to accept Location and templated types, directly corresponding to the utility function updates introduced in these builder files
• PR #1436 — Modifies QCO ctrl builder implementation and API, with overlapping changes to the same builder files

Suggested labels

bug

Suggested reviewers

• burgholzer

Poem

🐰 A hop through the builders, so neat and so bright,
VariantToValue shines with converted delight,
Parameters dance through a unified way,
While tests bloom like clover to brighten the day! 🌱

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The PR description addresses the core issue, provides context from issue #1426, lists completed checklist items, and notes pending documentation/changelog updates. However, it does not include changelog entries despite the task being listed in the checklist as pending and changelog modifications being evident in the changeset.	Add changelog entries documenting this fix in the CHANGELOG.md file, and confirm all checklist items (documentation, changelog, upgrade guide) are completed or explicitly deferred.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the bug fix: resolving an issue with CtrlOp::getBodyUnitary() for operations with parameters. It is specific, concise, and accurately reflects the main change in the PR.

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

---

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!

[burgholzer]

I am not 100% that this is the right solution here.
The IR for parasitized operations shouldn't actually pass verification because the verifier of the QC) CtrlOp asserts that any CtrlOp only contains two operations (the UnitaryOp and the yield). I am surprised that we are not hitting this so far.
I assume that we should be running mlir::verify in the compiler pipeline tests on all circuits constructed by any builders or produced by the compiler.

I would argue that the real fix here is to modify the builders so that they directly insert the constant operations in the highest scope possible.
This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

My request here would be two-fold

• Can we fix the builders instead of iterating over the control op body?
• Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

[taminob]

This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

What's the argument against just moving the parameter into the body? Wouldn't that also simplify optimizations involving the control modifier since it actually is IsolatedFromAbove?

* Can we fix the builders instead of iterating over the control op body?

Do you have an idea how we could define this "highest scope possible"? Because wouldn't that be the ModuleOp, but we probably want it in the FuncOp instead?

* Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

I guess that should be possible, but how do we want to handle that in actual non-test code? Running a verification after every optimization would probably cause a lot of unnecessary overhead. Or do we just assume that the optimizations are all doing valid transformations? Because it could also happen that "optimization 1" will cause an invalid IR which "optimization 2" will transform back into a valid IR but with a different functionality than intended and thus only the final IR check might be insufficient?

[burgholzer]

This might prevent us from ever declaring the CtrlOp as IsolatedFromAbove, which might not be feasible anyway because it was (and needs to be) valid to use a parameter in a controlled rotation gate that was defined outside of the region.

What's the argument against just moving the parameter into the body? Wouldn't that also simplify optimizations involving the control modifier since it actually is IsolatedFromAbove?

There might be multiple uses of the same parameter across operations and we do not want to bury these in (potentially nested) modifier regions.
Parameters might originate from the results of complex computations, so you will never be able to move them into the control regions in every use case.

* Can we fix the builders instead of iterating over the control op body?

Do you have an idea how we could define this "highest scope possible"? Because wouldn't that be the ModuleOp, but we probably want it in the FuncOp instead?

The canonicalization pipeline that we employ as part of the compiler pipeline already employs this "bubbling up as high as possible". You can see that in the various stages of the IR in the tests. This should also give you an indication of where the appropriate place for the constant ops is. I believe it should be the first regions with with IsolatedFromAbove trait (which might be the ModuleOp).
Maybe the trick here is to ensure that canonicalization can pull up the constants originally defined within modifier operations.

* Can we ensure that the IR in our tests is verified (especially in the compiler pipeline tests)?

I guess that should be possible, but how do we want to handle that in actual non-test code? Running a verification after every optimization would probably cause a lot of unnecessary overhead. Or do we just assume that the optimizations are all doing valid transformations? Because it could also happen that "optimization 1" will cause an invalid IR which "optimization 2" will transform back into a valid IR but with a different functionality than intended and thus only the final IR check might be insufficient?

See https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-should-be-valid-before-and-after-each-pass and https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier
This is actually enforced by MLIR by default when running passes with a pass manager.
Which makes me slightly confused whether there actually is something to fix here, or whether the implementation is working as intended.

[taminob]

See https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-should-be-valid-before-and-after-each-pass and https://mlir.llvm.org/getting_started/DeveloperGuide/#ir-verifier
This is actually enforced by MLIR by default when running passes with a pass manager.
Which makes me slightly confused whether there actually is something to fix here, or whether the implementation is working as intended.

Thanks for the links! I need to do some reading and experimenting, but there definitely is something broken here because getUnitaryBody() will try to return a arith::ConstantOp (but since it is dyn_cast<UnitaryOpInterface>, it will return nullptr instead).
Not really relevant to the unitary matrix PR if I simply remove these gates from the test cases, but needs to be fixed anyway, I guess, since that is a rather severe issue in my opinion (and can cause some headache as I can attest 😅).

Edit: I also made sure that the test case I added will fail without my proposed fix, that is why I'm sure it currently is broken

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

[burgholzer]

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

We are currently not seeing them in the compiler pipeline tests, but this could very much be due to us not having a test with controlled rotations (which we should add on that case).
Based on the discussion above I would assume that we don't have such a test yet and if we add one to the compiler pipeline tests we would observe the error.

[taminob]

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added ASSERT_TRUE(mlir::verify(*module).succeeded()); to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

[burgholzer]

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Thanks.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added ASSERT_TRUE(mlir::verify(*module).succeeded()); to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

Yeah. Seams like a bug in the builder to me. Or in our definition of a valid control modifier, but I am heavily leaning to the bug.

@denialhaag could you maybe briefly take a look at this?

[denialhaag]

What would be good is to reduce this PR to a reproducer of the issues you are seeing so that we have them as logs in the CI.

Done.

Thanks.

Edit Nr. 2: Ah, I think I got what you're saying - in the actual code it's not an issue but just the test code because it's running without the verifier at the moment. I'll check out adding the verifier to the broken test case

I added `ASSERT_TRUE(mlir::verify(*module).succeeded());` to the test case with this result, so in practice it should be caught, although the builder probably shouldn't cause something like this since your link above also states "Similarly, the IR after a pass runs should be verifier-valid. If a pass produces IR that fails the verifier then the pass has a bug.".

Yeah. Seams like a bug in the builder to me. Or in our definition of a valid control modifier, but I am heavily leaning to the bug.

@denialhaag could you maybe briefly take a look at this?

I am all but certain that the issue is that we are not resolving the std::variant before the call to CtrlOp::create here:

core/mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp

Lines 227 to 234 in 0089f7d

	QCProgramBuilder& QCProgramBuilder::mc##OP_NAME( \
	const std::variant<double, Value>&(PARAM), ValueRange controls, \
	Value target) { \
	checkFinalized(); \
	CtrlOp::create(*this, controls, \
	[&] { OP_CLASS::create(*this, target, PARAM); }); \
	return *this; \
	}

Instead, the std::variant is resolved here (i.e., inside the control modifier) via the call to variantToValue:

core/mlir/lib/Dialect/QC/IR/Operations/StandardGates/RXOp.cpp

Lines 22 to 26 in 0089f7d

	void RXOp::build(OpBuilder& builder, OperationState& state, Value qubitIn,
	const std::variant<double, Value>& theta) {
	auto thetaOperand = variantToValue(builder, state.location, theta);
	build(builder, state, qubitIn, thetaOperand);
	}

I think we could simply call the variantToValue function also before CtrlOp::create. I will push the necessary fixes to this PR tomorrow!

[burgholzer]

I am all but certain that the issue is that we are not resolving the std::variant before the call to CtrlOp::create here:

core/mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp

Lines 227 to 234 in 0089f7d

	QCProgramBuilder& QCProgramBuilder::mc##OP_NAME( \
	const std::variant<double, Value>&(PARAM), ValueRange controls, \
	Value target) { \
	checkFinalized(); \
	CtrlOp::create(*this, controls, \
	[&] { OP_CLASS::create(*this, target, PARAM); }); \
	return *this; \
	}

Instead, the std::variant is resolved here (i.e., inside the control modifier) via the call to variantToValue:

core/mlir/lib/Dialect/QC/IR/Operations/StandardGates/RXOp.cpp

Lines 22 to 26 in 0089f7d

	void RXOp::build(OpBuilder& builder, OperationState& state, Value qubitIn,
	const std::variant<double, Value>& theta) {
	auto thetaOperand = variantToValue(builder, state.location, theta);
	build(builder, state, qubitIn, thetaOperand);
	}

I think we could simply call the variantToValue function also before CtrlOp::create. I will push the necessary fixes to this PR tomorrow!

Indeed, that seems to be the main issue and a nice solution to the issue! Thanks for quickly getting back to this.

[burgholzer]

This looks good now. Thanks @denialhaag 🙏
The last commit should be reverted though as it will trigger warnings in the IDE again.
These are the parameter names of the underlying methods. The "ods" is part of that. If removed, the linter complains that the definition uses different names from the declaration

[denialhaag]

The last commit should be reverted though as it will trigger warnings in the IDE again.

Done!

These are the parameter names of the underlying methods. The "ods" is part of that. If removed, the linter complains that the definition uses different names from the declaration

Thanks for the explanation; I did not know that!

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@CHANGELOG.md`:
- Line 14: PR `#1464` is a bug fix grouped under the "Added" section in
CHANGELOG.md; please add a separate "Fixed" entry referencing [`#1464`] (and
mentioning CtrlOp::getBodyUnitary()) or move a concise note about the bug fix
out of the "Added" section into a new "Fixed" subsection so the fix is
discoverable while keeping the infrastructure grouping for the other MLIR PRs.