✨ Add Constant Propagation#1845
Draft
LiRem101 wants to merge 198 commits into
Draft
Conversation
…e into mlir/hadamard-lifting
Collaborator
Author
|
@coderabbitai Full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesIntroduces QCO quantum/classical state tracking and a Constant propagation foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 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 `@CHANGELOG.md`:
- Around line 15-16: Correct the contributor attribution markup in the changelog
entry by adding the missing closing bold delimiter after the username, while
preserving the existing link and entry text.
In
`@mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/ClassicalArithOperation.h`:
- Around line 31-107: Replace the native arithmetic in getArithIntegerOpResult
and getArithDoubleOpResult with MLIR arith folding or APInt/APFloat operations
so results match arith semantics, including overflow, shifts, division,
remainder, NaN, and signed-zero behavior. Keep maximumf and maxnumf handled
distinctly, and propagate a not-foldable result whenever the computed value
cannot be represented safely rather than forcing an int64_t or double result.
In
`@mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/GateToMap.h`:
- Around line 148-152: Update the iSWAPOp case in the gate-to-map optimization
so the |01⟩ and |10⟩ coefficients use +i (Complex(0, 1)) rather than -i,
matching MQT’s iSWAP matrix. Add an isolated iSWAP test that directly verifies
these mappings, independent of the multi-gate aggregate test.
- Around line 126-132: Update the U2Op case in the gate-to-map conversion so
every matrix entry uses the common 1/√2 normalization factor, not only the first
coefficient; preserve the existing parameterized phase expressions. Add a direct
regression test covering U2 basis-state propagation and verifying the normalized
output.
In
`@mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/HybridState.hpp`:
- Around line 11-12: Add `#pragma` once at the beginning of the HybridState
header, while retaining the existing MQT_CORE_HYBRIDSTATE_H include guard.
- Around line 51-81: Update isOperationExecutable so both positive and negative
controls use one consistent zero predicate based on the documented exact
zero/nonzero semantics, rather than the current 1e-4 norm thresholds and
opposing comparisons. Ensure zero-valued controls satisfy only the appropriate
polarity, nonzero values satisfy the opposite polarity, and preserve the
existing missing-value domain_error behavior.
In
`@mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/QuantumState.hpp`:
- Around line 54-57: Cap supported state width before any unsigned-int mask
shifts in the quantum-state constant propagation logic, including the
union-table handling and the code around amplitudeMap updates. When a union
entry exceeds the bit width of unsigned int, transition it to TOP or otherwise
reject it before shifting; preserve existing behavior for representable widths
and prevent overflow or basis-state aliasing.
- Around line 11-12: Replace the manual include guard with `#pragma` once in
QuantumState.hpp at
mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/QuantumState.hpp:11-12,
and remove its matching closing `#endif`. Apply the same change to UnionTable.hpp
at
mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.hpp:11-12,
removing its closing `#endif` as well.
In
`@mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.hpp`:
- Around line 168-175: Update the entry-count calculation around
numberOfNewEntries to use std::size_t and guard each multiplication against
overflow by checking whether the current count exceeds maximumHybridEntries
divided by e.states.size() before multiplying. Throw the existing domain_error
when the cap would be exceeded, preserving the limit enforcement for large
Cartesian products.
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 214-231: Correct the public pass documentation in the pass
description and the additional referenced section: change “routines assumes” to
“routine assumes,” “vales” to “values,” “alaways” to “always,” and “tracted” to
“tracked,” without modifying the documented behavior.
In `@mlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation.cpp`:
- Around line 1074-1090: Update the worklist processing around isEntryPoint and
iterateThroughWorklist so each entry-point function creates and uses its own
UnionTable state. Build the worklist and invoke processing per entry function,
ensuring values and equivalence lookups cannot persist across functions;
preserve the existing limits and rewriter behavior.
- Around line 612-638: Update putOperationIntoBranch so every value in
ControlsToModify::classicalPosCtrlsToAdd and classicalNegCtrlsToAdd contributes
to the branch condition, rather than selecting only the first DenseSet element.
Build nested conditions or an equivalent conjunction that preserves the original
positive and negative control requirements, and apply the same correction to the
corresponding logic at the later referenced branch.
- Around line 560-604: Update the both-empty branch handling around the
implicit-swap mapping so `order` is derived from each branch’s YieldOp operands,
not only from non-empty `thenArgs` or `elseArgs`. Ensure identity yields produce
a complete identity mapping and permuted yields are detected before
`order.at(...)` is used during result replacement, preserving the existing swap
error behavior.
- Around line 868-893: Update the target-mapping logic around targetQubits and
resultQubits to resize both vectors to numTargets and assign each mapped value
by index instead of inserting into an empty vector. In the target loop, compare
arguments[argIndex] with body.getInputTarget(i) and store
op->getInputTarget(argIndex) at the matched index; apply the equivalent indexed
assignment for yieldOP output mappings using op->getOutputTarget(uOpOutIndex).
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/HybridState.cpp`:
- Around line 91-98: Update HybridState::operator== to handle top on either
operand before comparing probability or dereferencing qState: return true only
when both states are TOP, and false when exactly one is TOP. Preserve the
existing non-TOP probability and quantum-state comparison for states where both
qState values are available.
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/QuantumState.cpp`:
- Around line 39-48: Update QuantumState::QuantumState to copy globalQubitNumber
into a local container before sorting, then build globalToLocalQubitNumber from
the sorted copy. Do not mutate the caller-owned span.
- Around line 154-167: Guard every unsigned-int basis-state shift throughout
QuantumState, including construction, unify, gate propagation,
measurement/reset, and qubit-query paths. Validate that each local qubit index
is below std::numeric_limits<unsigned int>::digits before evaluating 1U <<
index, rejecting states or operations that exceed the supported width; preserve
existing behavior for valid indices.
In
`@mlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.cpp`:
- Around line 283-299: Apply the configured maximumHybridEntries cap while
accumulating states in the reset propagation loop around
HybridState::propagateReset. Before or during vecOfNewStates insertion, detect
when the generated total exceeds the limit and handle it using the same
top-state fallback behavior as the measurement branching path, rather than
assigning all branches to ute->states. Preserve the existing domain_error
handling and normal replacement flow when the cap is not exceeded.
- Around line 319-328: Update UnionTable::propagateIntAlloc and the
corresponding double-allocation path to create one shared UnionTableEntry
pointer, then insert that same pointer into both entries and valuesToEntries
instead of constructing separate objects. Preserve the existing entry contents
and indexing behavior.
- Around line 342-405: Update UnionTable::isQubitAlwaysOne, isQubitAlwaysZero,
isClassicalValueAlwaysTrue, isClassicalValueAlwaysFalse, and
hasAlwaysZeroProbability to detect TOP entries before evaluating their state
collections. Treat TOP as unknown: return false for “always” proofs and do not
report zero probability, rather than allowing empty-state all_of or iteration
results to imply certainty. Apply the same TOP handling to the related
proof/query paths around the additional referenced methods.
In
`@mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp`:
- Around line 369-375: Populate the right-hand state’s classical maps in all
three unification tests: at
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp
lines 369-375, add v2 and v3 to hState2; at lines 393-397 and 412-416, make
hState2 mutable and add the intended classical values before calling unify.
- Around line 238-251: The negative-control tests pass ctrl as the
positive-control argument. In test_hybridState.cpp lines 238-251, update the
propagateMeasurement call to pass an empty positive-control vector and ctrl as
the negative-control vector, then assert measurement executes when the control
value is zero. In lines 302-315, make the same argument correction for
propagateReset and assert reset executes under the zero negative control.
🪄 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: 5af346a4-b749-424b-b2dd-cf13bc2052c5
📒 Files selected for processing (23)
.agent/plans/first-mlir-python-bindings.md.github/pull_request_template.mdCHANGELOG.mdUPGRADING.mddocs/_templates/doxygen-header.htmldocs/conf.pydocs/mlir/python_compiler_collection.mdmlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/ClassicalArithOperation.hmlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/GateToMap.hmlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/HybridState.hppmlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/QuantumState.hppmlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.hppmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation.cppmlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/HybridState.cppmlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/QuantumState.cppmlir/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_quantumState.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_unionTable.cppmlir/unittests/Dialect/QCO/Transforms/Optimizations/test_qco_constant_propagation.cpppyproject.toml
💤 Files with no reviewable changes (1)
- docs/conf.py
Comment on lines
+15
to
+16
| - ✨ Add a `constant propagation` pass for reducing superfluous quantum | ||
| resources by propagating the quantum machine state ([#1845]) ([**@lirem101]) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the author attribution markup.
Line 16 is missing the closing **, so the contributor attribution renders incorrectly.
Proposed fix
- resources by propagating the quantum machine state ([`#1845`]) ([**`@lirem101`])
+ resources by propagating the quantum machine state ([`#1845`]) ([**`@lirem101`**])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - ✨ Add a `constant propagation` pass for reducing superfluous quantum | |
| resources by propagating the quantum machine state ([#1845]) ([**@lirem101]) | |
| - ✨ Add a `constant propagation` pass for reducing superfluous quantum | |
| resources by propagating the quantum machine state ([`#1845`]) ([**`@lirem101`**]) |
🤖 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 `@CHANGELOG.md` around lines 15 - 16, Correct the contributor attribution
markup in the changelog entry by adding the missing closing bold delimiter after
the username, while preserving the existing link and entry text.
Comment on lines
+31
to
+107
| inline int64_t getArithIntegerOpResult(mlir::Operation* operation, | ||
| int64_t value1, int64_t value2 = 0, | ||
| int64_t value3 = 0) { | ||
|
|
||
| for (mlir::Value operand : operation->getOperands()) { | ||
| if (isa<mlir::VectorType>(operand.getType())) { | ||
| throw std::runtime_error( | ||
| "Constant propagation does not support vectors as classical types."); | ||
| } | ||
| } | ||
|
|
||
| return mlir::TypeSwitch<mlir::Operation*, int64_t>(operation) | ||
| .Case<mlir::arith::AddIOp>([&](auto) { return value1 + value2; }) | ||
| .Case<mlir::arith::AndIOp>([&](auto) { return value1 & value2; }) | ||
| .Case<mlir::arith::CeilDivSIOp>([&](auto) { | ||
| // Division that rounds to positive infinity | ||
| return ceil(1.0 * value1 / value2); | ||
| }) | ||
| .Case<mlir::arith::DivSIOp>([&](auto) { | ||
| // Division that rounds towards zero | ||
| return value1 / value2; | ||
| }) | ||
| .Case<mlir::arith::FloorDivSIOp>([&](auto) { | ||
| // Division that rounds to negative infinity | ||
| return floor(1.0 * value1 / value2); | ||
| }) | ||
| .Case<mlir::arith::MaxSIOp>( | ||
| [&](auto) { return value1 > value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MinSIOp>( | ||
| [&](auto) { return value1 < value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MulIOp>([&](auto) { return value1 * value2; }) | ||
| .Case<mlir::arith::OrIOp>([&](auto) { return value1 | value2; }) | ||
| .Case<mlir::arith::RemSIOp>( | ||
| [&](auto) { return remainder(value1, value2); }) | ||
| .Case<mlir::arith::ShLIOp>([&](auto) { return value1 << value2; }) | ||
| .Case<mlir::arith::ShRSIOp>([&](auto) { return value1 >> value2; }) | ||
| .Case<mlir::arith::SubIOp>([&](auto) { return value1 - value2; }) | ||
| .Case<mlir::arith::XOrIOp>([&](auto) { return value1 ^ value2; }) | ||
| .Case<mlir::arith::SelectOp>( | ||
| [&](auto) { return value1 == 0 ? value3 : value2; }) | ||
| .Default([&](auto) -> int64_t { | ||
| throw std::runtime_error("Unsupported integer operation in " | ||
| "mlir::qco::classicalarithoperation"); | ||
| }); | ||
| } | ||
|
|
||
| inline double getArithDoubleOpResult(mlir::Operation* operation, double value1, | ||
| double value2 = 0.0) { | ||
|
|
||
| for (mlir::Value operand : operation->getOperands()) { | ||
| if (isa<mlir::VectorType>(operand.getType())) { | ||
| throw std::runtime_error( | ||
| "Constant propagation does not support vectors as classical types."); | ||
| } | ||
| } | ||
|
|
||
| return mlir::TypeSwitch<mlir::Operation*, double>(operation) | ||
| .Case<mlir::arith::AddFOp>([&](auto) { return value1 + value2; }) | ||
| .Case<mlir::arith::DivFOp>([&](auto) { return value1 / value2; }) | ||
| .Case<mlir::arith::MaximumFOp>( | ||
| [&](auto) { return value1 > value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MaxNumFOp>( | ||
| [&](auto) { return value1 > value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MinimumFOp>( | ||
| [&](auto) { return value1 < value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MinNumFOp>( | ||
| [&](auto) { return value1 < value2 ? value1 : value2; }) | ||
| .Case<mlir::arith::MulFOp>([&](auto) { return value1 * value2; }) | ||
| .Case<mlir::arith::NegFOp>([&](auto) { return -value1; }) | ||
| .Case<mlir::arith::RemFOp>( | ||
| [&](auto) { return remainder(value1, value2); }) | ||
| .Case<mlir::arith::SubFOp>([&](auto) { return value1 - value2; }) | ||
| .Default([&](auto) -> double { | ||
| throw std::runtime_error("Unsupported floating-point operation in " | ||
| "mlir::qco::classicalarithoperation"); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
What are the exact scalar folding semantics of MLIR arith.remsi, arith.remf, ceildivsi, floordivsi, maximumf, and maxnumf, including integer bit widths and poison/undefined cases?
💡 Result:
The MLIR 'arith' dialect defines basic arithmetic operations that operate on scalars (integers or floating-point), as well as vectors and tensors of these types [1]. Unless otherwise stated, these operations use two's complement representation for integers and propagate poison values [1]. If any input is poison, the result is poison [1]. arith.remsi (Signed Integer Division Remainder) This operation performs a signed integer remainder [2][3]. It treats the leading bit as a sign bit (e.g., 6 % -2 = 0) [2][3]. Division by zero is undefined behavior [2][3]. For vector and tensor types, if any element is divided by zero, the behavior is undefined [2][3]. arith.remf (Floating-Point Remainder) This operation performs a floating-point remainder, with semantics matching the llvm.frem instruction [4]. The sign of the result always matches the sign of the dividend (the left-hand side operand) [4]. arith.ceildivsi (Signed Integer Ceil Division) This performs signed integer division rounded towards positive infinity (e.g., 7 / -2 = -3) [5]. arith.floordivsi (Signed Integer Floor Division) This performs signed integer division rounded towards negative infinity (e.g., 5 / -2 = -3) [5][6]. arith.maximumf (Floating-Point Maximum) Returns the maximum of two arguments, treating -0.0 as less than +0.0 [2][5]. If either argument is NaN, the result is NaN [2][5]. arith.maxnumf (Floating-Point Maximum) Returns the maximum of two arguments [2][5]. If the arguments are -0.0 and +0.0, the result is either value [2][5]. If one argument is NaN, the result is the other argument [2][5]. Note on Integer Bit Widths: These operations are polymorphic and accept integers of any bit width (e.g., i8, i16, i32, i64) as defined in the MLIR type system [1][2]. The specific behavior, such as two's complement arithmetic and overflow behavior, applies generically across these widths [1]. Note on Poison: Unless otherwise specified, all operations in the arith dialect propagate poison values, meaning that if any of the inputs are poison, the output is also poison [1]. For structured types (vector/tensor), this propagation occurs element-wise [1].
Citations:
- 1: https://mlir.llvm.org/docs/Dialects/ArithOps/
- 2: https://github.com/llvm/llvm-project/blob/06b5a7c205cbf33d1c40efc77e0820ddd4e060fa/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
- 3: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td
- 4: https://lists.llvm.org/pipermail/all-commits/Week-of-Mon-20240624/166425.html
- 5: https://mlir.llvm.org/python-bindings/autoapi/mlir/dialects/arith/index.html
- 6: [mlir][arith] doc updates for ub semantics, and int representations llvm/llvm-project#72932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target header and nearby symbols.
ast-grep outline mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/ClassicalArithOperation.h --view expanded || true
echo
echo "=== File excerpt ==="
sed -n '1,220p' mlir/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/ClassicalArithOperation.h
echo
echo "=== Call sites for helpers ==="
rg -n "getArith(Integer|Double)OpResult|maximumf|maxnumf|remsi|remf|ceildivsi|floordivsi" mlir -g '!**/build/**'Repository: munich-quantum-toolkit/core
Length of output: 5551
Use MLIR folding/APInt/APFloat here instead of native C++ arithmetic.
std::remainder,double-based ceil/floor division, and raw shifts/overflow rules do not matcharithsemantics.maximumfandmaxnumfare also conflated; they differ on NaN and signed zero.- Delegate to the ops’ fold hooks or use
APInt/APFloat, and return not-foldable when the result can’t be represented safely.
🤖 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/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/ClassicalArithOperation.h`
around lines 31 - 107, Replace the native arithmetic in getArithIntegerOpResult
and getArithDoubleOpResult with MLIR arith folding or APInt/APFloat operations
so results match arith semantics, including overflow, shifts, division,
remainder, NaN, and signed-zero behavior. Keep maximumf and maxnumf handled
distinctly, and propagate a not-foldable result whenever the computed value
cannot be represented safely rather than forcing an int64_t or double result.
Comment on lines
+126
to
+132
| .Case<mlir::qco::U2Op>([&](auto) { | ||
| return ResultMap{ | ||
| {{0, {{0, Complex(inv_sqrt2, 0)}, {1, exp(Complex(0, params[0]))}}}, | ||
| {1, | ||
| {{0, -exp(Complex(0, params[1]))}, | ||
| {1, exp(Complex(0, params[0] + params[1]))}}}}}; | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the 1/√2 factor to every U2 matrix entry.
Only the first coefficient is normalized, making this matrix non-unitary. MQT defines the entire U2 matrix with a common 1/√2 factor. (mqt.readthedocs.io)
Proposed fix
.Case<mlir::qco::U2Op>([&](auto) {
return ResultMap{
- {{0, {{0, Complex(inv_sqrt2, 0)}, {1, exp(Complex(0, params[0]))}}},
+ {{0,
+ {{0, Complex(inv_sqrt2, 0)},
+ {1, inv_sqrt2 * exp(Complex(0, params[0]))}}},
{1,
- {{0, -exp(Complex(0, params[1]))},
- {1, exp(Complex(0, params[0] + params[1]))}}}}};
+ {{0, -inv_sqrt2 * exp(Complex(0, params[1]))},
+ {1, inv_sqrt2 *
+ exp(Complex(0, params[0] + params[1]))}}}}};
})Add a direct U2 basis-state regression test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .Case<mlir::qco::U2Op>([&](auto) { | |
| return ResultMap{ | |
| {{0, {{0, Complex(inv_sqrt2, 0)}, {1, exp(Complex(0, params[0]))}}}, | |
| {1, | |
| {{0, -exp(Complex(0, params[1]))}, | |
| {1, exp(Complex(0, params[0] + params[1]))}}}}}; | |
| }) | |
| .Case<mlir::qco::U2Op>([&](auto) { | |
| return ResultMap{ | |
| {{0, | |
| {{0, Complex(inv_sqrt2, 0)}, | |
| {1, inv_sqrt2 * exp(Complex(0, params[0]))}}}, | |
| {1, | |
| {{0, -inv_sqrt2 * exp(Complex(0, params[1]))}, | |
| {1, inv_sqrt2 * | |
| exp(Complex(0, params[0] + params[1]))}}}}}; | |
| }) |
🤖 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/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/GateToMap.h`
around lines 126 - 132, Update the U2Op case in the gate-to-map conversion so
every matrix entry uses the common 1/√2 normalization factor, not only the first
coefficient; preserve the existing parameterized phase expressions. Add a direct
regression test covering U2 basis-state propagation and verifying the normalized
output.
Comment on lines
+148
to
+152
| .Case<mlir::qco::iSWAPOp>([&](auto) { | ||
| return ResultMap{{{0, {{0, Complex(1, 0)}}}, | ||
| {1, {{2, Complex(0, -1)}}}, | ||
| {2, {{1, Complex(0, -1)}}}, | ||
| {3, {{3, Complex(1, 0)}}}}}; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use +i for iSWAP, not -i.
These coefficients implement iSWAP†. MQT's iSWAP matrix maps |01⟩ and |10⟩ with +i. (mqt.readthedocs.io)
Proposed fix
.Case<mlir::qco::iSWAPOp>([&](auto) {
return ResultMap{{{0, {{0, Complex(1, 0)}}},
- {1, {{2, Complex(0, -1)}}},
- {2, {{1, Complex(0, -1)}}},
+ {1, {{2, Complex(0, 1)}}},
+ {2, {{1, Complex(0, 1)}}},
{3, {{3, Complex(1, 0)}}}}};
})Add an isolated iSWAP test rather than relying on the multi-gate aggregate test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .Case<mlir::qco::iSWAPOp>([&](auto) { | |
| return ResultMap{{{0, {{0, Complex(1, 0)}}}, | |
| {1, {{2, Complex(0, -1)}}}, | |
| {2, {{1, Complex(0, -1)}}}, | |
| {3, {{3, Complex(1, 0)}}}}}; | |
| .Case<mlir::qco::iSWAPOp>([&](auto) { | |
| return ResultMap{{{0, {{0, Complex(1, 0)}}}, | |
| {1, {{2, Complex(0, 1)}}}, | |
| {2, {{1, Complex(0, 1)}}}, | |
| {3, {{3, Complex(1, 0)}}}}}; |
🤖 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/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/GateToMap.h`
around lines 148 - 152, Update the iSWAPOp case in the gate-to-map optimization
so the |01⟩ and |10⟩ coefficients use +i (Complex(0, 1)) rather than -i,
matching MQT’s iSWAP matrix. Add an isolated iSWAP test that directly verifies
these mappings, independent of the multi-gate aggregate test.
Comment on lines
+11
to
+12
| #ifndef MQT_CORE_HYBRIDSTATE_H | ||
| #define MQT_CORE_HYBRIDSTATE_H |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required #pragma once.
As per coding guidelines, C++ headers must use #pragma once.
🤖 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/include/mlir/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/HybridState.hpp`
around lines 11 - 12, Add `#pragma` once at the beginning of the HybridState
header, while retaining the existing MQT_CORE_HYBRIDSTATE_H include guard.
Source: Coding guidelines
Comment on lines
+283
to
+299
| std::vector<HybridState> vecOfNewStates; | ||
|
|
||
| for (auto hs : ute->states) { | ||
| try { | ||
| auto newStates = | ||
| hs.propagateReset(qubitsToGlobalIndices.at(quantumTarget), | ||
| posCtrlsClassical, negCtrlsClassical); | ||
| vecOfNewStates.insert(vecOfNewStates.end(), newStates.begin(), | ||
| newStates.end()); | ||
| } catch (std::domain_error&) { | ||
| putEntriesToTop({*ute}); | ||
| vecOfNewStates.clear(); | ||
| break; | ||
| } | ||
| } | ||
| ute->states = vecOfNewStates; | ||
| replaceValuesGlobally(quantumTargetVec, newQuantumValueVec); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Apply maximumHybridEntries to reset branching.
Unlike measurement, reset assigns every generated branch without checking the configured cap. Repeated resets can grow states exponentially and bypass the pass’s advertised complexity limit.
🤖 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/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.cpp`
around lines 283 - 299, Apply the configured maximumHybridEntries cap while
accumulating states in the reset propagation loop around
HybridState::propagateReset. Before or during vecOfNewStates insertion, detect
when the generated total exceeds the limit and handle it using the same
top-state fallback behavior as the measurement branching path, rather than
assigning all branches to ute->states. Preserve the existing domain_error
handling and normal replacement flow when the cap is not exceeded.
Comment on lines
+319
to
+328
| void UnionTable::propagateIntAlloc(const Value intValue, const int64_t number) { | ||
| auto hs = HybridState({}, maxNonzeroAmplitudes); | ||
| hs.addIntegerValue(intValue, number); | ||
|
|
||
| auto ute = UnionTableEntry(); | ||
| ute.states.push_back(hs); | ||
| ute.participatingClassicalValues.insert(intValue); | ||
| entries.insert(std::make_shared<UnionTableEntry>(ute)); | ||
| valuesToEntries[intValue] = std::make_shared<UnionTableEntry>(ute); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Store the same UnionTableEntry pointer in both indexes.
Each classical allocation inserts one object into entries and another into valuesToEntries. Later mutations through the value map are therefore invisible when iterating entries.
Proposed pattern
- entries.insert(std::make_shared<UnionTableEntry>(ute));
- valuesToEntries[intValue] = std::make_shared<UnionTableEntry>(ute);
+ const auto entry = std::make_shared<UnionTableEntry>(std::move(ute));
+ entries.insert(entry);
+ valuesToEntries[intValue] = entry;Apply the same change to double allocations.
Also applies to: 330-340
🤖 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/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.cpp`
around lines 319 - 328, Update UnionTable::propagateIntAlloc and the
corresponding double-allocation path to create one shared UnionTableEntry
pointer, then insert that same pointer into both entries and valuesToEntries
instead of constructing separate objects. Preserve the existing entry contents
and indexing behavior.
Comment on lines
+342
to
+405
| bool UnionTable::isQubitAlwaysOne(const Value q) const { | ||
| const unsigned int qubitIndex = qubitsToGlobalIndices.at(q); | ||
| const auto ute = valuesToEntries.at(q); | ||
| return std::ranges::all_of(ute->states, [&](const auto& hs) { | ||
| return hs.isQubitAlwaysOne(qubitIndex); | ||
| }); | ||
| } | ||
|
|
||
| bool UnionTable::isQubitAlwaysZero(const Value q) const { | ||
| const unsigned int qubitIndex = qubitsToGlobalIndices.at(q); | ||
| const auto ute = valuesToEntries.at(q); | ||
| return std::ranges::all_of(ute->states, [&](const auto& hs) { | ||
| return hs.isQubitAlwaysZero(qubitIndex); | ||
| }); | ||
| } | ||
|
|
||
| bool UnionTable::isClassicalValueAlwaysTrue(const Value c) const { | ||
| const auto ute = valuesToEntries.at(c); | ||
| return std::ranges::all_of(ute->states, | ||
| [&](const auto& hs) { return hs.isValueTrue(c); }); | ||
| } | ||
|
|
||
| bool UnionTable::isClassicalValueAlwaysFalse(const Value c) const { | ||
| const auto ute = valuesToEntries.at(c); | ||
| return std::ranges::all_of( | ||
| ute->states, [&](const auto& hs) { return !hs.isValueTrue(c); }); | ||
| } | ||
|
|
||
| bool UnionTable::hasAlwaysZeroProbability( | ||
| const llvm::DenseMap<Value, bool>& qubitValues, | ||
| const llvm::DenseMap<Value, bool>& classicalValues) const { | ||
| std::set<UnionTableEntry> participatingEntries; | ||
| for (const auto& [qV, _] : qubitValues) { | ||
| participatingEntries.insert(*valuesToEntries.at(qV)); | ||
| } | ||
| for (const auto& [cV, _] : classicalValues) { | ||
| participatingEntries.insert(*valuesToEntries.at(cV)); | ||
| } | ||
| for (const auto& ute : participatingEntries) { | ||
| std::unordered_map<unsigned int, bool> qubitValuesThisEntry; | ||
| llvm::DenseMap<Value, bool> classicalValuesThisEntry; | ||
| for (const auto& [qV, qBool] : qubitValues) { | ||
| if (ute.participatingQubits.contains(qV)) { | ||
| qubitValuesThisEntry[qubitsToGlobalIndices.at(qV)] = qBool; | ||
| } | ||
| } | ||
| for (const auto& [v, vBool] : classicalValues) { | ||
| if (ute.participatingClassicalValues.contains(v)) { | ||
| classicalValuesThisEntry[v] = vBool; | ||
| } | ||
| } | ||
| bool oneEntryIsNonzero = false; | ||
| for (const auto& hs : ute.states) { | ||
| if (!hs.hasAlwaysZeroProbability(qubitValuesThisEntry, | ||
| classicalValuesThisEntry)) { | ||
| oneEntryIsNonzero = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!oneEntryIsNonzero) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Treat TOP as unknown, not as an empty impossible state.
For TOP entries, all_of(empty) makes qubits/classical values simultaneously “always zero” and “always one,” while hasAlwaysZeroProbability returns true. A later controlled gate can therefore be incorrectly removed. Explicitly return “unknown” for TOP in these proof/query paths.
Also applies to: 408-449, 548-626
🤖 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/lib/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/UnionTable.cpp`
around lines 342 - 405, Update UnionTable::isQubitAlwaysOne, isQubitAlwaysZero,
isClassicalValueAlwaysTrue, isClassicalValueAlwaysFalse, and
hasAlwaysZeroProbability to detect TOP entries before evaluating their state
collections. Treat TOP as unknown: return false for “always” proofs and do not
report zero probability, rather than allowing empty-state all_of or iteration
results to imply certainty. Apply the same TOP handling to the related
proof/query paths around the additional referenced methods.
Comment on lines
+238
to
+251
| TEST_F(HybridStateTest, doMeasurementWithNegClassicalCtrl) { | ||
| auto hState = HybridState(fourQubits, 4, 0.6); | ||
| constexpr auto v1 = mlir::Value(); | ||
| hState.addIntegerValue(v1, 0); | ||
| std::vector ctrl = {v1}; | ||
| hState.propagateGate(hOp.getOperation(), vectorZero); | ||
| const auto resStates = hState.propagateMeasurement(0, v1, ctrl); | ||
|
|
||
| EXPECT_TRUE(resStates.size() == 1); | ||
| const auto& resHybridState = resStates.at(0); | ||
| EXPECT_THAT( | ||
| resHybridState.toString(), | ||
| testing::HasSubstr( | ||
| "{|0000> -> 0.71, |0001> -> 0.71}: integerValue0 = 0; p = 0.60")); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The negative-control tests bind their control vectors to the positive-control parameter.
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L238-L251: callpropagateMeasurement(0, v1, {}, ctrl)and assert that a zero negative control executes measurement.mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L302-L315: callpropagateReset(0, {}, ctrl)and assert that a zero negative control executes reset.
📍 Affects 1 file
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L238-L251(this comment)mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L302-L315
🤖 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/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp`
around lines 238 - 251, The negative-control tests pass ctrl as the
positive-control argument. In test_hybridState.cpp lines 238-251, update the
propagateMeasurement call to pass an empty positive-control vector and ctrl as
the negative-control vector, then assert measurement executes when the control
value is zero. In lines 302-315, make the same argument correction for
propagateReset and assert reset executes under the zero negative control.
Comment on lines
+369
to
+375
| auto hState2 = HybridState(vectorOneThree, 10, 0.5); | ||
| hState2.propagateGate(hOp.getOperation(), vectorThree); | ||
| hState2.propagateGate(xOp.getOperation(), vectorOne, vectorThree); | ||
| hState1.addIntegerValue(v2, 7); | ||
| hState1.addDoubleValue(v3, 4.2); | ||
|
|
||
| const HybridState unified = hState1.unify(hState2); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The unification tests never populate the right-hand classical maps.
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L369-L375: addv2andv3tohState2.mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L393-L397: makehState2mutable and add the intended values to it.mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L412-L416: makehState2mutable and populate its classical-only values.
📍 Affects 1 file
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L369-L375(this comment)mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L393-L397mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp#L412-L416
🤖 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/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp`
around lines 369 - 375, Populate the right-hand state’s classical maps in all
three unification tests: at
mlir/unittests/Dialect/QCO/Transforms/Optimizations/ConstantPropagation/test_hybridState.cpp
lines 369-375, add v2 and v3 to hState2; at lines 393-397 and 412-416, make
hState2 mutable and add the intended classical values before calling unify.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the optimization pass constant propagation. It propagates the state of a quantum machine through a circuit and optimizes the applied gates with respect to the current state. Namely, it removes controlling qubits, exchanges controlling qubits with controlling classical values and removes phase gates which only introduce a global phase. The optimization routine is based on this paper, accepted at QSW 2026.
I suggest having constant propagation switched to off on default, because it assumes that all states are initial zero, can take quite a while and can create dynamic circuits, which might not be everyone's goal.
GPT-4 and GPT-5 have been used for refactoring and for first drafts of code. No Gen AI was integrated in the used IDE.
Fixes #1387
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.