✨ Add conversions between Jeff and QCO#1479
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
A brief overview of everything that remains open: Main FunctionI have not yet looked into the proper conversion of a QC-style main function to a Jeff-style main function. The current implementation of the tests relies on bare-bones modules without a properly defined entry point. This might require some coordination with Xanadu, as we should align on what a Jeff-style main function exactly looks like. That said, the implementation of the conversion itself should be straightforward. Unconvertible QC OperationsThe following QC operations can currently not be converted to Jeff because they do not have a direct counterpart:
Unconvertible Jeff OperationsThe following Jeff operations can currently not be converted to QC because they do not have a direct counterpart:
In addition, the following additional conversions are not implemented yet, as I believe their implementation needs further discussion:
TestsThe current version of the tests does not perform proper equivalence checking but relies on (very minimal) string comparisons. |
|
I'll try to collect my thoughts here as well so that there is some kind of record of them.
I think going from MQT -> Jeff should not be particularly hard, as we have a much simpler setup in general (at least for now).
Based on the discussions in unitaryfoundation/jeff#38,
Based on the discussions in unitaryfoundation/jeff#38,
Based on the discussions in unitaryfoundation/jeff#38,
Based on the discussions in unitaryfoundation/jeff#38, these should be converted to Pauli Rotations.
Based on the discussions in unitaryfoundation/jeff#38, these should be converted to custom gates.
While not explicitly mentioned in unitaryfoundation/jeff#38, I believe a
Based on the semantic description, I would argue that our
Based on the semantics in the spec, I would argue that
These will be a little trickier to support.
Given how prevalent these PPRs are in FTQC literature, we may sooner or later add this as a dedicated primitive to our supported gates. For now, we may convert the specializations but may fail the conversions for general rotations. As for custom gates, what we could do here is to support matching the names of gates that we could not explicitly export as well known gates (xx_plus_yy, ecr, etc.) so that a roundtrip of all MQT gates would work.
This very much depends on how we decide to handle registers. Hopefully, we can resolve this soon.
As discussed offline, these conversion patterns could become part of the Jeff-mlir repository itself. It may be worth opening an issue about that over there.
I would guess that roundtrip tests could be pretty effective here at catching issues. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
## Description This PR contains minor improvements to the MLIR code that I have stumbled across during #1479. I'm opening up this PR to keep #1479 a bit leaner. ## Checklist: - [x] The pull request only contains commits that are focused and relevant to this change. - [x] ~I have added appropriate tests that cover the new/changed functionality.~ - [x] ~I have updated the documentation to reflect these changes.~ - [x] I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals. - [x] ~I have added migration instructions to the upgrade guide (if needed).~ - [x] The changes follow the project's style guidelines and introduce no new warnings. - [x] The changes are fully tested and pass the CI checks. - [x] I have reviewed my own code changes. --------- Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com> Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp (1)
371-383:⚠️ Potential issue | 🟠 MajorRemove the synthetic return-value producer during main cleanup.
Line 376 only erases
func.return. On a Jeff→QCO→Jeff roundtrip, the temporary scalar op that was introduced solely to satisfy the intermediate() -> i64entry signature is left behind as dead IR in the entry block, so the final module is no longer Jeff-only. Erase that returned value's defining op when it becomes unused before creating the trivial return.Proposed fix
auto* returnOp = block->getTerminator(); if (!llvm::isa<func::ReturnOp>(returnOp)) { return failure(); } + Value returnedValue; + if (auto ret = llvm::cast<func::ReturnOp>(returnOp); + ret.getNumOperands() == 1) { + returnedValue = ret.getOperand(0); + } returnOp->erase(); + + if (returnedValue && returnedValue.hasOneUse()) { + if (auto* def = returnedValue.getDefiningOp(); + llvm::isa<arith::ConstantOp, jeff::IntConst1Op, + jeff::IntConst64Op, jeff::FloatConst64Op>(def)) { + def->erase(); + } + } // Add trivial return operation builder.setInsertionPointToEnd(block); func::ReturnOp::create(builder, loc);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp` around lines 371 - 383, The cleanup currently erases only the terminator (returnOp) but leaves the synthetic scalar producer behind; update the block cleanup in QCOToJeff.cpp to, after obtaining returnOp (the terminator) and verifying it's a func::ReturnOp, check the returnOp's returned value(s) (e.g., returnOp->getOperand(0) or the appropriate getResult/getOperand access), find its defining op (def = value.getDefiningOp()), and if def exists and has no other uses erase that defining op before erasing returnOp; then proceed to insert the trivial return with builder and reset main's type via main.setType(FunctionType::get(ctx, {}, {})). Ensure you reference returnOp, block, builder, and main when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 17-21: The CMake snippet fails to update a pre-existing cached
CMAKE_CXX_STANDARD, so change the logic in the CMakeLists to assign the normal
variable and then update the cache with FORCE when the detected value is less
than 20; specifically set CMAKE_CXX_STANDARD=20 (regular variable) and call
set(... CACHE STRING "C++ standard to conform to" FORCE) so the cache is
overwritten and the minimum C++20 requirement is actually enforced when
CMAKE_CXX_STANDARD is lower or undefined.
---
Duplicate comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 371-383: The cleanup currently erases only the terminator
(returnOp) but leaves the synthetic scalar producer behind; update the block
cleanup in QCOToJeff.cpp to, after obtaining returnOp (the terminator) and
verifying it's a func::ReturnOp, check the returnOp's returned value(s) (e.g.,
returnOp->getOperand(0) or the appropriate getResult/getOperand access), find
its defining op (def = value.getDefiningOp()), and if def exists and has no
other uses erase that defining op before erasing returnOp; then proceed to
insert the trivial return with builder and reset main's type via
main.setType(FunctionType::get(ctx, {}, {})). Ensure you reference returnOp,
block, builder, and main when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07bf9b14-39bf-4cc0-8b5f-931d3fdafc99
📒 Files selected for processing (6)
CMakeLists.txtcmake/ExternalDependencies.cmakecmake/GetVersion.cmakemlir/CMakeLists.txtmlir/lib/Conversion/QCOToJeff/CMakeLists.txtmlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
💤 Files with no reviewable changes (1)
- mlir/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp (1)
756-791:⚠️ Potential issue | 🟠 MajorValidate exact target/parameter arity in every
jeff.custombranch.
rstill assumes two params, the two-target branches still assume two targets, andxx_±yy/barrierstill accept malformed param counts silently. A malformed or version-skewed Jeff module can therefore walk past the end of these arrays instead of returningnotifyMatchFailure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp` around lines 756 - 791, The branches handling custom ops in JeffToQCO.cpp do not validate exact target/parameter arity before indexing into arrays; update each branch (e.g., the "r" branch that calls createOneTargetTwoParameter<qco::ROp>, the two-target branches that call createTwoTargetZeroParameter<qco::iSWAPOp>/createTwoTargetZeroParameter<qco::DCXOp>/createTwoTargetZeroParameter<qco::ECROp>, the two-parameter branches calling createTwoTargetTwoParameter<qco::XXMinusYYOp>/createTwoTargetTwoParameter<qco::XXPlusYYOp>, and the "barrier" branch that calls createBarrierOp) to explicitly check adaptor.getInTargetQubits().size() and op.getParams().size() for the exact expected counts and return rewriter.notifyMatchFailure(op, "<op> must have exactly N target(s)/M parameter(s)") on mismatch before calling the create* helpers.CMakeLists.txt (1)
17-20:⚠️ Potential issue | 🟠 MajorSet the normal
CMAKE_CXX_STANDARDvariable too.
set(... CACHE ... FORCE)updates the cache entry, but it does not overwrite a normalCMAKE_CXX_STANDARDinherited from a superproject. In that case this directory can still see17and initialize its targets with C++17. Set the regular variable before the cache write. Based on learnings, the MQT Core project uses the C++20 standard.Verification script
#!/bin/bash set -euo pipefail command -v cmake >/dev/null tmpdir="$(mktemp -d)" mkdir -p "$tmpdir/child" cat >"$tmpdir/CMakeLists.txt" <<'CMAKE' cmake_minimum_required(VERSION 3.24) project(parent LANGUAGES CXX) set(CMAKE_CXX_STANDARD 17) add_subdirectory(child) CMAKE cat >"$tmpdir/child/CMakeLists.txt" <<'CMAKE' if(NOT DEFINED CMAKE_CXX_STANDARD OR CMAKE_CXX_STANDARD LESS 20) set(CMAKE_CXX_STANDARD 20 CACHE STRING "C++ standard to conform to" FORCE) endif() message(STATUS "child-normal=${CMAKE_CXX_STANDARD}") get_property(cacheVal CACHE CMAKE_CXX_STANDARD PROPERTY VALUE) message(STATUS "child-cache=${cacheVal}") CMAKE cmake -S "$tmpdir" -B "$tmpdir/build" 2>&1 | grep -E 'child-normal=|child-cache='🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 17 - 20, The check that sets the cache-only CMAKE_CXX_STANDARD should also set the regular variable so an inherited non-cache value isn't left in effect; before calling set(... CACHE ... FORCE) assign CMAKE_CXX_STANDARD=20 (e.g., set(CMAKE_CXX_STANDARD 20)) and then perform the cache set, ensuring both the normal variable and the cache entry for CMAKE_CXX_STANDARD are updated to C++20.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp`:
- Around line 880-906: The code mutates the FuncOp signature and adds the
"passthrough" attribute before validating the body, which can leave a
half-rewritten main if checks fail; move the structural validation (ensure
op.getBlocks().size() == 1 and that the block's terminator is a func::ReturnOp)
to run before calling rewriter.startOpModification(op) and before changing
op.setType(...) / op->setAttr(...), and only after those checks pass perform the
existing modification (use rewriter.startOpModification, set FunctionType via
FunctionType::get, set the "passthrough" ArrayAttr, then
rewriter.finalizeOpModification) followed by creating the constant and replacing
the return as currently implemented.
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 291-299: state.strings is being appended for every custom/barrier
op which can overflow the uint16_t index used by jeff.entrypoint; change the
collection logic around where state.strings is updated (the place that currently
calls state.strings.emplace_back(name) near the jeff::CustomOp::create call and
the similar block at the later occurrence) to intern/deduplicate names before
pushing (e.g., maintain a map<string,uint32_t> or set to assign a single index
per unique name), only push unique names into state.strings, and after
collection check that state.strings.size() <= UINT16_MAX and fail early with a
clear error if it exceeds UINT16_MAX before any cast to uint16_t or writing
jeff.entrypoint metadata.
- Around line 1466-1486: The pass currently leaves helper func ops with
qubit-typed signatures legal (see target.addDynamicallyLegalOp<func::FuncOp>)
which allows a func.func like `@foo`(%q: !qco.qubit) -> !qco.qubit to survive with
a lowered body and mixed IR; update the conversion so that any remaining
func.func/func.call/func.return with QCO qubit types are considered illegal (or
add dedicated conversion patterns for func.func/func.call/func.return that lower
all-qubit signatures) — specifically change the dynamical legality predicate on
func::FuncOp (and add similar checks for func::CallOp and func::ReturnOp) to
return false when any argument/result has type !qco.qubit, or implement and
register conversion patterns for those ops to mirror the existing all-qubit
lowering.
- Around line 1345-1364: The code mutates the FuncOp (calls
rewriter.startOpModification(op), op.setType(...),
op->removeAttr("passthrough"), rewriter.finalizeOpModification(op)) before
verifying structural preconditions, so if the single-block/func.return checks
fail matchAndRewrite leaves the op modified; move the structural checks (verify
op.getBlocks().size() == 1 and that block->getTerminator() is a func::ReturnOp)
to before any call to rewriter.startOpModification(op) (or, alternatively, wrap
the modification and call rewriter.cancelOperation or undo the changes on
failure) so the FuncOp is only mutated after the preconditions pass and the
dynamic-legality predicate remains correct.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 17-20: The check that sets the cache-only CMAKE_CXX_STANDARD
should also set the regular variable so an inherited non-cache value isn't left
in effect; before calling set(... CACHE ... FORCE) assign CMAKE_CXX_STANDARD=20
(e.g., set(CMAKE_CXX_STANDARD 20)) and then perform the cache set, ensuring both
the normal variable and the cache entry for CMAKE_CXX_STANDARD are updated to
C++20.
In `@mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp`:
- Around line 756-791: The branches handling custom ops in JeffToQCO.cpp do not
validate exact target/parameter arity before indexing into arrays; update each
branch (e.g., the "r" branch that calls createOneTargetTwoParameter<qco::ROp>,
the two-target branches that call
createTwoTargetZeroParameter<qco::iSWAPOp>/createTwoTargetZeroParameter<qco::DCXOp>/createTwoTargetZeroParameter<qco::ECROp>,
the two-parameter branches calling
createTwoTargetTwoParameter<qco::XXMinusYYOp>/createTwoTargetTwoParameter<qco::XXPlusYYOp>,
and the "barrier" branch that calls createBarrierOp) to explicitly check
adaptor.getInTargetQubits().size() and op.getParams().size() for the exact
expected counts and return rewriter.notifyMatchFailure(op, "<op> must have
exactly N target(s)/M parameter(s)") on mismatch before calling the create*
helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e91527c-6f7c-4d97-9920-7a581d188bd5
📒 Files selected for processing (3)
CMakeLists.txtmlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp (1)
766-796:⚠️ Potential issue | 🟠 MajorMissing arity validation for several custom op branches.
The "r" branch validates target count but not parameter count. The two-target ops (
iswap,dcx,ecr,xx_minus_yy,xx_plus_yy) don't validate target or parameter counts before passing to helper functions that index into these ranges.🛡️ Proposed validation additions
} else if (name == "r") { if (op.getInTargetQubits().size() != 1) { return rewriter.notifyMatchFailure( op, "Custom R operations must have exactly one target qubit"); } + if (op.getParams().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom R operations must have exactly two parameters"); + } createOneTargetTwoParameter<qco::ROp>(op, rewriter, op.getParams(), adaptor.getInCtrlQubits(), adaptor.getInTargetQubits()[0]); } else if (name == "iswap") { + if (adaptor.getInTargetQubits().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom iSWAP operations must have exactly two target qubits"); + } createTwoTargetZeroParameter<qco::iSWAPOp>( op, rewriter, adaptor.getInCtrlQubits(), adaptor.getInTargetQubits()); } else if (name == "dcx") { + if (adaptor.getInTargetQubits().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom DCX operations must have exactly two target qubits"); + } createTwoTargetZeroParameter<qco::DCXOp>( op, rewriter, adaptor.getInCtrlQubits(), adaptor.getInTargetQubits()); } else if (name == "ecr") { + if (adaptor.getInTargetQubits().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom ECR operations must have exactly two target qubits"); + } createTwoTargetZeroParameter<qco::ECROp>( op, rewriter, adaptor.getInCtrlQubits(), adaptor.getInTargetQubits()); } else if (name == "xx_minus_yy") { + if (adaptor.getInTargetQubits().size() != 2 || op.getParams().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom xx_minus_yy must have exactly 2 targets and 2 parameters"); + } createTwoTargetTwoParameter<qco::XXMinusYYOp>( op, rewriter, op.getParams(), adaptor.getInCtrlQubits(), adaptor.getInTargetQubits()); } else if (name == "xx_plus_yy") { + if (adaptor.getInTargetQubits().size() != 2 || op.getParams().size() != 2) { + return rewriter.notifyMatchFailure( + op, "Custom xx_plus_yy must have exactly 2 targets and 2 parameters"); + } createTwoTargetTwoParameter<qco::XXPlusYYOp>(op, rewriter, op.getParams(), adaptor.getInCtrlQubits(), adaptor.getInTargetQubits());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp` around lines 766 - 796, The branches for custom ops lack full arity checks: ensure "r" verifies op.getParams().size()==2 before calling createOneTargetTwoParameter<qco::ROp> and return rewriter.notifyMatchFailure(op, "Custom R operations must have exactly two parameters") if not; for the two-target ops (createTwoTargetZeroParameter and createTwoTargetTwoParameter) validate adaptor.getInTargetQubits().size()==2 and for zero-parameter variants ensure op.getParams().empty(), for two-parameter variants ensure op.getParams().size()==2, returning rewriter.notifyMatchFailure with clear messages like "Custom <name> must have exactly two target qubits" or "must have N parameters" when checks fail; place these validations immediately before the respective helper calls (e.g., before createTwoTargetZeroParameter<qco::iSWAPOp>, createTwoTargetZeroParameter<qco::DCXOp>, createTwoTargetZeroParameter<qco::ECROp>, createTwoTargetTwoParameter<qco::XXMinusYYOp>, createTwoTargetTwoParameter<qco::XXPlusYYOp>).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCOToJeff/CMakeLists.txt`:
- Around line 11-24: The CMake fragment unconditionally creates target QCOToJeff
and calls target_include_directories using ${jeff-mlir_SOURCE_DIR} and
${jeff-mlir_BINARY_DIR}, which are undefined when BUILD_JEFF_MLIR_TRANSLATION is
off; wrap the addition of the QCOToJeff target and the
target_include_directories call in an if(BUILD_JEFF_MLIR_TRANSLATION) ...
endif() guard so the QCOToJeff target (and any Jeff-to-QCO conversion subdirs)
are only added when BUILD_JEFF_MLIR_TRANSLATION is enabled, leaving the rest of
the build path untouched.
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 716-726: The example for qco.r is wrong because the converter
currently emits is_adjoint=false by default and only flips it when the op is
wrapped in qco.inv; change the conversion logic in QCOToJeff.cpp so that when
lowering qco.r to jeff.custom you set is_adjoint = true by default (for the
plain qco.r op) and then invert that value if the source op is a qco.inv
wrapper; update the same logic used at the qco.r conversion site so
jeff.custom's is_adjoint matches the documented example.
- Around line 1337-1347: The code currently rejects any func.func with a
passthrough attribute and later drops the entire passthrough attribute, but the
transformation should only special-case the "entry_point" marker: change the
checks that use op->getAttrOfType<ArrayAttr>("passthrough") and
llvm::any_of(...) so they only treat functions as special when the ArrayAttr
contains the StringAttr "entry_point" (do not fail for other passthroughs), and
when removing the marker (the code that currently erases the whole passthrough
attribute), modify it to remove only the "entry_point" element from the
ArrayAttr (preserving any other passthrough entries) instead of deleting the
entire attribute; update both the early-failure/validation site and the
attribute-removal site to use this same logic.
---
Duplicate comments:
In `@mlir/lib/Conversion/JeffToQCO/JeffToQCO.cpp`:
- Around line 766-796: The branches for custom ops lack full arity checks:
ensure "r" verifies op.getParams().size()==2 before calling
createOneTargetTwoParameter<qco::ROp> and return rewriter.notifyMatchFailure(op,
"Custom R operations must have exactly two parameters") if not; for the
two-target ops (createTwoTargetZeroParameter and createTwoTargetTwoParameter)
validate adaptor.getInTargetQubits().size()==2 and for zero-parameter variants
ensure op.getParams().empty(), for two-parameter variants ensure
op.getParams().size()==2, returning rewriter.notifyMatchFailure with clear
messages like "Custom <name> must have exactly two target qubits" or "must have
N parameters" when checks fail; place these validations immediately before the
respective helper calls (e.g., before
createTwoTargetZeroParameter<qco::iSWAPOp>,
createTwoTargetZeroParameter<qco::DCXOp>,
createTwoTargetZeroParameter<qco::ECROp>,
createTwoTargetTwoParameter<qco::XXMinusYYOp>,
createTwoTargetTwoParameter<qco::XXPlusYYOp>).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dff3c6f-29b4-405d-b55f-58aab0133184
📒 Files selected for processing (3)
mlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/CMakeLists.txtmlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 1367-1370: The current code inserts a new func::ReturnOp at the
end of the block (using rewriter.setInsertionPointToEnd +
func::ReturnOp::create) and then erases the old returnOp, which can leave the
block in a transient invalid state; instead, replace the existing terminator
in-place by using the Rewriter's replacement API (e.g.
rewriter.replaceOpWithNewOp for func::ReturnOp) to create the new return at the
location of returnOp and remove the old op atomically, replacing the sequence of
rewriter.setInsertionPointToEnd, func::ReturnOp::create and
rewriter.eraseOp(returnOp) with a single replaceOpWithNewOp call targeting
returnOp and using returnOp->getLoc().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45cd74e2-ff28-499d-b0e2-4c04d2051359
📒 Files selected for processing (1)
mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Nicely done! I think this is ready to merge for now!
We likely want to tweak the integration a bit more, but this feels like a great first start. Feel free to comment/resolve on the final open comments and then merge yourself whenever you are happy!
|
Ah, one thing that I missed: Additionally, it could be good to add a sub-issue to #1196 that is resolved by this PR. Hope that makes sense. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Conversion/JeffToQCO/JeffToQCO.td`:
- Around line 14-27: Update the pass description (the description variable for
the JeffToQCO pass) to explicitly state that the current implementation only
supports modules with a single entry function and does not convert qubit-typed
helper func.func / func.call / func.return yet; edit the multi-line text where
description is defined to add a short sentence noting the single-entry-function
restriction and deferral of qubit-typed helper function/call/return conversion
to a future version so the help text matches the actual accepted input shape.
- Around line 29-32: The TableGen dialect list `dependentDialects` is missing
arith, causing an implicit dependency; update the list that currently contains
"mlir::jeff::JeffDialect" and "mlir::qco::QCODialect" to also include
"mlir::arith::ArithDialect" so the JeffToQCO pass (see JeffToQCO.cpp where
arith::ArithDialect is made legal and Jeff constant-to-arith patterns are
registered) will have its dependency declared explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53aa6746-af35-4067-8730-14b81059fa88
📒 Files selected for processing (2)
CHANGELOG.mdmlir/include/mlir/Conversion/JeffToQCO/JeffToQCO.td
Thanks a lot for the review! 🙂
Makes sense! Done.
Also makes sense! I have created #1546 and marked this PR accordingly. |
denialhaag
dismissed
coderabbitai[bot]’s stale review
@coderabbitai has resolved its last comments and finished its review, but it has forgotten to approve the PR.
2 participants
Description
This PR adds conversions for the Jeff dialect.
Fixes #1546
Checklist:
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).