-
-
Notifications
You must be signed in to change notification settings - Fork 63
✨ Add absorb-swaps pass
#1750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
✨ Add absorb-swaps pass
#1750
Changes from all commits
00ad822
77250d1
515e6bf
f6e1c39
b0474ff
10302c5
da710c7
d793c73
de89411
3dbe1a3
eea2685
3b2af5d
6312c37
1a884d8
c4bb64f
a779361
3bf15be
ac2a49e
03917b6
8948b78
982efb7
343387d
a108a9b
5b9c7f7
44dfccc
4d762f5
806a4a9
4ef7620
6843409
4584e2d
80e5b3c
bee32c9
ada1d3e
4dac9d2
bda8d0f
7b06a2c
6776770
4b1cd34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| /* | ||
| * Copyright (c) 2023 - 2026 Chair for Design Automation, TUM | ||
| * Copyright (c) 2025 - 2026 Munich Quantum Software Company GmbH | ||
| * All rights reserved. | ||
| * | ||
| * SPDX-License-Identifier: MIT | ||
| * | ||
| * Licensed under the MIT License | ||
| */ | ||
|
|
||
| #include "mlir/Dialect/QCO/IR/QCOOps.h" | ||
| #include "mlir/Dialect/QCO/Transforms/Passes.h" | ||
| #include "mlir/Dialect/QCO/Utils/Drivers.h" | ||
| #include "mlir/Dialect/QCO/Utils/WireIterator.h" | ||
|
|
||
| #include <mlir/Dialect/Func/IR/FuncOps.h> | ||
| #include <mlir/IR/BuiltinOps.h> | ||
| #include <mlir/IR/PatternMatch.h> | ||
| #include <mlir/Support/LLVM.h> | ||
| #include <mlir/Support/WalkResult.h> | ||
|
|
||
| namespace mlir::qco { | ||
| #define GEN_PASS_DEF_SWAPABSORPTION | ||
| #include "mlir/Dialect/QCO/Transforms/Passes.h.inc" | ||
|
|
||
| namespace { | ||
| struct SwapAbsorption : impl::SwapAbsorptionBase<SwapAbsorption> { | ||
| using SwapAbsorptionBase::SwapAbsorptionBase; | ||
|
|
||
| protected: | ||
| void runOnOperation() override { | ||
| ModuleOp anchor = getOperation(); | ||
| IRRewriter rewriter(&getContext()); | ||
|
|
||
| for (auto func : anchor.getOps<func::FuncOp>()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 /**
* @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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll have a look at that 👍
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @burgholzer here. One could even probably make an argument to add this pattern to the canonicalization pass instead of adding a new pass altogether (and all the boilerplate which comes along with it). This would again improve performance by applying the patterns in the same driver run as all the other patterns.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question in terms of clarification: this would run per operation in the circuit, right? it definitely seems more efficient, but does it cover all our required cases?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can still use the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue against a canonicalization pattern here. I had this suggestion in my review already. However, this would mean that the pattern would try to match every single swap operation in a circuit while it would only really match very little. |
||
| SmallVector<SWAPOp> readyToAbsorb; | ||
| SmallVector<WireIterator> wires; | ||
| do { | ||
| wires.clear(); | ||
| for (auto op : func.getOps<StaticOp>()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will collect all static operations repeatedly in every iteration of the |
||
| wires.emplace_back(op.getQubit()); | ||
| } | ||
| if (wires.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| readyToAbsorb.clear(); | ||
| findSwapsReadyForAbsorption(wires, readyToAbsorb); | ||
|
|
||
| for (auto swapOp : readyToAbsorb) { | ||
| rewriter.replaceOp(swapOp, | ||
| {swapOp.getQubit1In(), swapOp.getQubit0In()}); | ||
| } | ||
| } while (!readyToAbsorb.empty()); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| static void findSwapsReadyForAbsorption(MutableArrayRef<WireIterator> wires, | ||
| SmallVector<SWAPOp>& readyToAbsorb) { | ||
| std::ignore = walkProgramGraph<WireDirection::Forward>( | ||
|
Check warning on line 61 in mlir/lib/Dialect/QCO/Transforms/Optimizations/SwapAbsorption.cpp
|
||
| wires, [&](const ReadyRange& ready, ReleasedOps& released) { | ||
| for (const auto& [op, indices] : ready) { | ||
| if (isa<SWAPOp>(op)) { | ||
| readyToAbsorb.emplace_back(op); | ||
| } | ||
| released.emplace_back(op); | ||
| } | ||
| return WalkResult::interrupt(); | ||
| }); | ||
| } | ||
| }; | ||
| } // namespace | ||
| } // namespace mlir::qco | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll have a look at that 👍 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| /* | ||
| * Copyright (c) 2023 - 2026 Chair for Design Automation, TUM | ||
| * Copyright (c) 2025 - 2026 Munich Quantum Software Company GmbH | ||
| * All rights reserved. | ||
| * | ||
| * SPDX-License-Identifier: MIT | ||
| * | ||
| * Licensed under the MIT License | ||
| */ | ||
|
|
||
| #include "mlir/Dialect/QCO/Builder/QCOProgramBuilder.h" | ||
| #include "mlir/Dialect/QCO/IR/QCODialect.h" | ||
| #include "mlir/Dialect/QCO/IR/QCOOps.h" | ||
| #include "mlir/Dialect/QCO/Transforms/Passes.h" | ||
|
|
||
| #include <gtest/gtest.h> | ||
| #include <llvm/Support/LogicalResult.h> | ||
| #include <mlir/Dialect/Arith/IR/Arith.h> | ||
| #include <mlir/Dialect/Func/IR/FuncOps.h> | ||
| #include <mlir/IR/BuiltinOps.h> | ||
| #include <mlir/IR/DialectRegistry.h> | ||
| #include <mlir/IR/OperationSupport.h> | ||
| #include <mlir/IR/OwningOpRef.h> | ||
| #include <mlir/IR/Value.h> | ||
| #include <mlir/Pass/PassManager.h> | ||
| #include <mlir/Support/LLVM.h> | ||
|
|
||
| #include <cassert> | ||
| #include <memory> | ||
|
|
||
| using namespace mlir; | ||
| using namespace mlir::qco; | ||
|
|
||
| namespace { | ||
|
|
||
| class SwapAbsorbPassTest : public testing::Test { | ||
|
|
||
| protected: | ||
| void SetUp() override { | ||
| DialectRegistry registry; | ||
| registry.insert<qco::QCODialect, arith::ArithDialect, func::FuncDialect>(); | ||
| context = std::make_unique<MLIRContext>(); | ||
| context->appendDialectRegistry(registry); | ||
| context->loadAllAvailableDialects(); | ||
| } | ||
|
|
||
| static void applySwapAbsorb(OwningOpRef<ModuleOp>& moduleOp) { | ||
| PassManager pm(moduleOp->getContext()); | ||
| pm.addPass(qco::createSwapAbsorption()); | ||
| auto res = pm.run(*moduleOp); | ||
|
|
||
| ASSERT_TRUE(succeeded(res)); | ||
| } | ||
|
|
||
| std::unique_ptr<MLIRContext> context; | ||
| }; | ||
| }; // namespace | ||
|
|
||
| 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)); | ||
| } | ||
|
Comment on lines
+59
to
+80
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of a pointless test. Why would the pass change a program that does not contain the single operation the pass actually matches on?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for me its a requirement that circuits without swaps remain untouched. nevertheless, feel free to reach out, if you are uncomfortable with this. i'll remove this test then 👍 |
||
|
|
||
| TEST_F(SwapAbsorbPassTest, PassReordersTwoQubitCircuitWithLeadingSwap) { | ||
|
|
||
| qco::QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
|
|
||
| const auto q00 = builder.staticQubit(0); | ||
| const auto q10 = builder.staticQubit(1); | ||
|
|
||
| const auto [q01, q11] = builder.swap(q00, q10); | ||
|
|
||
| const auto q02 = builder.id(q01); | ||
| const auto q12 = builder.id(q11); | ||
|
Comment on lines
+92
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kind of pointless to use identity gates here, as they would generally be optimized away. Prefer to use non-trivial gates, if at all.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| builder.sink(q02); | ||
| builder.sink(q12); | ||
|
|
||
| auto moduleThroughPass = builder.finalize(); | ||
| applySwapAbsorb(moduleThroughPass); | ||
|
|
||
| ASSERT_EQ(q10, mlir::cast<IdOp>(q02.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q00, mlir::cast<IdOp>(q12.getDefiningOp()).getInputQubit(0)); | ||
| } | ||
|
|
||
| TEST_F(SwapAbsorbPassTest, PassAbsorbsTwoIndependentSwaps) { | ||
|
|
||
| qco::QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
|
|
||
| const auto q00 = builder.staticQubit(0); | ||
| const auto q10 = builder.staticQubit(1); | ||
| const auto q20 = builder.staticQubit(2); | ||
| const auto q30 = builder.staticQubit(3); | ||
|
|
||
| const auto [q01, q11] = builder.swap(q00, q10); | ||
| const auto [q21, q31] = builder.swap(q20, q30); | ||
|
|
||
| const auto q02 = builder.id(q01); | ||
| const auto q12 = builder.id(q11); | ||
| const auto q22 = builder.id(q21); | ||
| const auto q32 = builder.id(q31); | ||
|
|
||
| builder.sink(q02); | ||
| builder.sink(q12); | ||
| builder.sink(q22); | ||
| builder.sink(q32); | ||
|
|
||
| auto moduleThroughPass = builder.finalize(); | ||
| applySwapAbsorb(moduleThroughPass); | ||
|
|
||
| ASSERT_EQ(q10, mlir::cast<IdOp>(q02.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q00, mlir::cast<IdOp>(q12.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q30, mlir::cast<IdOp>(q22.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q20, mlir::cast<IdOp>(q32.getDefiningOp()).getInputQubit(0)); | ||
| } | ||
|
|
||
| TEST_F(SwapAbsorbPassTest, PassAbsorbsSwapWithLeadingSingleQubitGates) { | ||
|
|
||
| qco::QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
|
|
||
| const auto q00 = builder.staticQubit(0); | ||
| const auto q10 = builder.staticQubit(1); | ||
|
|
||
| const auto q01 = builder.id(q00); | ||
| const auto q11 = builder.id(q10); | ||
|
|
||
| const auto [q02, q12] = builder.swap(q01, q11); | ||
|
|
||
| const auto q03 = builder.id(q02); | ||
| const auto q13 = builder.id(q12); | ||
|
|
||
| builder.sink(q03); | ||
| builder.sink(q13); | ||
|
|
||
| auto moduleThroughPass = builder.finalize(); | ||
| applySwapAbsorb(moduleThroughPass); | ||
|
|
||
| ASSERT_EQ(q11, mlir::cast<IdOp>(q03.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q01, mlir::cast<IdOp>(q13.getDefiningOp()).getInputQubit(0)); | ||
| } | ||
|
|
||
| TEST_F(SwapAbsorbPassTest, PassAbsorbsTwoDependentSwaps) { | ||
|
|
||
| qco::QCOProgramBuilder builder(context.get()); | ||
| builder.initialize(); | ||
|
|
||
| const auto q00 = builder.staticQubit(0); | ||
| const auto q10 = builder.staticQubit(1); | ||
| const auto q20 = builder.staticQubit(2); | ||
|
|
||
| const auto [q01, q11] = builder.swap(q00, q10); | ||
| const auto [q12, q21] = builder.swap(q11, q20); | ||
|
|
||
| const auto q02 = builder.id(q01); | ||
| const auto q13 = builder.id(q12); | ||
| const auto q22 = builder.id(q21); | ||
|
|
||
| builder.sink(q02); | ||
| builder.sink(q13); | ||
| builder.sink(q22); | ||
|
|
||
| auto moduleThroughPass = builder.finalize(); | ||
| applySwapAbsorb(moduleThroughPass); | ||
|
|
||
| ASSERT_EQ(q20, mlir::cast<IdOp>(q13.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q00, mlir::cast<IdOp>(q22.getDefiningOp()).getInputQubit(0)); | ||
| ASSERT_EQ(q10, mlir::cast<IdOp>(q02.getDefiningOp()).getInputQubit(0)); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toAllocOpas well? In the pattern rewrite proposed in the other comment, that is likely to be fairly simple. Taking it one step further,qtensor.loadcould also qualify for such a simplification. And to go even further beyond, any mixture ofStaticOp,AllocOporLoadOpcan equally be simplified.