✨ Add absorb-swaps pass#1750
Conversation
…-quantum-toolkit/core into feat/swap-absorption-pass
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
works fine now 💪 i have another conceptional question though: which of the following circuits is correct? or: in my opinion its the second circuit 201, but the implementation we discussed produces the first result. |
|
the linter produces warnings on the one hand, but failes because of a http-error 403 (unauthorized) on the other hand. see here: https://github.com/munich-quantum-toolkit/core/actions/runs/26597942119/job/78373906843?pr=1750 is there anything i can do about it? |
Unfortunately, the action cannot post comments on PRs coming from a fork. 😕 That said, the report can also be found in the summary of the run: https://github.com/munich-quantum-toolkit/core/actions/runs/26597942119/attempts/1#summary-78373906843 |
The best way to visualize this is to apply the SWAPs sequentially: |
so if i resolve all the warnings the lint check will succeed, as it does not post a comment? |
If you resolve all warnings the |
Which way is recommended? |
Co-authored-by: matthias <matthias@bereumann.com> Signed-off-by: Johannes Moosburger <96540096+jmoosburger@users.noreply.github.com>
Signed-off-by: Johannes Moosburger <96540096+jmoosburger@users.noreply.github.com>
Thanks for your help and patience! |
|
@jmoosburger I've pushed some minor changes directly. Let's see what @burgholzer has to say, but I think this one should be good to go 🚀 |
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
left a comment
There was a problem hiding this comment.
Finally found some time to look at this. Thanks for your work on this @jmoosburger and @MatthiasReumann 🙏🏼
As you will probably see from the comments, I am not quite happy with this yet. Most of the comments are inline and should hopefully be rather self-explanatory. Let me know if anything is not clear.
One other issue that I still have with this is that the new pass is not added to the default pass pipeline, which is an oversight in my opinion. Why would we implement a pass when we do not intend to use it?
| SmallVector<WireIterator> wires; | ||
| do { | ||
| wires.clear(); | ||
| for (auto op : func.getOps<StaticOp>()) { |
There was a problem hiding this comment.
This will collect all static operations repeatedly in every iteration of the do...while loop, which seems fairly wasteful. Pretty sure there is a more efficient way to do this.
| ModuleOp anchor = getOperation(); | ||
| IRRewriter rewriter(&getContext()); | ||
|
|
||
| for (auto func : anchor.getOps<func::FuncOp>()) { |
There was a problem hiding this comment.
I am really wondering about the efficiency of this pass. I already pushed a couple of improvements, but I can't help it and feel like this is not the way to go.
Is there any particular reason why this pass cannot simply be one that matches on StaticOp and checks for subsequent SWAPOp that also have a StaticOp as there second input?
Something as simple as
/**
* @brief Absorb SWAP operations into static qubit allocations
*/
struct RemoveSWAPAfterStaticAllocation final : OpRewritePattern<SWAPOp> {
using OpRewritePattern::OpRewritePattern;
LogicalResult matchAndRewrite(SWAPOp op,
PatternRewriter& rewriter) const override {
auto qubit0 = op.getInputQubit(0);
if (!isa<StaticOp>(qubit0.getDefiningOp())) {
return failure();
}
auto qubit1 = op.getInputQubit(1);
if (!isa<StaticOp>(qubit1.getDefiningOp())) {
return failure();
}
rewriter.replaceOp(op, {qubit1, qubit0});
return success();
}
};This feels way simpler and more efficient than the pass implementation here.
| let summary = "This pass absorbs SWAP operations into the initial " | ||
| "program-to-hardware mapping."; | ||
| let description = [{ | ||
| For a SWAP operation exchanging static qubits q0 and q1, the pass replaces the use of the | ||
| first (second) input qubit with the second (first) output qubit of the SWAP and subsequently | ||
| removes the operation. As a result, the initial program-to-hardware mapping is changed. | ||
| This process is repeated until no more SWAP operations can be absorbed. | ||
|
|
||
| The pass assumes that the quantum program is already mapped to static qubits. | ||
| }]; |
There was a problem hiding this comment.
I think the wording here could be improved; maybe even the naming of the pass itself. As it stands, the name of the pass is not self explanatory.
"initial program-to-hardware mapping" is something that is oddly specific for a pass that just generally matches static operations and checks if any SWAPs are directly applied after the "allocation".
I am even wondering whether this should actually be limited to StaticOp. Why not extend the logic to AllocOp as well? In the pattern rewrite proposed in the other comment, that is likely to be fairly simple. Taking it one step further, qtensor.load could also qualify for such a simplification. And to go even further beyond, any mixture of StaticOp, AllocOp or LoadOp can equally be simplified.
| TEST_F(SwapAbsorbPassTest, PassDoesNotChangeSwaplessProgram) { | ||
|
|
||
| qco::QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
|
|
||
| const auto q00 = builder.staticQubit(0); | ||
| const auto q10 = builder.staticQubit(1); | ||
|
|
||
| const auto q01 = builder.h(q00); | ||
| const auto [q02, q11] = builder.cx(q01, q10); | ||
|
|
||
| builder.sink(q02); | ||
| builder.sink(q11); | ||
|
|
||
| auto moduleThroughPass = builder.finalize(); | ||
| auto originalModule = moduleThroughPass->clone(); | ||
|
|
||
| applySwapAbsorb(moduleThroughPass); | ||
| ASSERT_TRUE(mlir::OperationEquivalence::isEquivalentTo( | ||
| moduleThroughPass.get(), originalModule, | ||
| mlir::OperationEquivalence::Flags::IgnoreLocations)); | ||
| } |
There was a problem hiding this comment.
Kind of a pointless test. Why would the pass change a program that does not contain the single operation the pass actually matches on?
There was a problem hiding this comment.
for me its a requirement that circuits without swaps remain untouched.
from the tests point of view it's not possible to determine where exactly the pass matches on.
you cannot be sure a future implementation of this pass accidentally changes a circuit without a swap unless you test it.
nevertheless, feel free to reach out, if you are uncomfortable with this. i'll remove this test then 👍
| const auto q02 = builder.id(q01); | ||
| const auto q12 = builder.id(q11); |
There was a problem hiding this comment.
Kind of pointless to use identity gates here, as they would generally be optimized away. Prefer to use non-trivial gates, if at all.
There was a problem hiding this comment.
it doesn't matter, which single qubit gate is used at this place, to test this particular pass. a gate is required to check, whether the pass reorders the circuit correctly. i took identity as it is the simplest one and does not add functionality to the test circuit
| ASSERT_EQ(q10, ((IdOp)q02.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q00, ((IdOp)q12.getDefiningOp()).getInputQubit(0)); |
There was a problem hiding this comment.
Do not use C-style casts. I am surprised clang-tidy is not screaming at this.
There was a problem hiding this comment.
done and replaced with mlir:cast
There was a problem hiding this comment.
These tests do not really follow the structure of most of the other tests, which always build a reference program and use our IR verifier to check for equivalence.
I'd prefer to have this as uniform as possible.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Johannes Moosburger <96540096+jmoosburger@users.noreply.github.com>
|
thanks for your feedback 🙏
this is actually no oversight but results from this comment: #1661 (comment) |
## Description This PR should improve the stability of some of the timing-based QDMI tests. We have recently seen them failing more and more. ## 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.
…t#1796) Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Description
Checklist
I have added migration instructions to the upgrade guide (if needed).If PR contains AI-assisted content:
I have disclosed the use of AI tools in the PR description as per our AI Usage Guidelines.AI-assisted commits include anAssisted-by: [Model Name] via [Tool Name]footer.I confirm that I have personally reviewed and understood all AI-generated content, and accept full responsibility for it.