🐛 Fix conversions of jeff.switch and jeff.for#1776
Conversation
denialhaag
added
fix
denialhaag
force-pushed
the
isolated-from-above
branch
from
410657e to
c41d29f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
denialhaag
force-pushed
the
isolated-from-above
branch
from
c41d29f to
06031b1
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors Jeff↔QCO conversions to explicitly remap block arguments and above-values when moving regions, adds QCOProgramBuilder::floatConstant, introduces angle-parameterized test programs and registrations, and pins the jeff-mlir dependency to a specific commit; updates CHANGELOG with PR ChangesJeff-to-QCO Region Isolation and Parameterized Gate Support
Sequence Diagram(s)sequenceDiagram
participant JeffSwitch as jeff.SwitchOp
participant JeffToQCO as ConvertJeffSwitchOpToQCO
participant QCOIf as qco.if
participant MoveRegion as moveRegion
participant JeffBuilder as ConvertQCOIfOpToJeff
JeffSwitch->>JeffToQCO: create destination blocks + IRMapping
JeffToQCO->>QCOIf: clone non-terminators + remap terminator linear operands
QCOIf->>JeffBuilder: build jeff.SwitchOp with init-args (including above-values)
JeffBuilder->>MoveRegion: moveRegion(sourceRegion, destRegion, aboveValues)
MoveRegion->>JeffBuilder: cloned ops + jeff::YieldOp with remapped operands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
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 `@cmake/ExternalDependencies.cmake`:
- Around line 27-31: Add a short CMake comment immediately above the
FetchContent_Declare(jeff-mlir ...) block describing why the commit
3f34dc3e2865ceaffb8003b2410404306a49f0ab was pinned: cite the upstream PR/commit
title or number and summarize the relevant change (e.g., region isolation
semantics fix) that this project depends on so future maintainers understand the
rationale; update the comment if you later change the GIT_TAG in the
FetchContent_Declare for jeff-mlir.
🪄 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: 9eac6edb-b971-4c14-b618-3918728d5099
📒 Files selected for processing (9)
CHANGELOG.mdcmake/ExternalDependencies.cmakemlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/unittests/Conversion/JeffRoundTrip/test_jeff_round_trip.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This generally looks good to me. However I can't help but feel that some of the additions in the conversions feel more complicated than they need to be given the latest PR on the relax modifier PR. Maybe you could have another look!
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for trying! I think the last commit still is a genuine improvement 👍🏼
This looks like it is ready to go in to me 😌
I suppose the failures will go away with the new LLVM release, which we can hopefully update to soon.
I would be fine with merging this despite the failures in the meantime in order to not have it collect dust and conflicts.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp (1)
1196-1204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not erase
scf.yieldunconditionally.
moveRegion()already consumes the loop-body terminator whenConvertSCFForOpToJeffrewrites anscf.for. Keeping this as a generic pattern means dialect conversion can match the nestedscf::YieldOpfirst, delete the live terminator, and leave the parentscf.formalformed before its rewrite runs.Suggested fix
-struct ConvertSCFYieldOpToJeff final - : StatefulOpConversionPattern<scf::YieldOp> { - using StatefulOpConversionPattern::StatefulOpConversionPattern; - - LogicalResult - matchAndRewrite(scf::YieldOp op, OpAdaptor /*adaptor*/, - ConversionPatternRewriter& rewriter) const override { - rewriter.eraseOp(op); - return success(); - } -}; +// Remove this pattern. `ConvertSCFForOpToJeff::moveRegion` is responsible for +// consuming the source-region terminator and creating the destination +// `jeff::YieldOp`.Also drop
ConvertSCFYieldOpToJefffrom thepatterns.add<...>list inrunOnOperation().🤖 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/Conversion/QCOToJeff/QCOToJeff.cpp` around lines 1196 - 1204, ConvertSCFYieldOpToJeff currently erases scf::YieldOp unconditionally in matchAndRewrite which can remove a live terminator consumed by moveRegion()/ConvertSCFForOpToJeff; change matchAndRewrite in ConvertSCFYieldOpToJeff to not erase the op (e.g., return failure or do nothing) so it does not run as a generic pattern, and remove ConvertSCFYieldOpToJeff from the patterns.add<...> list in runOnOperation() so the yield is handled only by moveRegion()/ConvertSCFForOpToJeff; locate the class ConvertSCFYieldOpToJeff and its matchAndRewrite, and the registration site in runOnOperation() to apply these changes.
🤖 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.
Outside diff comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 1196-1204: ConvertSCFYieldOpToJeff currently erases scf::YieldOp
unconditionally in matchAndRewrite which can remove a live terminator consumed
by moveRegion()/ConvertSCFForOpToJeff; change matchAndRewrite in
ConvertSCFYieldOpToJeff to not erase the op (e.g., return failure or do nothing)
so it does not run as a generic pattern, and remove ConvertSCFYieldOpToJeff from
the patterns.add<...> list in runOnOperation() so the yield is handled only by
moveRegion()/ConvertSCFForOpToJeff; locate the class ConvertSCFYieldOpToJeff and
its matchAndRewrite, and the registration site in runOnOperation() to apply
these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3f0502c-179a-4b8c-aeb0-303a83933dec
📒 Files selected for processing (3)
cmake/ExternalDependencies.cmakemlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp (1)
880-908:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
jeff.switchcases that use or compute non-linear region values.This lowering still assumes every non-linear value is a pure passthrough: Lines 892-894 remap non-linear block arguments back to
inValues, so cloned ops that read them still capture values from above inside the newqco.ifregion, and Lines 922-925 replace every non-linear branch result with the incoming operand. That means a branch that uses a classical parameter/flag internally or yields a branch-computed classical value lowers to invalid/wrongqco.ifIR instead of failing conversion.Please add a match failure here unless each non-linear branch operand is unused by cloned ops and each non-linear yielded value aliases the corresponding incoming value.
Also applies to: 920-927
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed61ab96-41db-4bf4-ab91-b993dd177272
📒 Files selected for processing (2)
mlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
|
@coderabbitai, regarding your out-of-diff comment in #1776 (review): |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the specific pull request review
gh api repos/munich-quantum-toolkit/core/pulls/1776/reviews --jq '.[] | select(.id == 4481358024) | {id: .id, body: .body, state: .state}'
echo "---"
# Also get the review comments for that review
gh api repos/munich-quantum-toolkit/core/pulls/1776/reviews/4481358024/comments --jq '.[] | {id: .id, path: .path, body: .body, position: .position, original_position: .original_position, line: .line, original_line: .original_line}'Length of output: 3098 🏁 Script executed: #!/bin/bash
# Resolve the out-of-range review comment (cr-comment:v1:227af77458cba8f51c1f6578)
# First, list all review comments by coderabbit on PR 1776 to find the comment ID
gh api repos/munich-quantum-toolkit/core/pulls/1776/comments --jq '.[] | select(.body | contains("Reject `jeff.switch` cases")) | {id: .id, body: .body[0:100]}'Length of output: 580 The flagged comment was embedded in the review body rather than as a separate resolvable thread, so there's no separate comment ID to dismiss programmatically. The learning has been recorded so this won't recur in future reviews. Again, sorry for the incorrect concern! |
|
Tip For best results, initiate chat on the files or code changes.
I've added a learning to avoid repeating this mistake in future reviews. 🐇 (‾◡◝) ✏️ Learnings added
|
Thanks for pushing me to try! 😌 Feel free to merge this tomorrow morning. I don't think I can bypass the rules in MQT Core. 🤠 |
You now can. 🙌🏼 |
2 participants
Description
This PR fixes the conversions of
jeff.switchandjeff.forbe ensuring that the created regions are isolated from above.Fixes #1725
Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).